[WIPish] MSC1776: Implementing peeking via /sync

[ara4n]

Rendered

[maxidorius]

This MSC seems pointless:

1. It defines something that already exists: peek can already be done with /sync and is used in a set of my own projects, and I've seen it used in others like SimpleMatrix. Riot and client based on it are the ones who are still using a deprecated endpoint and should just start using /sync everywhere.
2. It adds a POST method to sync to return filter IDs which needs to be saved for later use, which is exactly the point of the existing filter endpoints and sync with a filter query parameter, just in another order, making this a no-op but adds complexity to sync for no added value.

It's already all in the spec. Am I missing something?

[t3chguy]

How does one peek using /sync 😕?

[maxidorius]

How does one peek using /sync confused?

Per example, most basic filter for Matrix HQ:

{
  "presence": {
    "not_types": [
      "*"
    ]
  },
  "account_data": {
    "not_types": [
      "*"
    ]
  },
  "room": {
    "rooms": [
      "!QtykxKocfZaZOUrTwp:matrix.org"
    ],
    "ephemeral": {
      "not_types": [
        "*"
      ]
    },
    "account_data": {
      "not_types": [
        "*"
      ]
    },
    "timeline": {
      "types": [
        "m.room.message"
      ]
    },
    "state": {
      "types": [
        "m.room.name",
        "m.room.member"
      ]
    }
  }
}

[ara4n]

I can just about believe this might work, but it wasn't intended or designed to work that way (afaik).

That said, i just tried this, with precisely that filter, on an account that wasn't joined to HQ:

curl -g 'https://matrix.org/_matrix/client/r0/sync?access_token='$ACCESS_TOKEN'&filter={"presence":{"not_types":["*"]},"account_data":{"not_types":["*"]},"room":{"rooms":["!QtykxKocfZaZOUrTwp:matrix.org"],"ephemeral":{"not_types":["*"]},"account_data":{"not_types":["*"]},"timeline":{"types":["m.room.message"]},"state":{"types":["m.room.name","m.room.member"]}}}'

It gave me no events from !QtykxKocfZaZOUrTwp:matrix.org at all. I can believe I may be mangling the filter somehow, although failing to see immediately what.

[ara4n]

The good news is that even if filters already behave in the way I propose in this MSC, the MSC still has merit by defining how filters can be unioned together in a single /sync stream, and by providing a simpler way of defining filters than the two-phase approach we have currently (/filter and then /sync).

[maxidorius]

@ara4n I never said synapse implements it that way. This repo is about the spec, and your MSC just has no added value since you replicate something that already exists by just moving stuff around. Filters are one of the first things that a client set ups when login in (Riot does that too) so a POST on /sync wouldn't be used. It feels you're just re-inventing your own wheel at this point.

[jcgruenhage]

@maxidor The reason to reduce those 2 requests into 1 is because the filters used are assumed to change often:

as well as peek changes in flair events for 3 users we're stalking because they're in the timeline currently displayed on screen.

Doubling the amount of http requests (that can't be done in parallel!) for switching rooms isn't nice. If the only thing this MSC achieves is enhancing efficiency, then it's still a good MSC.

[maxidorius]

@jcgruenhage If the filters are supposed to change often and the client doesn't need to save them long-term, then just use the query parameter on GET /sync to directly specify the filter - isn't that why it's there?

[jcgruenhage]

Oh right, that also exists. Good point.

[ara4n]

The current spec says nothing about filters being allowed to somehow magically add rooms which the user isn't in to the /sync response. The word filter itself should be a giveaway that we are applying a filter to remove stuff from the response; not add it in. If you've misinterpreted the spec that way, then apologies. Meanwhile, I feel that you are wasting my time by declaring your weird misinterpretation of the spec as The Truth and declaring the MSC useless.

Anyway, the good news for you is that this MSC proposes to add the behaviour that you assumed was there all along.

Meanwhile, as I pointed out, the MSC adds additional value by letting you combine together different filters within the same /sync stream, which is entirely impossible otherwise.

It's a pleasant convenience that it can also replace the POST /filter API.

[maxidorius]

@ara4n Maybe you should stop wasting people time because the behaviour I outlined is actually given by your spec.

/room/{roomId}/initialSync reads:

This endpoint was deprecated in r0 of this specification. There is no direct replacement; the relevant information is returned by the /sync API. See the migration guide.

As told, let's check the migration guide which states:

There is no direct replacement for the per-room /rooms/:roomId/initialSync endpoint, but the behaviour can be recreated by applying an ad-hoc filter using the filter parameter to /sync that selects only the required room ID:

GET .../sync?filter={"room":{"rooms":[$room_id]}}

I'm only following your own specification. So I never assumed anything, you did because you looked at what synapse is doing which doesn't follow the spec.

[ara4n]

so the confusion here is the phrase "the relevant information is returned by the /sync API" - which wasn't explicit that it was talking about the non-peek scenario? in which case this is a bug/ambiguity in the current spec; thanks for pointing it out. just to be clear: the filter parameter on the /sync endpoint in the spec currently does NOT let you peek into rooms you are not a member of. Just because it doesn't say that it lets you doesn't mean that it does.

[maxidorius]

The spec is explicit, quoting it again:

the behaviour can be recreated by applying an ad-hoc filter using the filter parameter to /sync that selects only the required room ID

There is definitely no ambiguity here. Synapse just doesn't follow it and your MSC which is titled "implementing peeking via /sync" is redundant with the spec which already tells you how to do it.

[ara4n]

no, that's not the spec, it's the migration guide. and it is talking (ambiguously) only about the non-peek use case. this is a bug in the documentation, and Synapse is behaving as expected.

[maxidorius]

So:

1. The spec which is authoritative says "See the migration guide."
2. The migration guide says: "Use ad-hoc filter"

But you say the migration guide is not authoritative even though the spec says to follow it. Weird.

Then:

1. /room/{roomId}/initialSync doesn't explicitly cover peek, but it's used that way for years.
2. /sync doesn't explicitly cover peek either.
3. Actually, peek is not found anywhere in the C2S r0.4.0 spec.

But you say the bug is in the documentation. More weird.

Finally:

1. Peeking is replicated in various tools using /sync
2. synapse is the only one not doing so
3. your MSC basically wants to use /sync like the rest of us have been for months

You say synapse is behaving as expected but you just want to do the same as the rest of us for something the spec doesn't actually prohibit. Now I'm just not buying it.

A MSC to clarify the peek usage of the various endpoints would be better to be honest.

[ara4n]

This is one of those situations where synapse was behaving as intended, and the spec was ambiguous (by not spelling out peek semantics).

Now I'm just not buying it.

Not entirely sure what you're accusing me of now. I had no idea you were abusing /sync like this until the beginning of this thread (or if I knew, i'd forgotten it).

