Add MIXRegistry, Limber MIX Mock Server and MIX Example #88

tarun018 · 2017-08-16T22:51:22Z

Standard output shows:

Connecting...Connected
Request list of channels.
List of rooms at example.tg
1. coven - [email protected]

Request supported nodes for MIX channel: [email protected]
Nodes supported by channel [email protected]
1. urn:xmpp:mix:nodes:participants
2. urn:xmpp:mix:nodes:messages
3. urn:xmpp:mix:nodes:presence
4. urn:xmpp:mix:nodes:jidmap

Successfully joined channel [email protected]
[warning] Swiften/MIX/MIXRegistry.cpp:24 joinChannel: Channel already joined: [email protected]
Successfully left channel [email protected]

Warning is intentional to notify about asking to join an already joined channel.

Update roster encoding for MIX channel Update roster parser and serializer License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details. Test-Information: Added tests for joining and leaving channels via MIXRegistry, which passes. Tested on Ubuntu 16.04 LTS. Change-Id: Idb9683f45734ac58daa58736fe199dc014b9352a

License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details. Test-Information: Tests added for joining and leaving MIX channels as in XEP-0369, which passes. Tested on Ubuntu 16.04 LTS. Change-Id: Ib85d6e866e09db060b9b2da428f074dfbfd26385

intosi · 2017-08-17T06:37:28Z

Limber/MIXMockServer.cpp

