Merge lp:~boiko/history-service/rtm-fix_group_thread_matching into lp:history-service/rtm-14.09

[Gustavo Pichorim Boiko]

Use QSqlQuery::bindValue() to pass filter arguments to the query to prevent errors.

Use QSqlQuery::bindValue() to pass filter arguments to the query to prevent errors.

== Checklist ==
Are there any related MPs required for this MP to build/function as expected? Please list.
No

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
Yes

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?
Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/<package-name>) on device or emulator?
Yes

If you changed the UI, was the change specified/approved by design?
N/A

If you changed UI labels, did you update the pot file?
N/A

If you changed the packaging (debian), did you add a core-dev as a reviewer to this MP?
N/A

[Tiago Salem Herrmann (tiagosh)]

Looks good.

--Checklist--
Did you perform an exploratory manual test run of the code change and any related functionality on
device or emulator?
Yes

Did CI run pass? If not, please explain why.
No, it does not run for rtm branches.

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
Yes

review: Approve

=== modified file 'plugins/sqlite/sqlitehistoryeventview.cpp'
--- plugins/sqlite/sqlitehistoryeventview.cpp	2015-01-23 20:21:34 +0000
+++ plugins/sqlite/sqlitehistoryeventview.cpp	2015-01-28 23:16:22 +0000
@@ -38,7 +38,8 @@
     mQuery.setForwardOnly(true);
 
     // FIXME: validate the filter
