Allow to directly query objects through object's id.

The change inserts, if given, an object id to request path. It allows
to construct query objects with only type name and id. Such query is
equivalent to a "view" request, that means that instead of returning
a array of results it gives full object only as a result.

From user perspective this change simplify certain group of queries,
for example this query:

QJsonObject query = {{"objectType", "objects.foo"},
                     {"query", {{"id", "1233"}}}};

could be changed to:

QJsonObject query = {{"objectType", "objects.foo"},
                     {"id", "1233"}};

The change cleanups a bit path computation, by not adding an object id
as a fall-back operation. Because of that hint IncludeIdPath could be
replaced by RequireIdPath, which is easier to interpret. It shows that
an operation has no chances to success if an id is missing.

Change-Id: If2bf09da0d7c4388493476a16b138640022c8581
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@digia.com>

3 files changed, 101 insertions, 71 deletions

diff --git a/src/enginio_client/enginioclient.cpp b/src/enginio_client/enginioclient.cpp
index 6a47696..3d1b888 100644
--- a/src/enginio_client/enginioclient.cpp
+++ b/src/enginio_client/enginioclient.cpp
@@ -262,6 +262,21 @@ QNetworkRequest EnginioClientConnectionPrivate::prepareRequest(const QUrl &url)
     return req;
 }
 
+bool EnginioClientConnectionPrivate::appendIdToPathIfPossible(QString *path, const QString &id, QByteArray *errorMsg, EnginioClientConnectionPrivate::PathOptions flags, QByteArray errorMessageHint)
+{
+    Q_ASSERT(path && errorMsg);
+    if (id.isEmpty()) {
+        if (flags == RequireIdInPath) {
+            *errorMsg = constructErrorMessage(errorMessageHint);
+            return false;
+        }
+        return true;
+    }
+    path->append('/');
+    path->append(id);
+    return true;
+}
+
 EnginioClientConnectionPrivate::EnginioClientConnectionPrivate() :
     _identity(),
     _serviceUrl(EnginioString::apiEnginIo),
diff --git a/src/enginio_client/enginioclient_p.h b/src/enginio_client/enginioclient_p.h
index 93ce864..286a30c 100644
--- a/src/enginio_client/enginioclient_p.h
+++ b/src/enginio_client/enginioclient_p.h
@@ -87,11 +87,11 @@ QT_BEGIN_NAMESPACE
     CHECK_AND_SET_URL_PATH_IMPL(Url, Object, Operation, EnginioClientConnectionPrivate::Default)
 
 #define CHECK_AND_SET_PATH_WITH_ID(Url, Object, Operation) \
-    CHECK_AND_SET_URL_PATH_IMPL(Url, Object, Operation, EnginioClientConnectionPrivate::IncludeIdInPath)
+    CHECK_AND_SET_URL_PATH_IMPL(Url, Object, Operation, EnginioClientConnectionPrivate::RequireIdInPath)
 
 class ENGINIOCLIENT_EXPORT EnginioClientConnectionPrivate : public QObjectPrivate
 {
-    enum PathOptions { Default, IncludeIdInPath = 1};
+    enum PathOptions { Default, RequireIdInPath = 1};
 
     struct ENGINIOCLIENT_EXPORT GetPathReturnValue : public QPair<bool, QString>
     {
@@ -105,6 +105,8 @@ class ENGINIOCLIENT_EXPORT EnginioClientConnectionPrivate : public QObjectPrivat
         operator QString() const { return second; }
     };
 
+    static bool appendIdToPathIfPossible(QString *path, const QString &id, QByteArray *errorMsg, PathOptions flags, QByteArray errorMessageHint = EnginioString::Requested_operation_requires_non_empty_id_value);
+
     template<class T>
     static GetPathReturnValue getPath(const T &object, int operation, QString *path, QByteArray *errorMsg, PathOptions flags = Default)
     {
@@ -114,6 +116,7 @@ class ENGINIOCLIENT_EXPORT EnginioClientConnectionPrivate : public QObjectPrivat
         QString &result = *path;
         result.reserve(96);
         result.append(QStringLiteral("/v1/"));
+        QString id = object[EnginioString::id].toString();
 
         switch (operation) {
         case Enginio::ObjectOperation: {
@@ -122,8 +125,9 @@ class ENGINIOCLIENT_EXPORT EnginioClientConnectionPrivate : public QObjectPrivat
                 msg = constructErrorMessage(EnginioString::Requested_object_operation_requires_non_empty_objectType_value);
                 return GetPathReturnValue(Failed);
             }
-
             result.append(objectType.replace('.', '/'));
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         }
         case Enginio::AccessControlOperation:
@@ -133,83 +137,65 @@ class ENGINIOCLIENT_EXPORT EnginioClientConnectionPrivate : public QObjectPrivat
                 msg = constructErrorMessage(EnginioString::Requested_object_acl_operation_requires_non_empty_objectType_value);
                 return GetPathReturnValue(Failed);
             }
-
             result.append(objectType.replace('.', '/'));
-            QString id = object[EnginioString::id].toString();
-            if (id.isEmpty()) {
-                msg = constructErrorMessage(EnginioString::Requested_object_acl_operation_requires_non_empty_id_value);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, RequireIdInPath, EnginioString::Requested_object_acl_operation_requires_non_empty_id_value))
                 return GetPathReturnValue(Failed);