This MSC seeks to clarify how peek works in /sync (to bring it inline with the behaviour you were assuming), which seems worthwhile to me.

It's absolutely ridiculous that you are kicking up all this fuss about an MSC which brings the spec in line with the behaviour you apparently want anyway, which makes me feel like you're just making a fuss to be obstructive.

[maxidorius]

It's absolutely ridiculous that you are kicking up all this fuss about an MSC which brings the spec in line with the behaviour you apparently want anyway, which makes me feel like you're just making a fuss to be obstructive.

It does more than that:

• Creates a new endpoint POST /sync
• It promotes the usage of it, forcing the homeserver to actually store filters which are already advised to be used via query parameters since there are most likely not long-lived or re-used often
• It adds a brand new logic to the Homeserver for applying several filters, which is not currently possible and is possibly a big MSC in its own right
• Defines two new keys for sync only available via POST
• Adds room pagination feature

If peek is about a single room and replicating /room/{roomId}/initialSync, why would you add features relating to more than one room?

So while your MSC claims it's only about implementing peeking via /sync, it does a lot more than that and is far outside the scope of peeking or replicating the initialSync behaviour and it's not just a fuss.

This is a either a feature-creep MSC or mis-scoped MSC but it's not a MSC to solve peek. You don't even talk about the GET endpoint, or about clarifying a behaviour - you just add new behaviour.
If this is only a matter of clarifying an ambiguous situation in the spec, then where is your proposed rewording in the MSC?

[ara4n]

You're putting words into my mouth, yet again.

So while your MSC claims it's only about implementing peeking via /sync

It doesn't. What the MSC says is:

Proposal: we add a POST form of /sync, which lets the user add/remove additional filters on the fly in order to aid peeking into specific sets of rooms with specific filters

If peek is about a single room and replicating /room/{roomId}/initialSync, why would you add features relating to more than one room?

Whoever said peek was just about a single room?

At this point, I am convinced that you are being deliberately argumentative and obstructive, and more interested in picking fights and winning arguments than improving the spec. Given the recent exchange over at #1763 (comment), and the last ~year of obnoxious behaviour, i've had enough and am removing you from the repo. Unfortunately github doesn't let you block users per-repo, so I have to block you from the org.

Thank you for all the bugs and defects you have pointed out, but thankfully there are other people capable and willing to point them out without also consistently behaving like a jerk. On the plus side, hopefully I can now encourage folks back to contribute to the spec who were avoiding it because they didn't want to get into discussions like this...

[turt2live]

Devil's advocate: why not a PUT version of https://matrix.org/docs/spec/client_server/r0.4.0.html#get-matrix-client-r0-user-userid-filter-filterid ?

[turt2live]

might need a new section for "peek these rooms please"

[turt2live]

Pass a leave/ban event for @:server down /sync. If the client is requesting format=federation this event would be unsigned/without hashes.

[Half-Shot]

Fundamentally, this feels like we are joining rooms without putting our membership event in them. We are specifying rooms that we would like to hear updates from, and listening on sync for them.

I have to admit I feel like filter is the wrong place for this to land, because this is adding things to the response and if anything it should end up as a seperate parameter on sync. I guess the advantage filter givess you is that you only need to specify your interest once, but I still feel it's the wrong place to do so.

My counter proposal would be to follow the normal semantics for subscribing to a room i.e /joining it but specifying a flag to say that it's merely a request to peek rather than attempt a full join as the user. The advantages are that you can largely drop this in place of existing client code, you don't need to maintain a filter object. If join feels like the wrong word, you could also just have a /peek endpoint that does precisely the same thing.

If all of that seems negative, sorry. I am over the moon that we are finally going to get a sensible way to peek :)

[Half-Shot]

This feels like implementing joins via filters, without actually joining. I can't help but feel this is really changing the whole meaning of filters, which (at least the name implies) filters a large set of data into a smaller set.

[Half-Shot]

I might be blind, but where would the room contents be seen in the sync response. In timeline? Would there be any meaningful way to determine if we are truly joined to them without knowing the filter?

[turt2live]

This MSC is tagged as obsolete, so closing.