-    QString condition = mPlugin->filterToString(filter);
+    QVariantMap filterValues;
+    QString condition = mPlugin->filterToString(filter, filterValues);
     QString order;
     if (!sort.sortField().isNull()) {
         order = QString("ORDER BY %1 %2").arg(sort.sortField(), sort.sortOrder() == Qt::AscendingOrder ? "ASC" : "DESC");
@@ -48,7 +49,18 @@
     QString queryText = QString("CREATE TEMP TABLE %1 AS ").arg(mTemporaryTable);
     queryText += mPlugin->sqlQueryForEvents(type, condition, order);
 
-    if (!mQuery.exec(queryText)) {
+    if (!mQuery.prepare(queryText)) {
+        mValid = false;
+        Q_EMIT Invalidated();
+        qCritical() << "Error:" << mQuery.lastError() << mQuery.lastQuery();
+        return;
+    }
+
+    Q_FOREACH(const QString &key, filterValues.keys()) {
+        mQuery.bindValue(key, filterValues[key]);
+    }
+
+    if (!mQuery.exec()) {
         mValid = false;
         Q_EMIT Invalidated();
         qCritical() << "Error:" << mQuery.lastError() << mQuery.lastQuery();

=== modified file 'plugins/sqlite/sqlitehistoryplugin.cpp'
--- plugins/sqlite/sqlitehistoryplugin.cpp	2015-01-25 13:25:56 +0000
+++ plugins/sqlite/sqlitehistoryplugin.cpp	2015-01-28 23:16:22 +0000
@@ -662,7 +662,7 @@
     return QDateTime(timestamp.date(), timestamp.time(), Qt::UTC).toLocalTime().toString("yyyy-MM-ddTHH:mm:ss.zzz");
 }
 
-QString SQLiteHistoryPlugin::filterToString(const History::Filter &filter, const QString &propertyPrefix) const
+QString SQLiteHistoryPlugin::filterToString(const History::Filter &filter, QVariantMap &bindValues, const QString &propertyPrefix) const
 {
     QString result;
     History::Filters filters;
@@ -690,7 +690,7 @@
         count = filters.count();
         for (int i = 0; i < count; ++i) {
             // run recursively through the inner filters
-            result += QString("(%1)").arg(filterToString(filters[i], propertyPrefix));
+            result += QString("(%1)").arg(filterToString(filters[i], bindValues, propertyPrefix));
             if (i != count-1) {
                 result += linking;
             }
@@ -698,36 +698,20 @@
         result += " )";
         break;
     default:
-        // FIXME: remove the toString() functionality or replace it by a better implementation
         if (filterProperty.isEmpty() || filterValue.isNull()) {
             break;
         }
 
-        switch (filterValue.type()) {
-        case QVariant::String:
-            // FIXME: need to escape strings
-            // wrap strings
-            value = QString("'%1'").arg(escapeFilterValue(filterValue.toString()));
-            break;
-        case QVariant::Bool:
-            value = filterValue.toBool() ? "1" : "0";
-            break;
-        case QVariant::Int:
-            value = QString::number(filterValue.toInt());
-            break;
-        case QVariant::Double:
-            value = QString::number(filterValue.toDouble());
-            break;
-        default:
-            value = filterValue.toString();
-        }
+        QString bindId = QString(":filterValue%1").arg(bindValues.count());
 
         QString propertyName = propertyPrefix.isNull() ? filterProperty : QString("%1.%2").arg(propertyPrefix, filterProperty);
         // FIXME: need to check for other match flags and multiple match flags
         if (filter.matchFlags() & History::MatchContains) {
+            // FIXME: maybe we should use QString("%1 LIKE '\%'||%2'\%'").arg(bindId) ?? needs more time for investigating
             result = QString("%1 LIKE '\%%2\%' ESCAPE '\\'").arg(propertyName, escapeFilterValue(filterValue.toString()));
         } else {
-            result = QString("%1=%2").arg(propertyName, value);
+            result = QString("%1=%2").arg(propertyName, bindId);
+            bindValues[bindId] = filterValue;
         }
     }
 

=== modified file 'plugins/sqlite/sqlitehistoryplugin.h'
--- plugins/sqlite/sqlitehistoryplugin.h	2015-01-24 19:34:40 +0000
+++ plugins/sqlite/sqlitehistoryplugin.h	2015-01-28 23:16:22 +0000
@@ -80,7 +80,7 @@
 
     static QString toLocalTimeString(const QDateTime &timestamp);
 
-    QString filterToString(const History::Filter &filter, const QString &propertyPrefix = QString::null) const;
+    QString filterToString(const History::Filter &filter, QVariantMap &bindValues, const QString &propertyPrefix = QString::null) const;
     QString escapeFilterValue(const QString &value) const;
 };
 

=== modified file 'plugins/sqlite/sqlitehistorythreadview.cpp'
--- plugins/sqlite/sqlitehistorythreadview.cpp	2015-01-23 20:21:34 +0000
+++ plugins/sqlite/sqlitehistorythreadview.cpp	2015-01-28 23:16:22 +0000
@@ -39,7 +39,8 @@
     mQuery.setForwardOnly(true);
 
     // FIXME: validate the filter
-    QString condition = mPlugin->filterToString(filter);
+    QVariantMap filterValues;
+    QString condition = mPlugin->filterToString(filter, filterValues);
     QString order;
     if (!sort.sortField().isNull()) {
         order = QString("ORDER BY %1 %2").arg(sort.sortField(), sort.sortOrder() == Qt::AscendingOrder ? "ASC" : "DESC");
@@ -49,8 +50,19 @@
     QString queryText = QString("CREATE TEMP TABLE %1 AS ").arg(mTemporaryTable);
     queryText += mPlugin->sqlQueryForThreads(type, condition, order);
 
+    if (!mQuery.prepare(queryText)) {
+        qCritical() << "Error:" << mQuery.lastError() << mQuery.lastQuery();
+        mValid = false;
+        Q_EMIT Invalidated();
+        return;
+    }
+
+    Q_FOREACH(const QString &key, filterValues.keys()) {
+        mQuery.bindValue(key, filterValues[key]);
+    }
+
     // create the temporary table
-    if (!mQuery.exec(queryText)) {
+    if (!mQuery.exec()) {
         qCritical() << "Error:" << mQuery.lastError() << mQuery.lastQuery();
         mValid = false;
         Q_EMIT Invalidated();

=== modified file 'plugins/sqlite/tests/SqlitePluginTest.cpp'
--- plugins/sqlite/tests/SqlitePluginTest.cpp	2015-01-25 13:25:56 +0000
+++ plugins/sqlite/tests/SqlitePluginTest.cpp	2015-01-28 23:16:22 +0000
@@ -718,35 +718,41 @@
 void SqlitePluginTest::testFilterToString_data()
 {
     QTest::addColumn<QVariantMap>("filterProperties");
+    QTest::addColumn<QVariantMap>("filterValues");
     QTest::addColumn<QString>("propertyPrefix");
     QTest::addColumn<QString>("resultString");
 
     History::Filter filter;
+    QVariantMap filterValues;
     filter.setFilterProperty("testProperty");
     filter.setFilterValue("stringValue");
-    QTest::newRow("simple string filter") << filter.properties() << QString() << "testProperty='stringValue'";
-
-    filter.setFilterValue("escaped\\0\\n%");
-    QTest::newRow("check that strings are escaped") << filter.properties() << QString() << "testProperty='escaped\\\\0\\\\n\\%'";
+    filterValues[":filterValue0"] = filter.filterValue();
+    QTest::newRow("simple string filter") << filter.properties() << filterValues << QString() << "testProperty=:filterValue0";
 
     filter.setFilterValue(12345);
-    QTest::newRow("simple integer filter") << filter.properties() << QString() << "testProperty=12345";
+    filterValues[":filterValue0"] = filter.filterValue();
+    QTest::newRow("simple integer filter") << filter.properties() << filterValues << QString() << "testProperty=:filterValue0";
 
     filter.setFilterValue(true);
-    QTest::newRow("simple true boolean filter") << filter.properties() << QString() << "testProperty=1";
+    filterValues[":filterValue0"] = filter.filterValue();
+    QTest::newRow("simple true boolean filter") << filter.properties() << filterValues << QString() << "testProperty=:filterValue0";
 
     filter.setFilterValue(false);
-    QTest::newRow("simple false boolean filter") << filter.properties() << QString() << "testProperty=0";
+    filterValues[":filterValue0"] = filter.filterValue();
+    QTest::newRow("simple false boolean filter") << filter.properties() << filterValues << QString() << "testProperty=:filterValue0";
 
     filter.setFilterValue(12345);
-    QTest::newRow("filter with a prefix") << filter.properties() << QString("prefix") << "prefix.testProperty=12345";
+    filterValues[":filterValue0"] = filter.filterValue();
+    QTest::newRow("filter with a prefix") << filter.properties() << filterValues << QString("prefix") << "prefix.testProperty=:filterValue0";
 
     filter.setMatchFlags(History::MatchContains);
     filter.setFilterValue("partialString");
-    QTest::newRow("match contains") << filter.properties() << QString() << "testProperty LIKE '\%partialString\%' ESCAPE '\\'";
+    filterValues.clear();
+    QTest::newRow("match contains") << filter.properties() << filterValues << QString() << "testProperty LIKE '\%partialString\%' ESCAPE '\\'";
 
     filter.setFilterValue("%");
-    QTest::newRow("partial match escaped") << filter.properties() << QString() << "testProperty LIKE '\%\\\%\%' ESCAPE '\\'";
+    filterValues.clear();
+    QTest::newRow("partial match escaped") << filter.properties() << filterValues << QString() << "testProperty LIKE '\%\\\%\%' ESCAPE '\\'";
 
     History::IntersectionFilter intersectionFilter;
     filter.setMatchFlags(History::MatchFlags());
@@ -756,7 +762,11 @@
     intersectionFilter.append(filter);
     filter.setFilterValue("a string");
     intersectionFilter.append(filter);
-    QTest::newRow("intersection filter") << intersectionFilter.properties() << QString() << "( (testProperty=12345) AND (testProperty=1) AND (testProperty='a string') )";
+    filterValues.clear();
+    filterValues[":filterValue0"] = 12345;
+    filterValues[":filterValue1"] = true;
+    filterValues[":filterValue2"] = "a string";
+    QTest::newRow("intersection filter") << intersectionFilter.properties() << filterValues << QString() << "( (testProperty=:filterValue0) AND (testProperty=:filterValue1) AND (testProperty=:filterValue2) )";
 
     History::UnionFilter unionFilter;
     filter.setFilterValue(12345);
@@ -765,17 +775,25 @@
     unionFilter.append(filter);
     filter.setFilterValue("a string");
     unionFilter.append(filter);
-    QTest::newRow("union filter") << unionFilter.properties() << QString() << "( (testProperty=12345) OR (testProperty=1) OR (testProperty='a string') )";
+    filterValues.clear();
+    filterValues[":filterValue0"] = 12345;
+    filterValues[":filterValue1"] = true;
+    filterValues[":filterValue2"] = "a string";
+    QTest::newRow("union filter") << unionFilter.properties() << filterValues << QString() << "( (testProperty=:filterValue0) OR (testProperty=:filterValue1) OR (testProperty=:filterValue2) )";
 }
 
 void SqlitePluginTest::testFilterToString()
 {
     QFETCH(QVariantMap, filterProperties);
+    QFETCH(QVariantMap, filterValues);
     QFETCH(QString, propertyPrefix);
     QFETCH(QString, resultString);
 
-    QString result = mPlugin->filterToString(History::Filter::fromProperties(filterProperties), propertyPrefix);
+    QVariantMap resultValues;
+    QString result = mPlugin->filterToString(History::Filter::fromProperties(filterProperties), resultValues, propertyPrefix);
     QCOMPARE(result, resultString);
+    QCOMPARE(resultValues, filterValues);
+
 }
 
 void SqlitePluginTest::testEscapeFilterValue_data()

=== modified file 'src/tests/EventViewTest.cpp'
--- src/tests/EventViewTest.cpp	2013-12-09 21:18:14 +0000
+++ src/tests/EventViewTest.cpp	2015-01-28 23:16:22 +0000
@@ -39,6 +39,7 @@
 private Q_SLOTS:
     void initTestCase();
     void testNextPage();
+    void testFilter_data();
     void testFilter();
     void testSort();
 
@@ -66,28 +67,66 @@
         events = view->nextPage();
     }
 
-    QCOMPARE(allEvents.count(), EVENT_COUNT * 2);
+    QCOMPARE(allEvents.count(), EVENT_COUNT * 2 + 1); // include the group text event
     Q_FOREACH(const History::Event &event, events) {
         QCOMPARE(event.type(), History::EventTypeText);
     }
 }
 
-void EventViewTest::testFilter()
+void EventViewTest::testFilter_data()
 {
+    QTest::addColumn<QVariantMap>("filterProperties");
+    QTest::addColumn<History::EventType>("eventType");
+    QTest::addColumn<int>("resultCount");
+    QTest::addColumn<QVariantMap>("firstEventProperties");
+
     History::IntersectionFilter filter;
     filter.append(History::Filter(History::FieldAccountId, "account0"));
     filter.append(History::Filter(History::FieldThreadId, "participant0"));
     filter.append(History::Filter(History::FieldEventId, "event21"));
-
-    History::EventViewPtr view = History::Manager::instance()->queryEvents(History::EventTypeVoice, History::Sort(History::FieldAccountId), filter);
+    History::VoiceEvent voiceEvent = History::Manager::instance()->getSingleEvent(History::EventTypeVoice,
+                                                                                  "account0",
+                                                                                  "participant0",
+                                                                                  "event21");
+    QVERIFY(!voiceEvent.isNull());
+    QTest::newRow("filter by accountId, threadId and eventId") << filter.properties() << History::EventTypeVoice << 1 << voiceEvent.properties();
+
+    filter.clear();
+    QStringList participants;
+    participants << "groupParticipant1" << "groupParticipant2";
+    History::Thread thread = History::Manager::instance()->threadForParticipants("groupAccount",
+                                                                                 History::EventTypeText,
+                                                                                 participants,
+                                                                                 History::MatchCaseSensitive);
+    QVERIFY(!thread.isNull());
+    History::TextEvent textEvent = History::Manager::instance()->getSingleEvent(History::EventTypeText,
+                                                                                thread.accountId(),
+                                                                                thread.threadId(),
+                                                                                "groupEvent0");
+    QVERIFY(!textEvent.isNull());
+    filter.append(History::Filter(History::FieldAccountId, thread.accountId()));
+    filter.append(History::Filter(History::FieldThreadId, thread.threadId()));
+    QTest::newRow("filter for a group conversation") << filter.properties() << History::EventTypeText << 1 << textEvent.properties();
+}
+
+void EventViewTest::testFilter()
+{
+    QFETCH(QVariantMap, filterProperties);
+    QFETCH(History::EventType, eventType);
+    QFETCH(int, resultCount);
+    QFETCH(QVariantMap, firstEventProperties);
+
+    History::Filter filter = History::Filter::fromProperties(filterProperties);
+    History::EventViewPtr view = History::Manager::instance()->queryEvents(eventType,
+                                                                           History::Sort(History::FieldAccountId),
+                                                                           filter);
     QVERIFY(view->isValid());
+
     History::Events events = view->nextPage();
-    QCOMPARE(events.count(), 1);
+    QCOMPARE(events.count(), resultCount);
+
     History::Event event = events.first();
-    QCOMPARE(event.accountId(), QString("account0"));
-    QCOMPARE(event.type(), History::EventTypeVoice);
-    QCOMPARE(event.threadId(), QString("participant0"));
-    QCOMPARE(event.eventId(), QString("event21"));
+    QCOMPARE(event.properties(), firstEventProperties);
 
     // make sure no more items are returned
     QVERIFY(view->nextPage().isEmpty());
@@ -106,7 +145,7 @@
     }
 
     QCOMPARE(allEvents.first().eventId(), QString("event00"));
-    QCOMPARE(allEvents.last().eventId(), QString("event%1").arg(EVENT_COUNT-1));
+    QCOMPARE(allEvents.last().eventId(), QString("groupEvent0"));
 
     History::Sort descendingSort(History::FieldEventId, Qt::DescendingOrder);
     allEvents.clear();
@@ -118,7 +157,7 @@
         events = view->nextPage();
     }
 
-    QCOMPARE(allEvents.first().eventId(), QString("event%1").arg(EVENT_COUNT-1));
+    QCOMPARE(allEvents.first().eventId(), QString("groupEvent0"));
     QCOMPARE(allEvents.last().eventId(), QString("event00"));
 }
 
@@ -166,6 +205,49 @@
         }
         QVERIFY(History::Manager::instance()->writeEvents(events));
     }
