Using gwamp to connect to crossbar #6195

DeepDiver1975 · 2017-11-28T10:31:36Z

ToDo

query capabilities for the wamp router endpoint
define proper session name - no idea what this should be
define a common realm which is to be used (involves server & router setup)
define topic names (involves server)
test with multiple accounts being connected to the same router - I had some issues ...
clarify authentication and topic access @DeepDiver1975

💟 I really love lambdas in C++ - nice to see the language evolving 💟

DeepDiver1975 · 2017-11-28T10:32:01Z

cmake/modules/QtVersionAbstraction.cmake

@@ -28,6 +28,8 @@ if(NOT TOKEN_AUTH_ONLY)
    endif()
 endif()

+find_package(Qt5WebSockets)


qwamp needs it - hmmm

maybe we patch qwamp to work without the qwebsocket dependency

DeepDiver1975 · 2017-11-28T10:32:07Z

cmake/modules/QtVersionAbstraction.cmake

@@ -47,6 +49,13 @@ if(APPLE AND NOT TOKEN_AUTH_ONLY)
    set (QT_LIBRARIES ${QT_LIBRARIES} ${Qt5MacExtras_LIBRARIES})
 endif()

+if(Qt5WebSockets_FOUND)


DeepDiver1975 · 2017-11-28T10:32:20Z

src/CMakeLists.txt

@@ -26,8 +26,11 @@ elseif(UNIX AND NOT APPLE)
  set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,relro -Wl,-z,now")
 endif()

+find_package(Qt5Qml REQUIRED)


DeepDiver1975 · 2017-11-28T10:32:34Z

src/gui/CMakeLists.txt

@@ -292,7 +301,7 @@ endif()

 add_library(updater STATIC ${updater_SRCS} ${updaterMoc})
 target_link_libraries(updater ${synclib_NAME})
-qt5_use_modules(updater Widgets Network Xml)
+qt5_use_modules(updater Widgets Network Xml Qml)


DeepDiver1975 · 2017-11-28T10:32:51Z

src/gui/CMakeLists.txt

