Document how presence EDUs work between servers #1482

turt2live · 2018-08-03T22:30:22Z

Rendered: see 'docs' commit status.

This PR is blocked on #1463 as it makes use of the "definitions embedded in the spec" feature, and builds upon EDUs.

Blocked on:

Improve documentation around EDUs and PDUs #1463

It's worth noting that Synapse does not make use of the poll or unpoll fields, and therefore the wording has been updated to permit servers to reject users. In the case of synapse, it would automatically reject everyone in the list by nature of ignoring it.

It's worth noting that Synapse does not make use of the `poll` or `unpoll` fields, and therefore the wording has been updated to permit servers to reject users. In the case of synapse, it would automatically reject everyone in the list by nature of ignoring it.

richvdh · 2018-08-17T12:32:19Z

scripts/templating/matrix_templates/units.py

@@ -49,6 +49,7 @@
    os.path.join(matrix_doc_dir, "api/identity/definitions"): "is",
    os.path.join(matrix_doc_dir, "api/push-gateway/definitions"): "push",
    os.path.join(matrix_doc_dir, "api/server-server/definitions"): "ss",
+    os.path.join(matrix_doc_dir, "api/server-server/definitions/presence"): "ss_presence",


is this used?

indeed the spec seems to refer to ss_event_schemas which doesn't seem to be defined?

that said it seems to be working somehow. I'm confused.

The magic was added in an earlier commit: ea307b5

The TLDR is it adds another section to the templating.

right. Nevertheless api/server-server/definitions/presence doesn't seem to exist.

richvdh · 2018-08-17T13:10:22Z

api/server-server/definitions/event-schemas/m.presence.accept.yaml

@@ -0,0 +1,45 @@
+# Copyright 2018 New Vector Ltd


hum; the existing event schemas are in /event-schemas/schema.

I'm not entirely convinced there is a clear enough delineation between "events that are only for the S-S API" and "events that are only for clients" and "events that may be of interest to both" to justify putting them in separate directories.

