Add generic class for handling methods and subscriptions for DDP APIand factor out authentication logic using this this mechanism.
ClosedPublic

Authored by alessandro on May 3 2020, 10:39 AM.

Details

Summary
  • Add generic class for handling methods and subscriptions for DDP API
  • Add utility function to convert strings to QJsonObject/QJsonArray
  • Factor out authentication logic in its own class (DDP API)
  • Add tests for DDPAuthenticationManager

Diff Detail

Repository
R865 Ruqola
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26490
Build 26508: arc lint + arc unit
alessandro requested review of this revision.May 3 2020, 10:39 AM
alessandro created this revision.
alessandro retitled this revision from Add generic class for handling methods and subscriptions for DDP API and factor out authentication logic using this this mechanism. to Add generic class for handling methods and subscriptions for DDP APIand factor out authentication logic using this this mechanism..May 3 2020, 10:40 AM
alessandro added reviewers: mlaurent, dfaure, kfunk.

Nice big refactoring ;)
What's the underlying motivation, out of curiosity? Just cleaner code, or this will allow to handle something better?

mlaurent requested changes to this revision.May 3 2020, 12:38 PM
mlaurent added inline comments.
src/core/ddpapi/ddpauthenticationmanager.cpp
77

"const" same for other code

259

qCWarnig(...)

src/core/ddpapi/ddpauthenticationmanager.h
34

not used

90

parent = nullptr (coding style)

113

initialize it

src/core/ddpapi/ddpmanager.h
42

parent = nullptr

This revision now requires changes to proceed.May 3 2020, 12:38 PM

Cleaner code, also it's easier to test interactions with the server if they are separated in logical blocks this way.
With this kind of granularity it should be easier to keep up with server updates and support older server versions too.
The overall idea would be to move the whole communication to DDP (websocket) instead of using some REST API and some websocket API.
This way it wouldn't be necessary to poll for updates (as it's done with the REST API), since the websocket is two ways and the client can get push notifications.
Also the server is smart enough to keep track of what the client knows already, so there's no need to do things like asking for the full room list and filtering the ones the room model knows about already when joining a new room.
I don't know for sure how far back the DDP API goes, but once this implementation is done it should be good enough for most recent servers.

alessandro updated this revision to Diff 81795.May 3 2020, 12:56 PM
  • Code style fixes

The overall idea would be to move the whole communication to DDP (websocket) instead of using some REST API and some websocket API.
This way it wouldn't be necessary to poll for updates (as it's done with the REST API), since the websocket is two ways and the client can get push notifications.
Also the server is smart enough to keep track of what the client knows already, so there's no need to do things like asking for the full room list and filtering the ones the room model knows about already when joining a new room.
I don't know for sure how far back the DDP API goes, but once this implementation is done it should be good enough for most recent servers.

For sure not we will not switch back to all DDP as we switched from DDP to rest api as RC dev created it as it's faster.
We will continue to switch to RESTAPI not switching back to DDP :)

mlaurent requested changes to this revision.May 3 2020, 3:34 PM
mlaurent added inline comments.
src/core/ddpapi/ddpauthenticationmanager.cpp
64

missing const QString params in each method

177

QLatin1String(QJsonDocument(mLastLoginPayload).toJson()) no ?

287

const

src/core/ddpapi/ddpclient.cpp
754

const

This revision now requires changes to proceed.May 3 2020, 3:34 PM
alessandro updated this revision to Diff 81806.May 3 2020, 3:49 PM
  • Add few more consts
alessandro updated this revision to Diff 81807.May 3 2020, 3:50 PM
  • Add few more consts

The overall idea would be to move the whole communication to DDP (websocket) instead of using some REST API and some websocket API.
This way it wouldn't be necessary to poll for updates (as it's done with the REST API), since the websocket is two ways and the client can get push notifications.
Also the server is smart enough to keep track of what the client knows already, so there's no need to do things like asking for the full room list and filtering the ones the room model knows about already when joining a new room.
I don't know for sure how far back the DDP API goes, but once this implementation is done it should be good enough for most recent servers.

For sure not we will not switch back to all DDP as we switched from DDP to rest api as RC dev created it as it's faster.
We will continue to switch to RESTAPI not switching back to DDP :)

I see. Could you expand a bit your point about performances? Is it a general REST vs websockets assessment, or does it have to do with ruqola's codebase specifically?

I see. Could you expand a bit your point about performances? Is it a general REST vs websockets assessment, or does it have to do with ruqola's codebase specifically?

No it's not ruqola specific it's the RC dev which optimized RC code by using RESTAPI versus DDP api.
They want that we use DDP only for update settings, and use RESTAPI for others methods.
They ported all Rocket.Chat client (web client and apps client) to restapi, and they contacted me 1.5 years to inform me that it was better to use RESTAPI.
It's for that I switched to RESTAPI before all ruqola used DDP directly.

mlaurent requested changes to this revision.May 4 2020, 4:51 AM
mlaurent added inline comments.
src/core/ddpapi/ddpauthenticationmanager.cpp
225

qCDebug here

249

} else if (...) { (coding style)

258

} else {

src/core/ddpapi/ddpclient.cpp
776

const auto

802

const auto

803

const here too ?

832

const QPAir

src/core/utils.cpp
295

const auto doc

314

const auto doc

This revision now requires changes to proceed.May 4 2020, 4:51 AM
alessandro updated this revision to Diff 82174.May 7 2020, 7:15 AM
  • Fix const and code style
alessandro updated this revision to Diff 82175.May 7 2020, 7:40 AM
  • Fix const and code style
mlaurent accepted this revision.May 7 2020, 12:08 PM

Thanks

This revision is now accepted and ready to land.May 7 2020, 12:08 PM
alessandro closed this revision.May 7 2020, 3:23 PM