@@ -157,6 +157,13 @@ set(3rdparty_SRC
    ../3rdparty/qtsingleapplication/qtlocalpeer.cpp
    ../3rdparty/qtsingleapplication/qtsingleapplication.cpp
    ../3rdparty/qtsingleapplication/qtsinglecoreapplication.cpp
+    ../3rdparty/qmsgpack/src/msgpack.cpp


I'm sure there is a nicer way ...

I guess it's fine.

The ideal way would be to have that in a separate .cmake file included in the qmsgpack repository, but since it's not the case, then so be it.

ogoffart · 2017-11-28T10:39:35Z

src/gui/CMakeLists.txt

@@ -157,6 +157,13 @@ set(3rdparty_SRC
    ../3rdparty/qtsingleapplication/qtlocalpeer.cpp
    ../3rdparty/qtsingleapplication/qtsingleapplication.cpp
    ../3rdparty/qtsingleapplication/qtsinglecoreapplication.cpp
+    ../3rdparty/qmsgpack/src/msgpack.cpp


I guess it's fine.

The ideal way would be to have that in a separate .cmake file included in the qmsgpack repository, but since it's not the case, then so be it.

ogoffart · 2017-11-28T10:55:26Z

src/gui/folder.cpp

+        session->start();
+      });
+
+    QObject::connect(_webSocket.data(), QOverload<QAbstractSocket::SocketError>::of(&QAbstractSocket::error), [&](QAbstractSocket::SocketError err){


QOverload requires Qt 5.7 (before that, we'd have to use static_cast)

(Actually, i've been thinking about copying QOverload within some header so we can use it while still being compatible with Qt 5.6)

well - what ever works - tell me whyt to do

ogoffart · 2017-11-28T10:57:41Z

CMakeLists.txt

@@ -218,6 +218,10 @@ add_definitions( -D_WIN32_WINNT=0x0600)
 add_definitions( -DWINVER=0x0600)
 endif( WIN32 )

+set(CMAKE_CXX_STANDARD 14)


Yay, \o/ finally i'd be able to use auto in lamda :-)

I guess we can do that because we stopped supporting these old debian and centOS that did not even have GCC 4.8
although this is not strictly required for your patch.

no - not required - but much more fun coding ;-)

FYI @jnweiger @dschmidt compilers

guruz · 2017-11-28T11:22:01Z

CI fails

[ 62%] Building CXX object test/CMakeFiles/FolderManTest.dir/__/src/gui/socketapi.cpp.o
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/gui/folder.cpp:37:19: fatal error: qwamp.h: No such file or directory
 #include "qwamp.h"
                   ^
compilation terminated.
test/CMakeFiles/FolderManTest.dir/build.make:100: recipe for target 'test/CMakeFiles/FolderManTest.dir/__/src/gui/folder.cpp.o' failed

@DeepDiver1975 you can build with tests to reproduce

guruz · 2017-11-28T11:24:05Z

.gitmodules

+	url = git@github.com:unitednetworks/qwamp.git
+[submodule "src/3rdparty/qmsgpack"]
+	path = src/3rdparty/qmsgpack
+	url = git@github.com:romixlab/qmsgpack.git


Heads-up to @hodyroff regarding licenses.

https://github.com/unitednetworks/qwamp
says Apache 2.0 correct? Thats perfect and I'll add it to the list.
https://github.com/romixlab/qmsgpack/blob/master/LICENSE
is MIT right? Also perfect ... I do need to add both, right?

guruz · 2017-11-28T11:24:23Z

CMakeLists.txt

@@ -218,6 +218,10 @@ add_definitions( -D_WIN32_WINNT=0x0600)
 add_definitions( -DWINVER=0x0600)
 endif( WIN32 )

+set(CMAKE_CXX_STANDARD 14)


FYI @jnweiger @dschmidt compilers

guruz · 2017-11-28T11:29:48Z

src/gui/folder.cpp

+          qDebug() << "Session joined to realm1 with session ID " << s;
+
+          session->subscribe("etag-changed-channel", [&](const QVariantList& args, const QVariantMap& options){
+              qDebug() << "Event received";


While in general I think the lambdas are cool, we should not become a nodejs with 3x nested callbacks :-)

guruz · 2017-11-28T11:36:14Z

src/gui/folder.h

@@ -353,6 +353,7 @@ private slots:
    bool _csyncUnavail;
    bool _proxyDirty;
    QPointer<RequestEtagJob> _requestEtagJob;
+    QScopedPointer<QTcpSocket> _webSocket;


QSslSocket?

Guide me - can QSslSocket also establish a non-ssl connection?

https://code.woboq.org/qt5/qtbase/src/network/ssl/qsslsocket.cpp.html#_ZN10QSslSocket22connectToHostEncryptedERK7QStringt6QFlagsIN9QIODevice12OpenModeFlagEEN15QAbstractSocket20NetworkLayerProtocolE

https://code.woboq.org/qt5/qtbase/src/network/ssl/qsslsocket.cpp.html#_ZN10QSslSocket13connectToHostERK7QStringt6QFlagsIN9QIODevice12OpenModeFlagEEN15QAbstractSocket20NetworkLayerProtocolE

You can use one of those depending on the account URL http vs https or how would this work? I wonder if the websocket URL should come from capabilities though so the server admin can scale it differently..

Things to keep in mind is the sslErrors signal. We have a AbstractSslErrorHandler that you can use.

The question is if websockets could return different ssl errors or not. Maybe as first implementation you assume they return the same error as the normal account WebDAV calls ..

The server would expose the websocket url via a capability. The Url will either be ws:// or wss://.

So depending on using ssl or not I either call connectToHost or connectToHostEncrypted ?

I'll postpone the ssl topic - websockets with self signed certificates are not working.
Hard to test at the moment.

guruz · 2017-11-28T11:51:55Z

https://github.com/romixlab/qmsgpack

By default these Qt types are supported: Int, UInt, LongLong, ULongLong, Double, QByteArray, Bool, QString, QStringList, QVariantList, QVariantMap.

Is QString serialized to the same kind of string as you will emit from the server side? So it's interoperable with non-Qt code using WAMP?

Crossbar.io supports all the favors of WAMP-over-WebSocket, including different serialization formats (JSON and MsgPack) as well as listening transports.

DeepDiver1975 · 2017-11-28T11:55:09Z

Is QString serialized to the same kind of string as you will emit from the server side? So it's interoperable with non-Qt code using WAMP?

In my non-representative local test the string as sent by the server popped up in the client the same way.
But it only contained ascii ...... need to have a second look.

DeepDiver1975 · 2017-11-29T14:11:12Z

the windows build hates me - qt version? compiler used?

DeepDiver1975 · 2017-11-30T19:18:42Z

the windows build hates me - qt version? compiler used?

location of error: https://github.com/DeepDiver1975/qwamp/blob/782e65859617e52fa0d92d56551fc05da62c1f8f/qwamp.h#L415

compiler error:

[ 69%] Building CXX object src/gui/CMakeFiles/owncloud.dir/folderstatusdelegate.cpp.obj
In file included from /usr/i686-w64-mingw32/sys-root/mingw/include/windows.h:71:0,
                 from /var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/common/utility.h:35,
                 from /var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/gui/../libsync/account.h:31,
                 from /var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/gui/folder.cpp:18:
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/3rdparty/qwamp/qwamp.h:415:9: error: expected identifier before numeric constant
         ERROR = 8,
         ^
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/3rdparty/qwamp/qwamp.h:415:9: error: expected '}' before numeric constant
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/3rdparty/qwamp/qwamp.h:415:9: error: expected unqualified-id before numeric constant
In file included from /var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/gui/folder.cpp:37:0:
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/3rdparty/qwamp/qwamp.h:434:2: warning: extra ';' [-Wpedantic]
 };
  ^
/var/lib/jenkins/workspace/cloud-client_client_PR-6195-BV7HAL2B65CGYGSIKPS4QYBZTIWUU3BEFWV3AREJE2WQUV77E2EA/src/3rdparty/qwamp/qwamp.h:450:1: error: expected declaration before '}' token
 }
 ^
cc1plus: warning: unrecognized command line option '-Wno-gnu-zero-variadic-macro-arguments'
src/gui/CMakeFiles/owncloud.dir/build.make:573: recipe for target 'src/gui/CMakeFiles/owncloud.dir/folder.cpp.obj' failed
make[2]: *** [src/gui/CMakeFiles/owncloud.dir/folder.cpp.obj] Error 1

ogoffart

There are lots of unrelated whitespace changes.

ogoffart · 2017-12-04T09:02:21Z

src/libsync/capabilities.cpp

+QString Capabilities::getWebSocketUrl() const
+{
+    if (!_capabilities.contains("websocket")) {
+        return "";


return QString()

(Otherwise you make a useless conversion from char*)

Also later.

ogoffart · 2017-12-04T09:04:30Z

src/gui/folder.cpp

+        _session.reset(new QWamp::Session(sessionName, *_webSocket.data(), QWamp::Session::MessageFormat::Msgpack, true));
+
+        QObject::connect(_session.data(), &QWamp::Session::joined, [&](qint64 s) {
+            qDebug() << "Session joined to realm1 with session ID " << s;


I know this is only temporary and you are going to clean up. In the final PR there should be no qDebug without categories.

ogoffart · 2017-12-04T09:06:59Z

src/gui/folder.cpp

+            // TODO: add current userid
+            _session->subscribe("files.root-etag-changed.admin", [&](const QVariantList& args, const QVariantMap& options) {
+                qDebug() << "Event received";
+                QString notificationEtag = args.at(0).toString();


QList::at crashes if the list don't have enough argument, which can happen, i guess, if the server is sending garbage.
So you can use value() which will return an empty variant if out of bounds. or you should check if there is enough element in the list before.

ogoffart · 2017-12-04T09:11:26Z

src/gui/folder.cpp

+    }
+    //initialize wamp connection
+    _webSocket.reset(new QTcpSocket());
+    QUrl webSocketUrl(wsUrl);


You also probably need to handle the disconnect case, and re try to connect. Also handle error such as timeout and other kind.
So you will need to move this connection code somewhere else so you can call it when you want to re-connect.

ogoffart · 2017-12-04T09:11:53Z

src/gui/folder.cpp

+    });
+
+    QObject::connect(_webSocket.data(), static_cast<void (QAbstractSocket::*)(QAbstractSocket::SocketError err)>(&QAbstractSocket::error), [&](QAbstractSocket::SocketError err) {
+        qInfo() << "tcp error: " << err;


This probably deserve to be a qCWarning. Don't forget to add a category. Also keep it in one line.

ogoffart · 2017-12-04T09:13:03Z

src/gui/folder.cpp

+        qInfo() << _webSocket->errorString();
+    });
+    qInfo() << "Connecting to websocket " << wsUrl;
+    _webSocket->abort();


ogoffart · 2017-12-04T09:13:46Z

src/gui/folder.cpp

+    if (webSocketUrl.scheme() == "ws") {
+        _webSocket->connectToHost(webSocketUrl.host(), webSocketUrl.port());
+    } else {
+        qCCritical(lcFolder) << "Invalid web socket scheme: " << webSocketUrl;


qCWarning... critical is a bit too high.

DeepDiver1975 self-assigned this Nov 28, 2017

DeepDiver1975 commented Nov 28, 2017

View reviewed changes

ogoffart reviewed Nov 28, 2017

View reviewed changes

guruz reviewed Nov 28, 2017

View reviewed changes

guruz added the DO NOT MERGE YET label Nov 28, 2017

DeepDiver1975 force-pushed the qwamp branch 2 times, most recently from 3896aa1 to 5ba6e6f Compare November 29, 2017 13:04

Using gwamp to connect to crossbar

b636143

DeepDiver1975 force-pushed the qwamp branch from 5ba6e6f to b636143 Compare November 29, 2017 13:29

Take websocket url from the server capabilities

a8e3a74

DeepDiver1975 added 2 commits December 1, 2017 13:10

Remove QT4 flag

1c03d4c

trigger sync on event

5c727e5

ogoffart reviewed Dec 4, 2017

View reviewed changes

SamuAlfageme mentioned this pull request Jan 9, 2018

[Feature Request] Propagate mount write/update permissions to the client owncloud/core#30070

Closed

SamuAlfageme mentioned this pull request Jun 8, 2018

Increase chunked upload finalization timeout #6527

Closed

DeepDiver1975 closed this Oct 31, 2018

TheOneRing deleted the qwamp branch December 2, 2019 16:43

Using gwamp to connect to crossbar #6195

Using gwamp to connect to crossbar #6195

Conversation

DeepDiver1975 commented Nov 28, 2017 • edited Loading

ToDo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guruz commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guruz commented Nov 28, 2017

DeepDiver1975 commented Nov 28, 2017

DeepDiver1975 commented Nov 29, 2017

DeepDiver1975 commented Nov 30, 2017

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 28, 2017 •

edited

Loading