(Note to self for when I inevitably dig this up again: the existing schemas really are yaml, as per the rejected #320.)

I'm concerned about doing this for two reasons:

The server events are slightly different in some cases, and would like to keep them away from the c2s to avoid accidental use.

This means another symlink, which I might just flip a table over if I have to keep fixing the damn things.

One solution is to make event-schemas a magical directory that gets referenced/loaded in the python script, but that feels like a whole new PR to me (and potentially dangerous).

richvdh · 2018-08-17T13:12:18Z

api/server-server/definitions/event-schemas/m.presence.accept.yaml

+  presence of the the observed user.
+allOf:
+  - $ref: ../edu.yaml
+  - type: object


the other schema files don't nest the 'type/properties' under 'allOf': is there a reason we are doing so here?

I don't understand the question. This is to merge an EDU with more specific definitions for the type, content, etc

right, but other files which use allOf do it like:

type: object allOf: - $ref: ../edu.yaml properties: ...

(instead of nesting properties under allOf).

I'm not sure if one is more correct than the other, but I'd like to know if we're making a decision to change it.

Where are you seeing the properties at such a high level? The OpenAPI way is to put them with the definition under allOf.

for example, https://github.com/matrix-org/matrix-doc/blob/master/event-schemas/schema/m.room.message

I can see that's probably wrong, though.

yea, I'm tempted to call that wrong and push forward with doing it the "OpenAPI" way (I think it technically works in either way though). Potentially fixing the other cases of it are worth doing in the last hours of August.

richvdh · 2018-08-17T13:15:33Z

api/server-server/definitions/event-schemas/m.presence.accept.yaml

+  - type: object
+    properties:
+      edu_type:
+        type: string


the other schema files use a single-member enum for this

richvdh · 2018-08-17T13:45:31Z

specification/server_server_api.rst

+
+{{definition_ss_event_schemas_m_presence_accept}} 
+
+{{definition_ss_event_schemas_m_presence_accept}}


s/accept/deny/ I guess

richvdh · 2018-08-17T13:46:52Z

api/server-server/definitions/event-schemas/m.presence.yaml

+                currently_active:
+                  type: boolean
+                  description: |-
+                    Whether or not the user is currently using a device of theirs.


can we define this in a clearer way. What does "currently" mean? How does it interact with 'last active ago'?

(github didn't collapse the thread - improved documentation by adding a blurb below this)

OK, I'm afraid the first sentence here is still pretty tortuous.

"Whether the user is currently active." is probably just as helpful and doesn't involve an ugly dangling possessive

Wording suggestions appreciated here - I'm having troubles coming up with a better way to word this at the moment.

How does this sound:

True if the user is likely to be interacting with their client. This may be indicated by the user having a last_active_ago within the last few minutes. Defaults to false.

well, that sounds great :)

richvdh · 2018-08-17T13:47:27Z

api/server-server/definitions/event-schemas/m.presence.yaml

+            description: |-
+              A list of presence updates that the receiving server is likely
+              to be interested in, or is subscribed to.
+            items:


can we share the definitions with the C-S side m.presence? It seems annoying that we have to maintain all this text twice.

potentially, I'll take a closer look at the client-server format and make them link if I can.

richvdh · 2018-08-17T13:53:31Z

specification/server_server_api.rst


 .. TODO-doc
  - Explain the timing-based round-trip reduction mechanism for presence
    messages
  - Explain the zero-byte presence inference logic
  See also: docs/client-server/model/presence

+{{definition_ss_event_schemas_m_presence}}


It's jarring that this ends up formatted very differently to the event schemas in the C-S spec.

My dream is to put the c2s spec through the same treatment.

Hum. the heading is fugly. can it be made to say "m.presence.invite schema" or "m.presence.invite EDU schema" rather than "Presence Invite EDU Schema" (cf also #1531)

richvdh · 2018-08-17T13:54:10Z

As discussed, the poll and unpoll fields seem to be redundant, and since they're unimplemented, we should just take them out.

Half-Shot · 2018-08-17T14:27:45Z

Also worth mentioning you've explained the poll and unpoll stuff very clearly and then the invite event format is tacked on the end without explanation, can this get an explanation?

Reference: https://github.com/matrix-org/synapse/blob/d69decd5c78c72abef50b597a689e2bc55a39702/synapse/handlers/presence.py#L66-L68

…ence

turt2live · 2018-08-17T15:36:44Z

@richvdh @Half-Shot please take another look. I'm still hesitant to make the client-server and server-server specs share events at this stage and kinda feel it's best left solved in matrix-org/matrix-spec#348. The typing EDU has been updated in this PR because it was missed in it's PR, and it feels minor enough to not bother with it's own PR.

…ence

richvdh · 2018-08-17T15:55:01Z

scripts/templating/matrix_templates/units.py

@@ -49,6 +49,7 @@
    os.path.join(matrix_doc_dir, "api/identity/definitions"): "is",
    os.path.join(matrix_doc_dir, "api/push-gateway/definitions"): "push",
    os.path.join(matrix_doc_dir, "api/server-server/definitions"): "ss",
+    os.path.join(matrix_doc_dir, "api/server-server/definitions/presence"): "ss_presence",


right. Nevertheless api/server-server/definitions/presence doesn't seem to exist.

richvdh · 2018-08-17T16:08:42Z

specification/server_server_api.rst


 .. TODO-doc
  - Explain the timing-based round-trip reduction mechanism for presence
    messages
  - Explain the zero-byte presence inference logic
  See also: docs/client-server/model/presence

+{{definition_ss_event_schemas_m_presence}}


Hum. the heading is fugly. can it be made to say "m.presence.invite schema" or "m.presence.invite EDU schema" rather than "Presence Invite EDU Schema" (cf also #1531)

richvdh · 2018-08-17T16:19:12Z

api/server-server/definitions/event-schemas/m.presence.accept.yaml

+  presence of the the observed user.
+allOf:
+  - $ref: ../edu.yaml
+  - type: object


right, but other files which use allOf do it like:

type: object allOf: - $ref: ../edu.yaml properties: ...

(instead of nesting properties under allOf).

I'm not sure if one is more correct than the other, but I'd like to know if we're making a decision to change it.

richvdh · 2018-08-17T16:26:16Z

api/server-server/definitions/event-schemas/m.presence.yaml

+                  type: integer
+                  format: int64
+                  description: |-
+                    The number of milliseconds that have ellapsed since the user


richvdh · 2018-08-17T16:32:13Z

api/server-server/definitions/event-schemas/m.presence.yaml

+                currently_active:
+                  type: boolean
+                  description: |-
+                    Whether or not the user is currently using a device of theirs.


OK, I'm afraid the first sentence here is still pretty tortuous.

"Whether the user is currently active." is probably just as helpful and doesn't involve an ugly dangling possessive

richvdh · 2018-08-17T16:32:38Z

api/server-server/definitions/event-schemas/m.presence.yaml

+                status_msg:
+                  type: string
+                  description: An optional description to accompany the presence.
+                  example: "Making cupcakes"


yum, cupcakes.

…anup of titles

Half-Shot · 2018-08-19T20:02:37Z

@turt2live Thank you, the description is much easier to follow now for invites.

turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Aug 3, 2018

turt2live mentioned this pull request Aug 3, 2018

Federation API r0 megathread #1464

Closed

35 tasks

turt2live force-pushed the travis/s2s/presence branch from f44d0cd to f1ebe8f Compare August 4, 2018 01:52

turt2live force-pushed the travis/s2s/presence branch from f1ebe8f to 05a2427 Compare August 8, 2018 14:24

turt2live removed the blocked Something needs to be done before action can be taken on this PR/issue. label Aug 8, 2018

turt2live requested a review from a team August 8, 2018 14:24

richvdh requested changes Aug 17, 2018

View reviewed changes

richvdh assigned turt2live Aug 17, 2018

turt2live added 6 commits August 17, 2018 09:16

Remove poll/unpoll from presence

e7aed3d

Add a note about how presence lists work

ff1afaa

Add a mention about how currently_active works

549a25c

Reference: https://github.com/matrix-org/synapse/blob/d69decd5c78c72abef50b597a689e2bc55a39702/synapse/handlers/presence.py#L66-L68

Use strict types for the presence EDUs

766402a

Merge remote-tracking branch 'matrix-org/master' into travis/s2s/pres…

a53fa93

…ence

Add a strict type the m.typing EDU

96896fe

turt2live requested a review from richvdh August 17, 2018 15:36

turt2live assigned richvdh and unassigned turt2live Aug 17, 2018

Merge remote-tracking branch 'matrix-org/master' into travis/s2s/pres…

ebca4c7

…ence

richvdh reviewed Aug 17, 2018

View reviewed changes

turt2live added 2 commits August 17, 2018 10:53

Rename the presence EDU files to be accurate to their types; Misc cle…

750d4f9

…anup of titles

Spelling

a4015d5

richvdh assigned turt2live and unassigned richvdh Aug 20, 2018

Improve description for currently_active

e712466

turt2live assigned richvdh and unassigned turt2live Aug 20, 2018

Don't try and be fancy about titles

8b65da1

turt2live requested a review from richvdh August 20, 2018 20:39

richvdh approved these changes Aug 21, 2018

View reviewed changes

richvdh assigned turt2live and unassigned richvdh Aug 21, 2018

turt2live merged commit 1d7ea31 into matrix-org:master Aug 21, 2018

turt2live deleted the travis/s2s/presence branch August 21, 2018 17:26

turt2live mentioned this pull request Aug 27, 2018

Clarify the meaning of the currently_active flags in presence events (SPEC-419) #686

Closed


		{{definition_ss_event_schemas_m_presence_accept}}

		{{definition_ss_event_schemas_m_presence_accept}}

Document how presence EDUs work between servers #1482

Document how presence EDUs work between servers #1482

Conversation

turt2live commented Aug 3, 2018 • 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

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

turt2live Aug 17, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Aug 17, 2018

Half-Shot commented Aug 17, 2018

turt2live commented Aug 17, 2018

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

Half-Shot commented Aug 19, 2018

turt2live commented Aug 3, 2018 •

edited

Loading

turt2live Aug 17, 2018 •

edited

Loading