-            }
-            result.append('/');
-            result.append(id);
             result.append('/');
             result.append(EnginioString::access);
             return GetPathReturnValue(true, EnginioString::access);
         }
         case Enginio::FileOperation: {
             result.append(EnginioString::files);
-            // if we have a fileID, it becomes "view", otherwise it is up/download
-            QString fileId = object[EnginioString::id].toString();
-            if (!fileId.isEmpty()) {
-                result.append('/');
-                result.append(fileId);
-            }
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         }
         case Enginio::FileGetDownloadUrlOperation: {
             result.append(EnginioString::files);
-            QString fileId = object[EnginioString::id].toString();
-            if (fileId.isEmpty()) {
-                msg = constructErrorMessage(EnginioString::Download_operation_requires_non_empty_fileId_value);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, RequireIdInPath, EnginioString::Download_operation_requires_non_empty_fileId_value))
                 return GetPathReturnValue(Failed);
-            }
-            result.append(QLatin1Char('/') + fileId + QStringLiteral("/download_url"));
+            result.append(QStringLiteral("/download_url"));
             break;
         }
         case Enginio::FileChunkUploadOperation: {
-            const QString fileId = object[EnginioString::id].toString();
-            Q_ASSERT(!fileId.isEmpty());
-            result.append(EnginioString::files + QLatin1Char('/') + fileId + QStringLiteral("/chunk"));
+            Q_ASSERT(!id.isEmpty());
+            result.append(EnginioString::files);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
+            result.append(QStringLiteral("/chunk"));
             break;
         }
         case Enginio::SearchOperation:
             result.append(EnginioString::search);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         case Enginio::SessionOperation:
             result.append(EnginioString::session);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         case Enginio::UserOperation:
             result.append(EnginioString::users);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         case Enginio::UsergroupOperation:
             result.append(EnginioString::usergroups);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, flags))
+                return GetPathReturnValue(Failed);
             break;
         case Enginio::UsergroupMembersOperation:
         {
-            QString id = object[EnginioString::id].toString();
-            if (id.isEmpty()) {
-                msg = constructErrorMessage(EnginioString::Requested_usergroup_member_operation_requires_non_empty_id_value);
-                return GetPathReturnValue(Failed);
-            }
             result.append(EnginioString::usergroups);
-            result.append('/');
-            result.append(id);
+            if (!appendIdToPathIfPossible(path, id, errorMsg, RequireIdInPath, EnginioString::Requested_usergroup_member_operation_requires_non_empty_id_value))
+                return GetPathReturnValue(Failed);
             result.append('/');
             result.append(EnginioString::members);
             return GetPathReturnValue(true, EnginioString::member);
         }
         }
 
-        if (flags & IncludeIdInPath) {
-            QString id = object[EnginioString::id].toString();
-            if (id.isEmpty()) {
-                msg = constructErrorMessage(EnginioString::Requested_operation_requires_non_empty_id_value);
-                return GetPathReturnValue(Failed);
-            }
-            result.append('/');
-            result.append(id);
-        }
-
         return GetPathReturnValue(true, QString());
     }
 
@@ -557,7 +543,6 @@ public:
         url.setQuery(urlQuery);
 
         QNetworkRequest req = prepareRequest(url);
-
         return networkManager()->get(req);
     }
 
