fix(client-server): Fix a typo in /rooms/{roomId}/relations/{eventId}

[Hywan]

This patch fixes a typo in /rooms/{roomId}/relations/{eventId}. The specification says about the from request query parameter:

The pagination token to start returning results from. If not supplied, results start at the most recent topological event known to the server.

Can be a next_batch or prev_batch token from a previous call, or a returned start token from /messages, or a next_batch token from /sync.

The last part is wrong. It should be:

… or a prev_batch token from /sync.

This change has been discussed with @erikjohnston.

Pull Request Checklist

• Pull request includes a changelog file
• Pull request includes a sign off
• Pull request is classified as 'other changes'

Preview: https://pr2357--matrix-spec-previews.netlify.app

[richvdh]

I did some archaeology here.

Originally, the spec PR for /relations, #1062, suggested that this could be either a next_batch or a prev_batch from /sync; but since at that time, there was no dir param, that got changed in 5bb8b3e#diff-4b4318313fad520775e17b4bb460fd99d4f9471657b4ce51c98129af5bf67ac6L60-L65. However, I think it was probably changed the wrong way, and should indeed have been prev_batch, as suggested.

Note that the MSC that added /relations, MSC2675, was pretty vague: "Tokens from /sync or /messages can be passed to /relations to only get relating events from a section of the timeline."

Later, MSC3715 added the dir param, but explicitly proposed keeping the "next_batch token from /sync".

---

I think this is probably a fair change, though it does make me wonder if changing the spec in this way will make any existing implementations non-compliant.

Do we know what Synapse allows here?

[richvdh]

We just need a once-sentence summary of the change here. See previous changelog entries for examples: https://spec.matrix.org/v1.18/changelog/v1.18/

[Hywan]

Updated and rebased.

[Hywan]

Thank you for the archaeological session.

Synapse supports using a prev_batch token from /sync as a from value for /relations. That's the reason of my patch here: the implementation mismatches the spec and I believed the spec contained a typo.

[richvdh]

Synapse supports using a prev_batch token from /sync as a from value for /relations. That's the reason of my patch here: the implementation mismatches the spec and I believed the spec contained a typo.

And to confirm: it doesn't allow a next_batch?

[Hywan]

I don't know. Synapse uses a StreamToken without further type checking.

The only constraint I see is with this sentence from the specification:

Note that when paginating the from token should be “after” the to token in terms of topological ordering, because it is only possible to paginate “backwards” through events, starting at from.

So from must represent a point in time that is more recent than to, i.e. from > to. Okay. So technically, it also imposes next_batch > prev_batch.

However, nothing seems to forbid to use next_patch or prev_batch (from /sync or /messages) as the from or to values. If it's a StreamToken, then… it's a “point in time”.

I contradict myself with my initial goal though. Having a second thought about this: it's not an error to use next_batch as the from value. What is an error is to suggest that prev_batch is excluded.

I think the to parameter phrases things a bit better:

Like from, this can be a previous token from a prior call to this endpoint or from /messages or /sync.

/sync, /messages, or /relations can use tokens interchangeably as they only represent an “anchor/position in the topological stream of events”. And /relations is smart enough to add a constraint with from > to. I think that's the correct way to phrase it.

So I propose to rephrase the from description to:

The pagination token to start returning results from. If not supplied, results start at the most recent topological event known to the server.

Can be a token from /messages (start or end) or from /sync (prev_batch or next_batch).

Ideally, the specification would create a type for these tokens instead of having a string, but that's for another day.

What do you think?

[richvdh]

Those excerpts from the spec are quite hard to parse, without context. It would be helpful if you could link to the relevant section of the spec. I guess we're talking about https://spec.matrix.org/v1.18/client-server-api/#get_matrixclientv1roomsroomidrelationseventid?

So I propose to rephrase the from description to:

The pagination token to start returning results from. If not supplied, results start at the most recent topological event known to the server.
Can be a token from /messages (start or end) or from /sync (prev_batch or next_batch).

I think that's mostly fine, but it does worry me that this is a breaking change in the spec, and as such really ought to have an MSC. We're asking server implementations to support something they might currently disallow, which means that really people ought to have a chance to object to it.

We can make an exception if we can say "this is clearly what was meant", but I'm not 100% sure we can, in this case.

@erikjohnston what is your feeling here?

[Hywan]

I guess we're talking about https://spec.matrix.org/v1.18/client-server-api/#get_matrixclientv1roomsroomidrelationseventid?

Yes.

[erikjohnston]

The intention with these sort of pagination APIs is to be able to fill in the gaps between two syncs, which you can either do via:

1. Typically, dir=b and setting from as the prev_batch of the later sync and to to the next_batch of the earlier sync.
2. Or, dir=f and setting from as the next_batch of the earlier sync and to to the prev_batch of the later sync.

I think clarifying the above in the spec should be non-controversial.

Synapse does support any combination of from/to and prev_batch/next_batch regardless of dir, but that is probably more due to implementation details. I'm not sure how much it makes sense to support this expanded operation: e.g. supporting from being set to a next_batch when dir=b will result in the pagination including things that were part of the /sync response. You could imagine wanting to do so if you'd e.g. filtered out relations from /sync responses and then wanted to fill them in, but that feels quite convoluted/niche.

[richvdh]

Synapse does support any combination of from/to and prev_batch/next_batch regardless of dir, but that is probably more due to implementation details.

I kinda wish that it only allowed you do to things that were explicitly permitted in the spec, to stop clients relying on things that aren't so permitted, and thus ending up being incompatible with other servers. But 🤷.

@Hywan can we make the text match Erik's description?

[Hywan]

@richvdh Hope the new version is right and concise.

[MadLittleMods]

Starting a proper thread for this discussion.

Pagination tokens are positions between events. You should be able to further paginate from any position.

While this information is coming from my Synapse knowledge, I don't think that idea is Synapse specific.

You can see the sort of Complement tests I added around this kind of thing for /timestamp_to_event where you can paginate backwards from the start or end tokens,

[Hywan]

Any suggestion of how it should be phrased then?

[richvdh]

Why would you want to paginate backwards from an end token?

[MadLittleMods]

It boils down to "because you can" and client flexibility. They may not want to process the event from /context and instead are only using it to get a pagination token to use as part of their normal pagination flows (uniform handling).

A more technical reason is perhaps you're using the filter query parameter: with /context, "It is not applied to the event itself" but it will apply consistently with /messages.

[richvdh]

Honestly "because Synapse lets me get away with it" doesn't hold much water for me here. We need to be mindful of meeting the requirements of a client without unduly constraining server implementations. Further, since we are talking about changing the definition of an existing, specified endpoint, there is a fine line between "correcting an obvious error" and "changing the way the endpoint works". It is not fair to other server implementations to suddenly add requirements without following the MSC process.

Nevertheless, an analogy with /messages is somewhat compelling. The spec for that has no restriction against paginating backwards from a next_batch in /sync, so we should probably replicate that. (Though it does appear to preclude paginating in either direction from the results of /context or /search, which I don't think is deliberate.)

(Also, why would we only permit paginating backwards from the start of a /messages rather than an end?)

So I guess we should loosen these constraints.

Suggested change

	If `dir=b`, then `from` can be `next_batch` from a previous call, or a
	`prev_batch` token from a [`/sync`] or a `start` token from a [`/messages`].
	This token can be any of:
	* `next_batch` from the response to a previous call.
	* a `prev_batch` or `next_batch` token from a [`/sync`] response.
	* a `start` or `end` token from a [`/messages`], [`/context`] or [`/search`] response

(And remove the separate description for dir=f)

I'm hoping for insight from the rest of the SCT on this, because frankly I'm struggling to decide.

[richvdh]

This looks sensible to me now, though waiting for Eric's comments before merging

[richvdh]

Honestly "because Synapse lets me get away with it" doesn't hold much water for me here. We need to be mindful of meeting the requirements of a client without unduly constraining server implementations. Further, since we are talking about changing the definition of an existing, specified endpoint, there is a fine line between "correcting an obvious error" and "changing the way the endpoint works". It is not fair to other server implementations to suddenly add requirements without following the MSC process.

Nevertheless, an analogy with /messages is somewhat compelling. The spec for that has no restriction against paginating backwards from a next_batch in /sync, so we should probably replicate that. (Though it does appear to preclude paginating in either direction from the results of /context or /search, which I don't think is deliberate.)

(Also, why would we only permit paginating backwards from the start of a /messages rather than an end?)

So I guess we should loosen these constraints.

Suggested change

	If `dir=b`, then `from` can be `next_batch` from a previous call, or a
	`prev_batch` token from a [`/sync`] or a `start` token from a [`/messages`].
	This token can be any of:
	* `next_batch` from the response to a previous call.
	* a `prev_batch` or `next_batch` token from a [`/sync`] response.
	* a `start` or `end` token from a [`/messages`], [`/context`] or [`/search`] response

(And remove the separate description for dir=f)

I'm hoping for insight from the rest of the SCT on this, because frankly I'm struggling to decide.