Use AS-specific room publication API #306

lukebarnard1 · 2016-12-12T14:14:10Z

Corresponds to the changes outlined here: matrix-org/synapse#1676

network_id (which is an IRC server.domain) has been added to each instance returned by /thirdparty/protocols.
When room visibility is changed, the new endpoint specifically for ASes is used, which specifies the network_id to be the server.domain of the first bridged channel in the list of channels mapped to that room.

This requires matrix-org/matrix-js-sdk#306

This is the same as desc: server.config.name or if thats falsy, then server.domain. This is the same network_id that will be used in the new AS-specific room publishing API.

This requires the commit 742d942 from luke/feature-AS-room-publication

kegsay · 2016-12-12T14:20:17Z

lib/bridge/PublicitySyncer.js

        if (currentState !== correctState) {
-            return cli.setRoomDirectoryVisibility(roomId, correctState).then(
+            return cli.setRoomDirectoryVisibilityAppService(networkId, roomId, correctState).then(


How does this work for the existing Freenode rooms on matrix.org which are not in a networked list?

Much in the same way it would work for those not in the main list.

kegsay · 2016-12-12T14:25:32Z

lib/bridge/PublicitySyncer.js

@@ -181,8 +181,12 @@ PublicitySyncer.prototype._solveVisibility = Promise.coroutine(function*() {
        let currentState = this._visibilityMap.roomVisibilities[roomId];
        let correctState = shouldBePrivate(roomId, []) ? 'private' : 'public';

+        // Use the server domain of the first mapping
+        // 'localhost #channel1' => 'localhost'
+        const networkId = this._visibilityMap.mappings[roomId][0].split(' ')[0];


Is this._visibilityMap.mappings[roomId] guaranteed to be non-null? We seem to do a null guard on :140 but we don't here. But :127 makes me think that yes, it is guaranteed to be non-null. But then why the null guard on :140? Some clarification here would be good.

Also, ignoring the null guard issue, will there always be a mapped channel for this room ID? Now that I think is no, which would then NPE on this line.

The null guard shouldn't be needed, you're right (it didn't originate for any particular reason, I checked in it's blame history). And yes the mappings are guaranteed to be set for a room, and contain at least one channel mapping otherwise that room wouldn't be in the RoomStore, and thus wouldn't be returned by getAllChannelMappings.

But I'd be happy to null guard just in case.

kegsay · 2016-12-12T14:27:26Z

Otherwise LGTM

erikjohnston · 2016-12-12T14:45:25Z

lib/bridge/IrcBridge.js

@@ -560,6 +560,7 @@ IrcBridge.prototype.getThirdPartyProtocol = function(protocol) {
        },
        instances: servers.map((server) => {
            return {
+                network_id: server.domain,


Can this change? e.g. if we change the domain we use to connect to a network? Its important that that this remains constant unless the AS republishes all the rooms after a change.

It's rare, but it can change like when we hop to IPv6 from IPv4. We may want to indirect here (e.g. have a network_id value in the config file, but default to server.domain if one is not supplied).

This becomes relevant when we inevitably move Moznet to v6 (it's still v4 atm).

I had totally forgotten about this. I shall add network_id to the config.

Instead of keying of the server domain for publicity syncing, use a network ID which should be unique amongst the servers being bridged to allow the server domain being changed without public room lists being affected. This networkId could be used in future as a way to key servers on the bridge, although because they are defined as strings they are not guaranteed to be unique.

kegsay · 2016-12-12T16:47:47Z

config.sample.yaml

@@ -31,6 +31,9 @@ ircService:
      # A human-readable description string
      # description: "Example.com IRC network"

+      # An ID for uniquely identifying this server amongst other servers being bridged.
+      networkId: "example"


I would put this in a comment like the description field is, as most people need not care about this field.

kegsay · 2016-12-12T16:48:40Z

lib/irc/IrcServer.js

+ * bridge across all servers/instances. Defaults to the domain of the server.
+ */
+IrcServer.prototype.getNetworkId = function() {
+    return this.config.networkId || server.domain;


What's a server? :)

kegsay · 2016-12-12T16:48:58Z

lib/irc/IrcServer.js

@@ -34,6 +34,14 @@ IrcServer.prototype.getReadableName = function() {
 }

 /**
+ * Return the network ID of this server, which should be unique for this
+ * bridge across all servers/instances. Defaults to the domain of the server.
+ */


JSDoc please.

kegsay · 2016-12-12T16:50:29Z

lib/DataStore.js

@@ -170,7 +170,7 @@ DataStore.prototype.getAllChannelMappings = Promise.coroutine(function*() {
            (e) => e.matrix_id === roomId
        ).map((e) => {
            return {
-                domain: e.remote.domain,
+                networkId: this._serverMappings[e.remote.domain],


Updated the response object without updating docs. Please update docs.

kegsay · 2016-12-12T17:00:00Z

lib/bridge/IrcHandler.js

@@ -573,7 +573,7 @@ IrcHandler.prototype.onMode = Promise.coroutine(function*(req, server, channel,
        }
        // Update the visibility for all rooms connected to this channel
        return this.ircBridge.publicitySyncer.updateVisibilityMap(
-            true, server.domain + ' ' + channel, enabled
+            true, server.getNetworkId() + ' ' + channel, enabled


I really don't like this. I didn't say anything at the time but I'm in a less accepting mood today. I appreciate that the publicity syncer needs to key off a tuple of network ID + channel. I don't feel it is the caller's responsibility to craft the unique key. We should be calling a method which accepts a server and channel and returns the key, so we can consistently apply the concatenation rules. Otherwise, as I'm sure you now appreciate, you need to find alllll the places where we make the key and change them in the same way in order to apply a fix, which makes it really really easy to miss one place. See https://github.com/matrix-org/matrix-appservice-irc/blob/master/lib/DataStore.js#L484 for an example.

kegsay · 2016-12-13T11:42:14Z

lib/bridge/PublicitySyncer.js

@@ -125,7 +140,7 @@ PublicitySyncer.prototype._solveVisibility = Promise.coroutine(function*() {

    roomIds.forEach((roomId) => {
        this._visibilityMap.mappings[roomId] = mappings[roomId].map((mapping) => {
-            return mapping.domain + ' ' + mapping.channel
+            return mapping.networkId + ' ' + mapping.channel


Shouldn't we be calling getIRCVisMapKey?

kegsay · 2016-12-13T11:42:32Z

Otherwise LGTM

kegsay · 2016-12-13T12:28:13Z

lib/bridge/PublicitySyncer.js

@@ -140,7 +141,7 @@ PublicitySyncer.prototype._solveVisibility = Promise.coroutine(function*() {

    roomIds.forEach((roomId) => {
        this._visibilityMap.mappings[roomId] = mappings[roomId].map((mapping) => {
-            return mapping.networkId + ' ' + mapping.channel
+            return getIRCVisMapKey(mapping.networkId, mapping.channel);


You forgot a this.

Luke Barnard added 3 commits December 12, 2016 13:38

Return network_id with each instance

6f6fab9

This is the same as desc: server.config.name or if thats falsy, then server.domain. This is the same network_id that will be used in the new AS-specific room publishing API.

Just use the server domain

4eae977

Use js-sdk setRoomDirectoryVisibilityAppService

9bd881f

This requires the commit 742d942 from luke/feature-AS-room-publication

lukebarnard1 changed the title ~~Luke/api change public rooms~~ Use AS-specific room publication API Dec 12, 2016

lukebarnard1 requested a review from kegsay December 12, 2016 14:16

kegsay reviewed Dec 12, 2016

View reviewed changes

erikjohnston reviewed Dec 12, 2016

View reviewed changes

Luke Barnard added 3 commits December 12, 2016 16:29

Fix networkId used in getAllChannelMappings

f61e08d

Expect more than 0 characters in the networkId

f756a45

kegsay reviewed Dec 12, 2016

View reviewed changes

Luke Barnard added 5 commits December 13, 2016 09:59

Comment out networkId

0850c40

Different comment on getNetworkId

6943267

Fix networkId server -> this

94ee49f

Better getNetworkId comment

5a095ca

Provide a documented key generation function

c9dedb4

lukebarnard1 requested a review from kegsay December 13, 2016 10:16

kegsay reviewed Dec 13, 2016

View reviewed changes

Use getIRCVisMapKey in pub syncer itself

7b98683

kegsay reviewed Dec 13, 2016

View reviewed changes

add "this"

7bdd62c

lukebarnard1 merged commit 416042b into develop Dec 13, 2016

lukebarnard1 mentioned this pull request Mar 2, 2017

add gitter back to room directory element-hq/element-web#3086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use AS-specific room publication API #306

Use AS-specific room publication API #306

lukebarnard1 commented Dec 12, 2016 •

edited

Loading

kegsay Dec 12, 2016

lukebarnard1 Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 12, 2016

lukebarnard1 Dec 12, 2016 •

edited

Loading

kegsay commented Dec 12, 2016

erikjohnston Dec 12, 2016

kegsay Dec 12, 2016

lukebarnard1 Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 12, 2016

kegsay Dec 13, 2016

kegsay commented Dec 13, 2016

kegsay Dec 13, 2016

Use AS-specific room publication API #306

Use AS-specific room publication API #306

Conversation

lukebarnard1 commented Dec 12, 2016 • edited Loading

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

lukebarnard1 Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

kegsay commented Dec 12, 2016

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

kegsay commented Dec 13, 2016

Choose a reason for hiding this comment

lukebarnard1 commented Dec 12, 2016 •

edited

Loading

lukebarnard1 Dec 12, 2016 •

edited

Loading