+
+    // create a text thread with multiple participants
+    QStringList participants;
+    participants << "groupParticipant1" << "groupParticipant2";
+
+    History::Thread groupTextThread = History::Manager::instance()->threadForParticipants("groupAccount",
+                                                                                          History::EventTypeText,
+                                                                                          participants,
+                                                                                          History::MatchCaseSensitive,
+                                                                                          true);
+    QVERIFY(!groupTextThread.isNull());
+
+    // and write a single event to it, just to make sure it works
+    History::TextEvent groupTextEvent("groupAccount",
+                                      groupTextThread.threadId(),
+                                      "groupEvent0",
+                                      "groupSender",
+                                      QDateTime::currentDateTime(),
+                                      true,
+                                      "A group message",
+                                      History::MessageTypeText);
+    QVERIFY(History::Manager::instance()->writeEvents(History::Events() << groupTextEvent));
+
+    // create a text thread with multiple participants
+    participants.clear();
+    participants << "groupParticipant1" << "groupParticipant2";
+
+    History::Thread groupVoiceThread = History::Manager::instance()->threadForParticipants("groupAccount",
+                                                                                           History::EventTypeVoice,
+                                                                                           participants,
+                                                                                           History::MatchCaseSensitive,
+                                                                                           true);
+    QVERIFY(!groupVoiceThread.isNull());
+
+    // and write a single event to it, just to make sure it works
+    History::VoiceEvent groupVoiceEvent("groupAccount",
+                                        groupVoiceThread.threadId(),
+                                        "groupEvent0",
+                                        "groupSender",
+                                        QDateTime::currentDateTime(),
+                                        true,
+                                        true);
+    QVERIFY(History::Manager::instance()->writeEvents(History::Events() << groupVoiceEvent));
 }
 
 QTEST_MAIN(EventViewTest)