@@ -565,7 +550,7 @@ public:
     QNetworkReply *downloadUrl(const ObjectAdaptor<T> &object)
     {
         QUrl url(_serviceUrl);
-        CHECK_AND_SET_PATH(url, object, Enginio::FileGetDownloadUrlOperation);
+        CHECK_AND_SET_PATH_WITH_ID(url, object, Enginio::FileGetDownloadUrlOperation);
         if (object.contains(EnginioString::variant)) {
             QString variant = object[EnginioString::variant].toString();
             QUrlQuery query;
diff --git a/tests/auto/enginioclient/tst_enginioclient.cpp b/tests/auto/enginioclient/tst_enginioclient.cpp
index aafebbe..d1f25cc 100644
--- a/tests/auto/enginioclient/tst_enginioclient.cpp
+++ b/tests/auto/enginioclient/tst_enginioclient.cpp
@@ -140,6 +140,49 @@ void tst_EnginioClient::initTestCase()
 
     prepareForSearch();
     EnginioTests::prepareTestUsersAndUserGroups(_backendId);
+
+    // create some todos objects
+    QJsonObject todos;
+    todos["name"] = QStringLiteral("todos");
+
+    QJsonObject title;
+    title["name"] = QStringLiteral("title");
+    title["type"] = QStringLiteral("string");
+    title["indexed"] = true;
+    QJsonObject completed;
+    completed["name"] = QStringLiteral("completed");
+    completed["type"] = QStringLiteral("boolean");
+    completed["indexed"] = false;
+    QJsonObject count;
+    count["name"] = QStringLiteral("count");
+    count["type"] = QStringLiteral("number");
+    count["indexed"] = false;
+
+    QJsonArray properties;
+    properties.append(title);
+    properties.append(completed);
+    properties.append(count);
+    todos["properties"] = properties;
+
+    QVERIFY(_backendManager.createObjectType(_backendName, EnginioTests::TESTAPP_ENV, todos));
+    {
+        EnginioClient client;
+        QObject::connect(&client, SIGNAL(error(EnginioReply *)), this, SLOT(error(EnginioReply *)));
+        client.setBackendId(_backendId);
+        client.setServiceUrl(EnginioTests::TESTAPP_URL);
+
+        QJsonObject object;
+        object["objectType"] = QString::fromUtf8("objects.todos");
+        object["title"] = QString::fromUtf8("init todo 1");
+        object["completed"] = false;
+        EnginioReply* reply1 = client.create(object);
+        object["title"] = QString::fromUtf8("init todo 2");
+        EnginioReply* reply2 = client.create(object);
+        QTRY_VERIFY(reply1->isFinished());
+        CHECK_NO_ERROR(reply1);
+        QTRY_VERIFY(reply2->isFinished());
+        CHECK_NO_ERROR(reply2);
+    }
 }
 
 void tst_EnginioClient::cleanupTestCase()
@@ -177,20 +220,31 @@ void tst_EnginioClient::query_todos()
     QSignalSpy spy(&client, SIGNAL(finished(EnginioReply *)));
     QSignalSpy spyError(&client, SIGNAL(error(EnginioReply*)));
     //![query-todo]
-    QJsonObject object;
-    object["objectType"] = QString::fromUtf8("objects.todos");
-    const EnginioReply* reply = client.query(object);
+    QJsonObject query;
+    query["objectType"] = QString::fromUtf8("objects.todos");
+    EnginioReply *reply = client.query(query);
     //![query-todo]
     QVERIFY(reply);
 
     QTRY_COMPARE(spy.count(), 1);
     QCOMPARE(spyError.count(), 0);
 
-    const EnginioReply *response = spy[0][0].value<EnginioReply*>();
+    EnginioReply *response = spy[0][0].value<EnginioReply*>();
     QCOMPARE(response, reply);
     CHECK_NO_ERROR(response);
     QVERIFY(!response->data().isEmpty());
     QVERIFY(!response->data()["results"].isUndefined());
+
+    {   // try to query the object with an id
+        QJsonArray results = reply->data()["results"].toArray();
+        QVERIFY(results.count() > 1); // the test assumes that querying by id will return less items.
+        QString id = results[0].toObject()["id"].toString();
+        query["id"] = id;
+        reply = client.query(query);
+        QTRY_VERIFY(reply->isFinished());
+        CHECK_NO_ERROR(reply);
+        QCOMPARE(reply->data()["id"].toString(), id);
+    }
 }
 
 void tst_EnginioClient::query_todos_filter()
@@ -882,30 +936,6 @@ void tst_EnginioClient::deleteReply()
 
 void tst_EnginioClient::create_todos()
 {
-    QJsonObject todos;
-    todos["name"] = QStringLiteral("todos");
-
-    QJsonObject title;
-    title["name"] = QStringLiteral("title");
-    title["type"] = QStringLiteral("string");
-    title["indexed"] = true;
-    QJsonObject completed;
-    completed["name"] = QStringLiteral("completed");
-    completed["type"] = QStringLiteral("boolean");
-    completed["indexed"] = false;
-    QJsonObject count;
-    count["name"] = QStringLiteral("count");
-    count["type"] = QStringLiteral("number");
-    count["indexed"] = false;
-
-    QJsonArray properties;
-    properties.append(title);
-    properties.append(completed);
-    properties.append(count);
-    todos["properties"] = properties;
-
-    QVERIFY(_backendManager.createObjectType(_backendName, EnginioTests::TESTAPP_ENV, todos));
-
     EnginioClient client;
     QObject::connect(&client, SIGNAL(error(EnginioReply *)), this, SLOT(error(EnginioReply *)));
     client.setBackendId(_backendId);