+ channels_.insert(channelJID);
+ }
+
+ std::unordered_set<std::string>& getChannels() {


Shouldn't this be a const reference?

intosi · 2017-08-17T06:41:02Z

Limber/MIXMockServer.cpp

+
+ //If request comes to domain service
+ if (stanza->getTo().isValid() && stanza->getTo() == session->getLocalJID()) {
+ if (std::shared_ptr<IQ> iq = std::dynamic_pointer_cast<IQ>(stanza)) {


Are you masking the existing iq variable with the same contents deliberately here?

intosi · 2017-08-17T06:42:40Z

Limber/MIXMockServer.cpp

+ if(iq->getPayload<RosterPayload>()) {
+ SWIFT_LOG(debug) << "Query: Roster" << std::endl;
+ session->sendElement(IQ::createResult(iq->getFrom(), iq->getID(), std::make_shared<RosterPayload>()));
+ }


Continuing processing the rest of handleElementReceived after this sendElement (and the ones below) isn't really useful, is it?

intosi · 2017-08-17T06:46:24Z

Limber/MIXMockServer.cpp

+ SimpleEventLoop eventLoop;
+ SimpleUserRegistry userRegistry;
+
+ userRegistry.addUser(JID("[email protected]"), "mixtest");


I think I would prefer it if you used example.com, which is guaranteed to never mask existing domains, instead of assuming there will never be a .tg TLD.

intosi · 2017-08-17T06:50:27Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+ assert(mixRegistry_);
+ assert(mixRegistry_->getMIXInstance(jid));
+
+ cout << "Successfully joined channel " << jid << std::endl;


Mixing prefixed and non-prefixed iostream bits looks odd. I think I'd prefer the prefixed ones here.

intosi · 2017-08-17T06:59:37Z

Swiften/Roster/XMPPRosterImpl.cpp

@@ -40,6 +40,15 @@ void XMPPRosterImpl::clear() {
 onRosterCleared();
 }

+bool XMPPRosterImpl::isMIXChannel(const JID& jid) {
+ std::map<JID, XMPPRosterItem>::const_iterator i = entries_.find(jid.toBare());


Assuming you really need the explicit const iterator here, I would prefer RosterMap::const_iterator instead.

tfar · 2017-08-17T09:03:30Z

Limber/MIXMockServer.cpp

+ if (iq) {
+ if(iq->getPayload<RosterPayload>()) {
+ SWIFT_LOG(debug) << "Query: Roster" << std::endl;
+ session->sendElement(IQ::createResult(iq->getFrom(), iq->getID(), std::make_shared<RosterPayload>()));


This isn't the final version right? I would expect it to return the MIX channels which the user has joined.

tfar · 2017-08-17T09:15:46Z

Swiften/Client/Client.h

@@ -191,6 +196,7 @@ namespace Swift {
 PresenceOracle* presenceOracle;
 DirectedPresenceSender* directedPresenceSender;
 StanzaChannelPresenceSender* stanzaChannelPresenceSender;
+ MIXRegistry* mixRegistry;


This could be a unique_ptr, right?

tarun018 · 2017-08-17T19:33:56Z

Updated based on feedback and added roster sync between clients:

Example (Client 1) :

./Swiften/Examples/MIXListAndJoin/MIXListAndJoin [email protected]/some mixtest2 example.com
Connecting...Connected
Request list of channels.
List of rooms at example.com
	1. coven - [email protected]

Request supported nodes for MIX channel: [email protected]
Nodes supported by channel [email protected]
	1. urn:xmpp:mix:nodes:participants
	2. urn:xmpp:mix:nodes:messages
	3. urn:xmpp:mix:nodes:presence
	4. urn:xmpp:mix:nodes:jidmap

Already Joined channels: 
No channels already joined.

Successfully joined channel [email protected]
[warning] Swiften/MIX/MIXRegistry.cpp:40 joinChannel: Channel already joined: [email protected]

However (client 2 of the same user):

./Swiften/Examples/MIXListAndJoin/MIXListAndJoin [email protected]/someelse mixtest2 example.com
Connecting...Connected
Request list of channels.
List of rooms at example.com
	1. coven - [email protected]

Request supported nodes for MIX channel: [email protected]
Nodes supported by channel [email protected]
	1. urn:xmpp:mix:nodes:participants
	2. urn:xmpp:mix:nodes:messages
	3. urn:xmpp:mix:nodes:presence
	4. urn:xmpp:mix:nodes:jidmap

Already Joined channels: 
	1. coven - [email protected]

[warning] Swiften/MIX/MIXRegistry.cpp:40 joinChannel: Channel already joined: [email protected]

tfar

Some feedback inline.

tfar · 2017-08-21T08:53:45Z

Limber/MIXMockServer.cpp

+ * All rights reserved.
+ * See the COPYING file for more information.
+ */
+/*


Please leave a newline between copyright blocks.

Will udpate.

tfar · 2017-08-21T10:37:33Z

Limber/MIXMockServer.cpp

+ //If request comes to own local server
+ if (stanza->getTo().isValid() && stanza->getTo() == session->getRemoteJID().toBare()) {
+ if (auto incomingJoinPayload = iq->getPayload<MIXJoin>()) {
+ if (boost::optional<JID> channelJID = incomingJoinPayload->getChannel()) {


Why use auto in the line above, but not here?

tfar · 2017-08-21T10:41:20Z

Limber/MIXMockServer.cpp

+ }
+
+ if (auto incomingLeavePayload = iq->getPayload<MIXLeave>()) {
+ if (boost::optional<JID> channelJID = incomingLeavePayload->getChannel()) {


Why use auto in the line above, but not here?

tfar · 2017-08-21T10:53:33Z

Limber/MIXMockServer.cpp

+ return;
+ }
+ } else {
+ session->sendElement(IQ::createError(iq->getFrom(), iq->getID(), ErrorPayload::BadRequest, ErrorPayload::Cancel));


If it's not an IQ you can't return an IQ error. You should probably just ignore it in this case.

tfar · 2017-08-21T11:07:47Z

Limber/MIXMockServer.cpp

+ stanza->setFrom(session->getRemoteJID());
+
+ auto iq = std::dynamic_pointer_cast<IQ>(stanza);
+ if (iq) {


It probably makes more sense to test for !iq and handle the error case there and afterwards check if it's a roster payload.

Yeah, makes more sense.

tfar · 2017-08-21T11:50:00Z

Limber/MIXMockServer.cpp

+ }
+
+ //If request comes to domain service
+ if (stanza->getTo().isValid() && stanza->getTo() == session->getLocalJID()) {


Is the isValid() test required here?

tfar · 2017-08-21T11:52:16Z

Limber/MIXMockServer.cpp

+ }
+ return;
+ }
+ }


This method is quite long. It probably makes sense to split it up in parts. Like handleElementReceivedForAccount, handleElementReceivedForMIXService, handleElementReceivedForMIXChannel, etc.

I've split the function into the three cases now.

tfar · 2017-08-21T11:54:07Z

Swiften/Elements/RosterPayload.h

@@ -42,7 +42,16 @@ namespace Swift {
 version_ = version;
 }

+ bool hasMIXAnnotationSupport() const {


This method name doesn't make it clear what it is for. You probably want to enable requesting MIX annotations for roster requests. How about hasRequestMIXAnnotations()? Similar for the following method.

tarun018 · 2017-08-21T12:54:59Z

Updated based on feedback.

tfar · 2017-08-21T13:30:39Z

Limber/MIXMockServer.cpp

+
+ auto iq = std::dynamic_pointer_cast<IQ>(stanza);
+ if (!iq) {
+ session->sendElement(IQ::createError(iq->getFrom(), iq->getID(), ErrorPayload::BadRequest, ErrorPayload::Cancel));


If it's not an IQ, you cannot respond with an IQ error!!!! IQ errors are when you receive an IQ but do not understand the content and cannot sensibly handle it.

I am now returning if element is not an iq.

tfar · 2017-08-21T13:33:06Z

Limber/MIXMockServer.cpp

+ }
+
+ //If request comes to domain service
+ if (stanza->getTo() == session->getLocalJID()) {


These should probably be else if statements, or does it make sense to have an IQ handled by multiple handleELementReceivedForX methods?

tfar · 2017-08-21T13:36:08Z

Limber/MIXMockServer.cpp

+
+ void handleElementReceivedForAccount(IQ::ref iq, std::shared_ptr<ServerFromClientSession> session) {
+ if (auto incomingJoinPayload = iq->getPayload<MIXJoin>()) {
+ if (auto channelJID = incomingJoinPayload->getChannel()) {


Creating two helper methods to handle join and leave would help to further improve readability of the code. You could then simply call them in here.

Added handleJoin and handleLeave methods.

tfar · 2017-08-21T13:37:49Z

Limber/MIXMockServer.cpp

+ session->sendElement(IQ::createError(iq->getFrom(), iq->getID(), ErrorPayload::BadRequest, ErrorPayload::Cancel));
+ }
+ }
+ return;


Could you remove return statements at the end of void methods. Thanks.

tfar

This PR is slowly getting there. 👍

tfar · 2017-08-21T16:25:30Z

Limber/MIXMockServer.cpp

+ return;
+ }
+
+ if(iq->getPayload<RosterPayload>()) {


Shouldn't this if be in the handleElementReceivedForAccount method? After all roster queries go to the account, not?

Roster queries does not have a "to" field, therefore if conditions down there won't pass.

handleElementReceivedForAccount is handling all queries for user's own local server.

tfar · 2017-08-21T16:27:20Z

Limber/MIXMockServer.cpp

+ handleElementReceivedForMIXChannel(iq, session);
+ }
+
+ //If request comes to domain service


Could you move the comments for these if inside the if block so that the else if (… directly follows the line with the }, thanks. The current style makes it appear an if statement has ended while the corresponding else is following up a couple lines below.

tfar · 2017-08-21T16:29:57Z

Limber/MIXMockServer.cpp

+
+ private:
+ IDGenerator idGenerator_;
+ PlatformXMLParserFactory xmlParserFactory;


Inconsistent, should end with _ as well.

This allows different client of same user to connect to server and retrieves already connected channels. License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details. Test-Information: Tests added for joining and leaving MIX channels as in XEP-0369, which passes. Tested on Ubuntu 16.04 LTS. Change-Id: I31d0b68528ea0653685e6bcbc66e8131cb2c0eb4

tfar · 2017-08-22T15:33:21Z

Limber/MIXMockServer.cpp

+ //If request comes to domain service
+ handleElementReceivedForMIXService(iq, session);
+ } else if (stanza->getTo() == session->getRemoteJID().toBare() || stanza->getTo() == JID()) {
+ //If request comes to own local server


This is to the account and not to the local server, the local server would be to='server domain'.

Updated the comment.

tfar · 2017-08-22T15:41:44Z

Limber/MIXMockServer.cpp

+ }
+
+ std::shared_ptr<RosterPayload> getRosterPayloadFromXMPPRoster(std::shared_ptr<XMPPRosterImpl> roster) {
+ auto rosterPayload = std::make_shared<RosterPayload>();


getFoo() usually describes a getter method. This seems to be a) not a method to retrieve a member and b) a method that does not otherwise access members, so can be static, right?

Yes this can be a static method.

tfar · 2017-08-22T15:45:31Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+
+#include <iostream>
+#include <memory>
+#include <unordered_set>


You are using assert() but do not #include cassert header.

tfar · 2017-08-22T15:49:31Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+ int channelCount = 0;
+ std::cout << "Already Joined channels: " << std::endl;
+ for (auto item : xmppRoster->getItems()) {
+ channelCount++;


Channels are only those in the roster that have the mix annotation. However, the MIX registry should probably be used to fetch the already joined channels, which has access to the roster. This way if the user wants to do something MIX related, they can always go tho the MIXRegistry, right? Opinions, @tarun018 @Kev ?

Seems like a sensible idea. I wanted to hold my client till onInitialRosterPopulated is emitted and syncRegistryWithRoster is successfully executed.

tfar · 2017-08-22T15:50:04Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+ mixRegistry_->onChannelJoinFailed.connect(&handleChannelJoinFailed);
+ mixRegistry_->onChannelLeft.connect(&handleChannelLeft);
+
+ XMPPRoster* xmppRoster = client->getRoster();


Please use auto here.

tfar · 2017-08-22T15:50:33Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+}
+
+static void handleRosterPopulated() {
+ XMPPRoster* xmppRoster = client->getRoster();


Please use auto here.

tfar · 2017-08-22T15:51:02Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+ std::cout << "Connecting..." << std::flush;
+ client->connect(options);
+ {
+ Timer::ref timer = networkFactories.getTimerFactory()->createTimer(30000);


Please use `auto´ here.

tfar · 2017-08-22T15:51:10Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

+ Timer::ref timer = networkFactories.getTimerFactory()->createTimer(30000);
+ timer->onTick.connect(boost::bind(&SimpleEventLoop::stop, &eventLoop));
+
+ Timer::ref disconnectTimer = networkFactories.getTimerFactory()->createTimer(25000);


Please use auto here.

tfar · 2017-08-22T15:52:25Z

Swiften/MIX/MIXRegistry.h

+#pragma once
+
+#include <memory>
+#include <unordered_set>


Missing a #include <string>.

tfar · 2017-08-22T18:12:24Z

Swiften/Examples/MIXListAndJoin/MIXListAndJoin.cpp

@@ -7,6 +7,7 @@
 #include <iostream>
 #include <memory>
 #include <unordered_set>
+#include <cassert>


Please sort headers, there is a script for this.

tfar · 2017-08-22T18:12:51Z

Swiften/MIX/MIXRegistry.h

@@ -8,6 +8,7 @@

 #include <memory>
 #include <unordered_set>
+#include <string>


Please sort headers.

Change-Id: I98bda1d18847cd1fabffde2c0f92fced31d2a5f3

Change-Id: Iab547e141952b29f4c9703035d2a1808927554c4

tfar · 2017-08-23T14:08:15Z

Swiften/MIX/MIXRegistry.cpp

+namespace Swift {
+
+MIXRegistry::MIXRegistry(const JID& ownJID, IQRouter* iqRouter, XMPPRoster* xmppRoster) : ownJID_(ownJID), iqRouter_(iqRouter), xmppRoster_(xmppRoster) {
+ scopedConnection_ = xmppRoster_->onInitialRosterPopulated.connect(boost::bind(&MIXRegistry::syncRegistryWithRoster, this));


scopedConnection_ doesn't sensibly describe the variable. That it is a scoped connection is clear from the type. Maybe name it initialRosterPopulationConnection_ or something like that

tfar · 2017-08-23T14:09:02Z

Swiften/MIX/MIXRegistry.cpp

+
+}
+
+void MIXRegistry::syncRegistryWithRoster() {


This can be a C++11 lambda instead in the ctor, as the method name doesn't match project convention anyway (handle*).

Added a lambda function.

tfar · 2017-08-23T14:10:40Z

Swiften/MIX/MIXRegistry.cpp

+ SWIFT_LOG(debug) << "Syncing with roster items. " << std::endl;
+ for (auto item : xmppRoster_->getItems()) {
+ if (item.isMIXChannel()) {
+ SWIFT_LOG(debug) << item.getJID() << std::endl;


Pretty detailed logging. Probably not needed.

tfar · 2017-08-23T14:10:47Z

Swiften/MIX/MIXRegistry.cpp

+ SWIFT_LOG(debug) << item.getJID() << std::endl;
+ auto i = entries_.find(item.getJID());
+ if (i == entries_.end()) {
+ SWIFT_LOG(debug) << "Adding " << item.getJID() << std::endl;


Pretty detailed logging. Probably not needed.

tfar · 2017-08-23T14:12:39Z

Swiften/MIX/MIXRegistry.h

+ void handleJoinResponse(MIXJoin::ref, ErrorPayload::ref);
+ void handleLeaveResponse(MIXLeave::ref, ErrorPayload::ref);
+
+ public:


Please move those in front of the the private methods above.

Change-Id: Ie4a349c6b9ef35f08f367885755446c316f1e20e

tfar

Looks ok to me now. What do you think @Kev ?

tfar · 2017-09-04T13:40:53Z

@Kev, both @intosi and I have reviewed this PR. If you can give it a review too I can go on and submit it to master.

tarun018 added 2 commits August 9, 2017 15:00

intosi requested changes Aug 17, 2017

View reviewed changes

tfar reviewed Aug 17, 2017

View reviewed changes

tarun018 force-pushed the MIXLimber branch 3 times, most recently from d8ef76f to acb0f16 Compare August 18, 2017 14:31

tfar requested changes Aug 21, 2017

View reviewed changes

tarun018 force-pushed the MIXLimber branch 3 times, most recently from c6aa5b3 to 6c07dce Compare August 21, 2017 12:52

tfar approved these changes Aug 21, 2017

View reviewed changes

tfar requested changes Aug 21, 2017

View reviewed changes

tarun018 force-pushed the MIXLimber branch from 6c07dce to 06d3eac Compare August 21, 2017 15:01

tfar requested changes Aug 21, 2017

View reviewed changes

tarun018 force-pushed the MIXLimber branch 2 times, most recently from f4035e2 to 67238a4 Compare August 21, 2017 20:32

tarun018 force-pushed the MIXLimber branch from 67238a4 to 24a3bd0 Compare August 21, 2017 20:37

tfar requested changes Aug 22, 2017

View reviewed changes

Updated based on feedback

829213a

Change-Id: I98bda1d18847cd1fabffde2c0f92fced31d2a5f3

tarun018 force-pushed the MIXLimber branch from 3908014 to 829213a Compare August 22, 2017 19:02

Update based on feedback

50cf8d0

Change-Id: Iab547e141952b29f4c9703035d2a1808927554c4

tarun018 force-pushed the MIXLimber branch from 127575c to 50cf8d0 Compare August 23, 2017 10:45

tfar requested changes Aug 23, 2017

View reviewed changes

Updated based on feedback

00bcfc1

Change-Id: Ie4a349c6b9ef35f08f367885755446c316f1e20e

tfar approved these changes Aug 23, 2017

View reviewed changes

tfar requested a review from Kev August 23, 2017 20:26

tarun018 changed the title ~~Add Limber MIX Mock Server and MIX Example~~ Add MIXRegistry, Limber MIX Mock Server and MIX Example Aug 28, 2017

intosi approved these changes Sep 4, 2017

View reviewed changes

Add MIXRegistry, Limber MIX Mock Server and MIX Example #88

Are you sure you want to change the base?

Add MIXRegistry, Limber MIX Mock Server and MIX Example #88

Conversation

tarun018 commented Aug 16, 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

tarun018 commented Aug 17, 2017 • edited

tfar 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

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

tarun018 commented Aug 21, 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

tfar 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

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

tarun018 Aug 22, 2017 • edited

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

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

tfar left a comment

Choose a reason for hiding this comment

tfar commented Sep 4, 2017

tarun018 commented Aug 17, 2017 •

edited

tarun018 Aug 22, 2017 •

edited