Implement Read Marker API #2120

lukebarnard1 · 2017-04-11T14:18:17Z

This implements the design at https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit#heading=h.8jqdhkapw60, summarised below.

Update: see #2128 for further alterations. The summary below has been updated.

m.read_marker event

{
    "content": {
        "event_id”: “$123456789098765hhjGslf:matrix.org"
    },
    "room_id": "!someroomid:matrix.org",
    "type": "m.read_marker"
}

To update it, the client sends a PUT.

RESTful HTTP endpoint

PUT /_matrix/client/r0/rooms/%21someroomid%3Amatrix.org/read_markers HTTP/1.1
Content-Type: application/json

{
	"m.read": "$347926718ewsc:example.com",
	"m.read_marker": "$4536432343kljf:example.com"
}

The REST API hits:

the RR handler if “m.read” has been set. This is optional and means that we don’t end up doing one call for RM and one separate call for RR, which is the common case when the user has caught up on their scrollback;
the RM handler if “m.read_marker” has been set. This handler asserts that the RM event ID is ahead in the stream in comparison to the current one and then uses the room account data API to store the m.read_marker event.

(See https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit#heading=h.lndohpg8at5u)

- This change causes a 405 to be sent if "m.read_marker" is set via /account_data - This also fixes-up the RM endpoint so that it actually Works.

erikjohnston · 2017-04-11T15:30:43Z

synapse/handlers/read_marker.py

@@ -0,0 +1,91 @@
+# -*- coding: utf-8 -*-
+# Copyright 2015, 2016 OpenMarket Ltd


erikjohnston · 2017-04-11T15:31:10Z

synapse/handlers/read_marker.py

+        """
+
+        # Get ordering for existing read marker
+        with (yield self.read_marker_linearizer.queue(room_id + "_" + user_id)):


Just use a tuple: (room_id, user_id)

erikjohnston · 2017-04-11T15:33:00Z

synapse/handlers/read_marker.py

+                retcols=["topological_ordering", "stream_ordering"],
+                keyvalues={"event_id": event_id},
+                allow_none=True
+            )


This should be a function inside the storage function. Conventionally functions that start with _ are private.

This also should have a desc="..." field so that that metrics work correctly. Usually its just the name of the calling function

erikjohnston · 2017-04-11T15:34:04Z

synapse/handlers/read_marker.py

+                if existing_to > new_to:
+                    should_update = False
+                elif existing_to == new_to and existing_so >= new_so:
+                    should_update = False


I'd move this entire block of code into the storage function and have a dedicated is_event_after function or some such

erikjohnston · 2017-04-11T15:35:50Z

Also, you upset pep8:

$ flake8 synapse tests 
synapse/handlers/read_marker.py:20:1: F401 'synapse.util.logcontext.PreserveLoggingContext' imported but unused
synapse/handlers/read_marker.py:22:1: F401 'synapse.types.get_domain_from_id' imported but unused
synapse/handlers/read_marker.py:28:1: E302 expected 2 blank lines, found 1
synapse/rest/client/v2_alpha/account_data.py:87:17: E128 continuation line under-indented for visual indent
synapse/rest/client/v2_alpha/account_data.py:88:17: E128 continuation line under-indented for visual indent
synapse/rest/client/v2_alpha/account_data.py:89:13: E124 closing bracket does not match visual indentation
synapse/rest/client/v2_alpha/read_marker.py:18:1: F401 'synapse.api.errors.SynapseError' imported but unused
synapse/rest/client/v2_alpha/read_marker.py:48:43: E703 statement ends with a semicolon

lukebarnard1 · 2017-04-12T08:44:20Z

Actually, it turns out that there's a bug if you haven't set a RM for a room previously. In the handler on line 46 it assumes that there is a m.read_marker key.

lukebarnard1 · 2017-04-12T08:48:19Z

Fixed!

erikjohnston · 2017-04-12T09:01:47Z

synapse/handlers/read_marker.py

+        the read marker has changed.
+        """
+
+        # Get ordering for existing read marker


Outdated comment?

erikjohnston · 2017-04-12T09:02:29Z

synapse/handlers/read_marker.py

+
+        # Get ordering for existing read marker
+        with (yield self.read_marker_linearizer.queue((room_id, user_id))):
+            account_data = yield self.store.get_account_data_for_room(user_id, room_id)


We probably should be using a storage function that pulls out based on type too. I think there already is one?

There's one for global_account_data but not for_room

erikjohnston · 2017-04-12T09:02:55Z

synapse/handlers/read_marker.py

+
+            existing_read_marker = None
+            if "m.read_marker" in account_data:
+                existing_read_marker = account_data["m.read_marker"]


This could also be written as:

existing_read_marker = account_data.get("m.read_marker", None)

erikjohnston · 2017-04-12T09:03:46Z

synapse/handlers/read_marker.py

+                    user_id, room_id, "m.read_marker", content
+                )
+                self.notifier.on_new_event(
+                    "account_data_key", max_id, users=[user_id], rooms=[room_id]


We don't need to notify the entire room about this change, just the user.

erikjohnston · 2017-04-12T09:05:07Z

synapse/rest/client/v2_alpha/account_data.py

+            raise SynapseError(
+                405,
+                "Cannot set m.read_marker through this API. "
+                "Use /rooms/!roomId:server.name/read_marker"


For future reference we tend to put spaces at the start:

"Cannot set m.read_marker through this API." " Use /rooms/!roomId:server.name/read_marker"

This makes it more obvious that there is at least one space, which is quite important for e.g. SQL

erikjohnston · 2017-04-12T09:06:32Z

synapse/rest/client/v2_alpha/read_marker.py

+
+    def __init__(self, hs):
+        super(ReadMarkerRestServlet, self).__init__()
+        self.hs = hs


Try to avoid taking a reference to hs if possible, and instead just pull out everything you need. This is for cleanliness and makes it clearer what the dependencies of this class is.

erikjohnston · 2017-04-12T09:07:17Z

synapse/rest/client/v2_alpha/read_marker.py

+        body = parse_json_object_from_request(request)
+
+        if "m.read" in body:
+            read_event_id = body["m.read"]


I usually do:

read_event_id = body.get("m.read", None) if read_event_id: ...

but its a matter of taste

erikjohnston · 2017-04-12T09:08:26Z

synapse/storage/events.py

+        if to_1 > to_2:
+            is_after = False
+        elif to_1 == to_2 and so_1 >= so_2:
+            is_after = False


I think you can actually write this as:

is_after = (to_1, so_1) > (to_2, so_2)

Sadly,

>>> ( 1, 2 ) < ( 1, 2 ) False >>> ( 1, 2 ) < ( 1, 3 ) True

So we'll have to go for the form with two comparisons. is_event_after is now also an accurate name with this change...

That's exactly what you want though, isn't it?

erikjohnston · 2017-04-12T09:09:29Z

Just a few nits, nothing particularly important. Feel free to ignore the various style comments depending on your taste

This is now stored on the server with similar treatment to RRs. The server will only store the specified eventId as the current read marker for a room if the event is ahead in the stream when compared to the existing RM. The exception is when the RM has never been set for this room for this user, in which case the event ID will be stored as the RM without any comparison. This API also allows for an optional RR event ID to be sent in the same request. This is because it might be the common case for some clients to update the RM at the same time as updating the RR. See design: https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit See server-side PR: matrix-org/synapse#2120

erikjohnston · 2017-04-13T09:40:34Z

synapse/storage/events.py

+        """
+        to_1, so_1 = yield self._get_event_ordering(event_id1)
+        to_2, so_2 = yield self._get_event_ordering(event_id2)
+        defer.returnValue(to_1 > to_2 and so_1 > so_2)


This is not equivalent to (to_1, so_1) > (to_2, so_2)?

in IRL we disproved my lovely boolean algebra; it had an error in it. sads.

erikjohnston · 2017-04-13T13:13:37Z

lgtm

* Implement API for setting RM This is now stored on the server with similar treatment to RRs. The server will only store the specified eventId as the current read marker for a room if the event is ahead in the stream when compared to the existing RM. The exception is when the RM has never been set for this room for this user, in which case the event ID will be stored as the RM without any comparison. This API also allows for an optional RR event ID to be sent in the same request. This is because it might be the common case for some clients to update the RM at the same time as updating the RR. See design: https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit See server-side PRs: matrix-org/synapse#2120, matrix-org/synapse#2128

@psaavedra

Changes in synapse v0.21.0 (2017-05-18) ======================================= No changes since v0.21.0-rc3 Changes in synapse v0.21.0-rc3 (2017-05-17) =========================================== Features: * Add per user rate-limiting overrides (PR matrix-org#2208) * Add config option to limit maximum number of events requested by ``/sync`` and ``/messages`` (PR matrix-org#2221) Thanks to @psaavedra! Changes: * Various small performance fixes (PR matrix-org#2201, matrix-org#2202, matrix-org#2224, matrix-org#2226, matrix-org#2227, matrix-org#2228, matrix-org#2229) * Update username availability checker API (PR matrix-org#2209, matrix-org#2213) * When purging, don't de-delta state groups we're about to delete (PR matrix-org#2214) * Documentation to check synapse version (PR matrix-org#2215) Thanks to @hamber-dick! * Add an index to event_search to speed up purge history API (PR matrix-org#2218) Bug fixes: * Fix API to allow clients to upload one-time-keys with new sigs (PR matrix-org#2206) Changes in synapse v0.21.0-rc2 (2017-05-08) =========================================== Changes: * Always mark remotes as up if we receive a signed request from them (PR matrix-org#2190) Bug fixes: * Fix bug where users got pushed for rooms they had muted (PR matrix-org#2200) Changes in synapse v0.21.0-rc1 (2017-05-08) =========================================== Features: * Add username availability checker API (PR matrix-org#2183) * Add read marker API (PR matrix-org#2120) Changes: * Enable guest access for the 3pl/3pid APIs (PR matrix-org#1986) * Add setting to support TURN for guests (PR matrix-org#2011) * Various performance improvements (PR matrix-org#2075, matrix-org#2076, matrix-org#2080, matrix-org#2083, matrix-org#2108, matrix-org#2158, matrix-org#2176, matrix-org#2185) * Make synctl a bit more user friendly (PR matrix-org#2078, matrix-org#2127) Thanks @APwhitehat! * Replace HTTP replication with TCP replication (PR matrix-org#2082, matrix-org#2097, matrix-org#2098, matrix-org#2099, matrix-org#2103, matrix-org#2014, matrix-org#2016, matrix-org#2115, matrix-org#2116, matrix-org#2117) * Support authenticated SMTP (PR matrix-org#2102) Thanks @DanielDent! * Add a counter metric for successfully-sent transactions (PR matrix-org#2121) * Propagate errors sensibly from proxied IS requests (PR matrix-org#2147) * Add more granular event send metrics (PR matrix-org#2178) Bug fixes: * Fix nuke-room script to work with current schema (PR matrix-org#1927) Thanks @zuckschwerdt! * Fix db port script to not assume postgres tables are in the public schema (PR matrix-org#2024) Thanks @jerrykan! * Fix getting latest device IP for user with no devices (PR matrix-org#2118) * Fix rejection of invites to unreachable servers (PR matrix-org#2145) * Fix code for reporting old verify keys in synapse (PR matrix-org#2156) * Fix invite state to always include all events (PR matrix-org#2163) * Fix bug where synapse would always fetch state for any missing event (PR matrix-org#2170) * Fix a leak with timed out HTTP connections (PR matrix-org#2180) * Fix bug where we didn't time out HTTP requests to ASes (PR matrix-org#2192) Docs: * Clarify doc for SQLite to PostgreSQL port (PR matrix-org#1961) Thanks @benhylau! * Fix typo in synctl help (PR matrix-org#2107) Thanks @HarHarLinks! * ``web_client_location`` documentation fix (PR matrix-org#2131) Thanks @matthewjwolff! * Update README.rst with FreeBSD changes (PR matrix-org#2132) Thanks @feld! * Clarify setting up metrics (PR matrix-org#2149) Thanks @encks!

lukebarnard1 and others added 2 commits April 11, 2017 11:55

Initial commit of RM server-side impl

e263c26

(See https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit#heading=h.lndohpg8at5u)

Finish implementing RM endpoint

d892079

- This change causes a 405 to be sent if "m.read_marker" is set via /account_data - This also fixes-up the RM endpoint so that it actually Works.

lukebarnard1 assigned erikjohnston Apr 11, 2017

erikjohnston suggested changes Apr 11, 2017

View reviewed changes

erikjohnston changed the title ~~Luke/read markers~~ Implement Read Marker API Apr 11, 2017

erikjohnston assigned lukebarnard1 and unassigned erikjohnston Apr 11, 2017

Luke Barnard added 5 commits April 11, 2017 17:07

flake8

0127423

Copyright

131485e

Refactor event ordering check to events store

7388026

flake8

867822f

travis flake8..

7f94709

Handle no previous RM

77fb2b7

erikjohnston reviewed Apr 12, 2017

View reviewed changes

Luke Barnard added 5 commits April 12, 2017 10:48

Remove comment, simplify null-guard

122cd52

Only notify user, not entire room

69a1851

Move a space

b9676a7

Remove Unused ref to hs

c0aba0a

More null-guard changes

cf6121e

Simplify is_event_after logic

b955706

lukebarnard1 assigned erikjohnston and unassigned lukebarnard1 Apr 12, 2017

lukebarnard1 mentioned this pull request Apr 12, 2017

Implement API for setting RM matrix-org/matrix-js-sdk#419

Merged

erikjohnston reviewed Apr 13, 2017

View reviewed changes

Correct logic in is_event_after

6a70647

erikjohnston approved these changes Apr 13, 2017

View reviewed changes

lukebarnard1 merged commit 78f0ddb into develop Apr 13, 2017

This was referenced Apr 18, 2017

Self-read markers should be synchronised across clients element-hq/element-web#1159

Closed

Sync RM across instances of Riot matrix-org/matrix-react-sdk#805

Merged

lukebarnard1 mentioned this pull request May 8, 2017

Add new Read Marker API to docs matrix-org/matrix-spec-proposals#910

Closed

hawkowl deleted the luke/read-markers branch September 20, 2018 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Read Marker API #2120

Implement Read Marker API #2120

lukebarnard1 commented Apr 11, 2017 •

edited

Loading

erikjohnston Apr 11, 2017

erikjohnston Apr 11, 2017

erikjohnston Apr 11, 2017

erikjohnston Apr 11, 2017

erikjohnston commented Apr 11, 2017

lukebarnard1 commented Apr 12, 2017

lukebarnard1 commented Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

lukebarnard1 Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

erikjohnston Apr 12, 2017

lukebarnard1 Apr 12, 2017

erikjohnston Apr 13, 2017

erikjohnston commented Apr 12, 2017

erikjohnston Apr 13, 2017

lukebarnard1 Apr 13, 2017

erikjohnston commented Apr 13, 2017

		@@ -0,0 +1,91 @@
		# -- coding: utf-8 --
		# Copyright 2015, 2016 OpenMarket Ltd

Implement Read Marker API #2120

Implement Read Marker API #2120

Conversation

lukebarnard1 commented Apr 11, 2017 • edited Loading

m.read_marker event

RESTful HTTP endpoint

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

erikjohnston commented Apr 11, 2017

lukebarnard1 commented Apr 12, 2017

lukebarnard1 commented Apr 12, 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

erikjohnston commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Apr 13, 2017

lukebarnard1 commented Apr 11, 2017 •

edited

Loading