concatUrl: Remove manual parsing, add test. #2817

src/gui/sharedialog.cpp

@@ -257,7 +257,10 @@ void ShareDialog::slotSharesFetched(const QString &reply)
             if (versionString.contains('.') && versionString.split('.')[0].toInt() >= 8) {
                 url = Account::concatUrlPath(_account->url(), QString("index.php/s/%1").arg(data.value("token").toString())).toString();
             } else {
-                url = Account::concatUrlPath(_account->url(), QString("public.php?service=files&t=%1").arg(data.value("token").toString())).toString();
+                QList<QPair<QString, QString>> queryArgs;
+                queryArgs.append(qMakePair(QString("service"), QString("files")));
+                queryArgs.append(qMakePair(QString("t"), data.value("token").toString()));
+                url = Account::concatUrlPath(_account->url(), QLatin1String("public.php"), queryArgs).toString();
             }
             _ui->lineEdit_shareLink->setText(url);
         }

src/libsync/account.cpp

@@ -380,43 +380,22 @@ void Account::setUrl(const QUrl &url)
     _url = url;
 }
 
-QUrl Account::concatUrlPath(const QUrl &url, const QString &concatPath)
+QUrl Account::concatUrlPath(const QUrl &url, const QString &concatPath,
+                            const QList< QPair<QString, QString> > &queryItems)
 {
-    QUrl tmpUrl = url;
-    QString path = tmpUrl.path();
-    // avoid '//'
-    if (path.endsWith('/') && concatPath.startsWith('/')) {
-        path.chop(1);
-    } // avoid missing '/'
-    else if (!path.endsWith('/') && !concatPath.startsWith('/')) {
-        path += QLatin1Char('/');
-    }
-    path += concatPath; // put togehter the complete path
-
-    QList< QPair<QString, QString> > queryItems;
-    // Check if there are query items within the path and if so, add them properly to the url.
-    if( path.contains(QLatin1Char('?')) ) {
-        // get the position of the delimiter between path and query items.
-        // remember: myphpscript.php?queryItem1=foo&queryItem2=bar
-        int cutPos = path.indexOf(QLatin1Char('?'));
-        if( cutPos > -1 ) {
-            QString itemStr = path.right(path.length() - cutPos-1);
-
-            // remote the query item part from the path.
-            if( cutPos < path.length() ) {
-                path.truncate(cutPos);
-            }
-
-            // parse the query items into the list of QPairs of Strings.
-            QStringList items = itemStr.split(QLatin1Char('&'));
-            Q_FOREACH( QString item, items ) {
-                QStringList pair = item.split(QLatin1Char('='));
-                if( pair.size() > 1 ) {
-                    queryItems.append( QPair<QString, QString>(pair.at(0), pair.at(1)) );
-                }
-            }
+    QString path = url.path();
+    if (! concatPath.isEmpty()) {
+        // avoid '//'
+        if (path.endsWith('/') && concatPath.startsWith('/')) {
+            path.chop(1);
+        } // avoid missing '/'
+        else if (!path.endsWith('/') && !concatPath.startsWith('/')) {
+            path += QLatin1Char('/');
         }
+        path += concatPath; // put the complete path together
     }
+
+    QUrl tmpUrl = url;
     tmpUrl.setPath(path);
     if( queryItems.size() > 0 ) {
         tmpUrl.setQueryItems(queryItems);

src/libsync/account.h

@@ -136,7 +136,8 @@ public:
     void setSslErrorHandler(AbstractSslErrorHandler *handler);
 
     // static helper function
-    static QUrl concatUrlPath(const QUrl &url, const QString &concatPath);
+    static QUrl concatUrlPath(const QUrl &url, const QString &concatPath,
+                              const QList< QPair<QString, QString> > &queryItems = QList< QPair<QString, QString> >());
 
     /**  Returns a new settings pre-set in a specific group.  The Settings will be created
          with the given parent. If no parents is specified, the caller must destroy the settings */

test/CMakeLists.txt

@@ -30,6 +30,7 @@ owncloud_add_test(NetrcParser ../src/cmd/netrcparser.cpp)
 owncloud_add_test(OwnSql "")
 owncloud_add_test(SyncJournalDB "")
 owncloud_add_test(SyncFileItem "")
+owncloud_add_test(ConcatUrl "")
 
 
 

test/testconcaturl.h

@@ -0,0 +1,104 @@
+/*
+ *    This software is in the public domain, furnished "as is", without technical
+ *    support, and with no warranty, express or implied, as to its usefulness for
+ *    any purpose.
+ *
+ */
+
+#ifndef MIRALL_TESTCONCATURL_H
+#define MIRALL_TESTCONCATURL_H
+
+#include <QtTest>
+
+#include <QUrl>
+#include <QString>
+
+#include "account.h"
+
+using namespace OCC;
+
+typedef QList< QPair<QString,QString> > QueryItems;
+
+static QueryItems make()
+{
+    return QueryItems();
+}
+
+static QueryItems make(QString key, QString value)
+{
+    QueryItems q;
+    q.append(qMakePair(key, value));
+    return q;
+}
+
+static QueryItems make(QString key1, QString value1,
+                       QString key2, QString value2)
+{
+    QueryItems q;
+    q.append(qMakePair(key1, value1));
+    q.append(qMakePair(key2, value2));
+    return q;
+}
+
+class TestConcatUrl: public QObject
+{
+    Q_OBJECT
+private slots:
+    void testFolder()
+    {
+        QFETCH(QString, base);
+        QFETCH(QString, concat);
+        QFETCH(QueryItems, query);
+        QFETCH(QString, expected);
+        QUrl baseUrl("http://example.com" + base);
+        QUrl resultUrl = Account::concatUrlPath(baseUrl, concat, query);
+        QString result = QString::fromUtf8(resultUrl.toEncoded());
+        QString expectedFull = "http://example.com" + expected;
+        QCOMPARE(result, expectedFull);
+    }
+
+    void testFolder_data()
+    {
+        QTest::addColumn<QString>("base");
+        QTest::addColumn<QString>("concat");
+        QTest::addColumn<QueryItems>("query");
+        QTest::addColumn<QString>("expected");
+
+        // Tests about slashes
+        QTest::newRow("noslash1")  << "/baa"  << "foo"  << make() << "/baa/foo";
+        QTest::newRow("noslash2")  << ""      << "foo"  << make() << "/foo";
+        QTest::newRow("noslash3")  << "/foo"  << ""     << make() << "/foo";
+        QTest::newRow("noslash4")  << ""      << ""     << make() << "";
+        QTest::newRow("oneslash1") << "/bar/" << "foo"  << make() << "/bar/foo";
+        QTest::newRow("oneslash2") << "/"     << "foo"  << make() << "/foo";
+        QTest::newRow("oneslash3") << "/foo"  << "/"    << make() << "/foo/";
+        QTest::newRow("oneslash4") << ""      << "/"    << make() << "/";
+        QTest::newRow("twoslash1") << "/bar/" << "/foo" << make() << "/bar/foo";
+        QTest::newRow("twoslash2") << "/"     << "/foo" << make() << "/foo";
+        QTest::newRow("twoslash3") << "/foo/" << "/"    << make() << "/foo/";
+        QTest::newRow("twoslash4") << "/"     << "/"    << make() << "/";
+
+        // Tests about path encoding
+        QTest::newRow("encodepath")
+                << "/a f/b"
+                << "/a f/c"
+                << make()
+                << "/a%20f/b/a%20f/c";
+
+        // Tests about query args
+        QTest::newRow("query1")
+                << "/baa"
+                << "/foo"
+                << make("a=a", "b=b",
+                        "c", "d")
+                << "/baa/foo?a%3Da=b%3Db&c=d";
+        QTest::newRow("query2")
+                << ""
+                << ""
+                << make("foo", "bar")
+                << "?foo=bar";
+    }
+
+};
+
+#endif