New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIPish] MSC1776: Implementing peeking via /sync #1776

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@ara4n
Copy link
Member

ara4n commented Jan 6, 2019

Rendered

@ara4n ara4n changed the title MSC1776: Implementing peeking via /sync [WIPish] MSC1776: Implementing peeking via /sync Jan 6, 2019

@ara4n ara4n added the proposal label Jan 6, 2019

ara4n added some commits Jan 6, 2019

dep
@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 6, 2019

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

This comment has been minimized.

Copy link
Contributor

t3chguy commented Jan 6, 2019

How does one peek using /sync 😕?

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 6, 2019

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

This comment has been minimized.

Copy link
Member

ara4n commented Jan 6, 2019

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

This comment has been minimized.

Copy link
Member

ara4n commented Jan 6, 2019

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).

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 6, 2019

@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

This comment has been minimized.

Copy link
Member

jcgruenhage commented Jan 6, 2019

@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.

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 6, 2019

@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

This comment has been minimized.

Copy link
Member

jcgruenhage commented Jan 6, 2019

Oh right, that also exists. Good point.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Jan 6, 2019

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.

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 7, 2019

@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

This comment has been minimized.

Copy link
Member

ara4n commented Jan 7, 2019

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.

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Member

ara4n commented Jan 7, 2019

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.

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 7, 2019

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 comment has been minimized.

Copy link
Member

ara4n commented Jan 7, 2019

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.

@maxidor

This comment has been minimized.

Copy link
Contributor

maxidor commented Jan 7, 2019

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

This comment has been minimized.

Copy link
Member

ara4n commented Jan 7, 2019

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...

per-room, which is desirable for reducing the size of initial /syncs and making
them more granular, in order to speed up login.

Proposal: we add a POST form of /sync, which lets the user add/remove additional

This comment has been minimized.

This comment has been minimized.

@turt2live

turt2live Jan 7, 2019

Member

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


## Issues

How do we represent peek failures?

This comment has been minimized.

@turt2live

turt2live Jan 7, 2019

Member

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
Copy link
Contributor

Half-Shot left a comment

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 :)

them more granular, in order to speed up login.

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

This comment has been minimized.

@Half-Shot

Half-Shot Jan 7, 2019

Contributor

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.

In case of receiving large sync responses (initial or catchup sync), we request
them to be batched with no more than 10 rooms per response.

The sync response would include:

This comment has been minimized.

@Half-Shot

Half-Shot Jan 7, 2019

Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment