Skip to content
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

AppServices: Why do events sent to AS's use "user_id", whereas events use "sender" #1269

Closed
anoadragon453 opened this issue May 30, 2018 · 11 comments · Fixed by #1552
Closed
Labels
application services wart A point where the protocol is inconsistent or inelegant

Comments

@anoadragon453
Copy link
Member

Should be addressed in an AppServices v2. Just means we have to rename that field for each event we're sending off :)

@turt2live
Copy link
Member

They do use sender already though? If not then no bridges would work, surely.

@anoadragon453
Copy link
Member Author

anoadragon453 commented May 30, 2018 via email

@turt2live
Copy link
Member

It looks like synapse actually sends both:

{
  "events": [
    {
      "age": 49803788,
      "content": {
        "avatar_url": null,
        "displayname": "_beep_user",
        "membership": "join"
      },
      "event_id": "$15270030010CYXhA:dev.t2bot.io",
      "membership": "join",
      "origin_server_ts": 1527003001647,
      "room_id": "!vfFxDRtZSSdspfTSEr:matrix.org",
      "sender": "@_beep_user:dev.t2bot.io",
      "state_key": "@_beep_user:dev.t2bot.io",
      "type": "m.room.member",
      "unsigned": {
        "age": 49803788
      },
      "user_id": "@_beep_user:dev.t2bot.io"
    }
  ]
}

Maybe this should be considered a spec bug?

@anoadragon453
Copy link
Member Author

anoadragon453 commented May 30, 2018 via email

@Half-Shot
Copy link
Contributor

(Would be good if someone could check Synapse to see if there isn't any subtle difference)

@richvdh
Copy link
Member

richvdh commented May 31, 2018

@Half-Shot: they are identical; the copy is done here: https://github.com/matrix-org/synapse/blob/master/synapse/events/utils.py#L197

This is an artifact of the fact that there was an older format for events (known as "v1" in some places) which used user_id here; that was of course unclear.

@richvdh richvdh added the wart A point where the protocol is inconsistent or inelegant label May 31, 2018
@anoadragon453
Copy link
Member Author

It would be really great to know which of the fields in @turt2live's example are mandatory and which are not as well.

@Half-Shot
Copy link
Contributor

@anoadragon453 What do you mean by mandatory, mandatory for whom?

@anoadragon453
Copy link
Member Author

The app service. But I suppose everything is mandatory if we've been sending it in the past already.

@Half-Shot
Copy link
Contributor

Half-Shot commented Jun 1, 2018

The app service. But I suppose everything is mandatory if we've been sending it in the past already.

Which one? There are many appservices. I can't remember any appservice poking at user_id, but from what I remember the node bridge/sdk don't really interact with the events at all anyway so it's not a problem from that perspective. All my bridges use sender so it's no problem my side. It would be worth deprecating it and seeing what breaks.

@anoadragon453
Copy link
Member Author

I think we'll just cut it in v2 app services with a prominent warning.

@turt2live turt2live added this to To do: appservices (prioritized) in August 2018 r0 Aug 14, 2018
turt2live added a commit to turt2live/matrix-doc that referenced this issue Aug 23, 2018
Fixes matrix-org#1269

This is also supposed to fix the 'age' problem, however that is a larger problem with the event schemas that is reserved for a future PR/commit.
Reference: matrix-org#1294
Reference: matrix-org#1524
@turt2live turt2live moved this from To do: appservices (prioritized) to In review in August 2018 r0 Aug 23, 2018
@turt2live turt2live moved this from In review to In review (just the issues) in August 2018 r0 Aug 24, 2018
August 2018 r0 automation moved this from In review (just the issues) to Done (this list will be incomplete) Aug 28, 2018
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this issue Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application services wart A point where the protocol is inconsistent or inelegant
Projects
No open projects
August 2018 r0
  
Done (this list will be incomplete)
Development

Successfully merging a pull request may close this issue.

4 participants