Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove "user_id" from GET /presence #7606

Merged
merged 4 commits into from
Jun 11, 2020
Merged

Remove "user_id" from GET /presence #7606

merged 4 commits into from
Jun 11, 2020

Conversation

Half-Shot
Copy link
Collaborator

@Half-Shot Half-Shot commented May 30, 2020 •
edited
Loading

https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-presence-userid-status doesn't include a user_id, and given the request URL includes the user_id I suspect there is limited value in diverging from the spec.

@Half-Shot Half-Shot changed the title Ensure GET .../presence conforms with spec Ensure GET .../presence conforms to spec May 30, 2020
@Half-Shot Half-Shot changed the title Ensure GET .../presence conforms to spec Remove "user_id" from GET /presence May 31, 2020
@erikjohnston
Copy link
Member

Hmm. We'd need to make sure clients out there aren't accidentally relying on this

@Half-Shot
Copy link
Collaborator Author

Half-Shot commented Jun 1, 2020 •
edited
Loading

Clients checked:

  • matrix-js-sdk doesn't implement GET /presence
  • matrix-js-bot-sdk doesn't support it either.
  • matrix-nio optionally supports user_id.
    • Update: They added support for this 3 days ago, I've asked them to reconsider.
  • matrix-ios-sdk doesn't use the user_id key
  • matrix-android-sdk support is unclear, unable to find the code which deserializes the response

@richvdh says that riot-android does call it, but user_id wasn't always returned. matrix.org doesn't respond to this endpoint so I'm feeling confident that this client won't be affected either.

@anoadragon453
Copy link
Member

matrix-android-sdk support is unclear, unable to find the code which deserializes the response

Could you test with your phone?

@Half-Shot Half-Shot mentioned this pull request Jun 1, 2020
Merged
@Half-Shot
Copy link
Collaborator Author

matrix-android-sdk support is unclear, unable to find the code which deserializes the response

Could you test with your phone?

Sure, but I don't know how you would cause Riot to do a GET /presence request to be honest.

@devNan0 devNan0 mentioned this pull request Jun 1, 2020
Merged
@richvdh richvdh mentioned this pull request Jun 1, 2020
Closed
@clokep clokep added the A-Spec-Compliance places where synapse does not conform to the spec label Jun 1, 2020
@clokep clokep requested a review from a team June 11, 2020 16:41
@clokep
Copy link
Member

clokep commented Jun 11, 2020

It looks like this is ready for review, but not 100% sure all the above questions were answered. @Half-Shot is this ready from your POV?

@Half-Shot
Copy link
Collaborator Author

Half-Shot commented Jun 11, 2020 •
edited
Loading

@Half-Shot is this ready from your POV?

Ready from my PoV. I'm happy that the majority of clients either do not support presence, or do not use the extra field from this endpoint.

@deepbluev7
Copy link
Contributor

I'm happy that the majority of clients either do not support presence

That's mostly because presence currently isn't that polished in synapse and receiving presence via /sync is also enough in many cases (where you can't limit the events received anyway, if that issue is still open).

Anyway, I can confirm that Nheko also doesn't use or even support the user_id in /presence in the unreleased support for presence.

clokep
clokep reviewed Jun 11, 2020
View reviewed changes
changelog.d/7606.bugfix Outdated Show resolved Hide resolved
clokep
clokep approved these changes Jun 11, 2020
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me.

I think the backwards compatibility concerns are pretty minimal given the investigation that was done.

@clokep clokep merged commit a3fbc23 into develop Jun 11, 2020
@clokep clokep deleted the hs/presence-get-conform branch June 11, 2020 18:13
@joepie91 joepie91 mentioned this pull request Jul 21, 2020
Open
This was referenced Sep 22, 2020
Merged
Merged
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit '03619324f' into dinsic
70032b3 
* commit '03619324f':
  Create a ListenerConfig object (#7681)
  Fix changelog wording
  1.15.1
  Wrap register_device coroutine in an ensureDeferred (#7684)
  Ensure the body is a string before comparing push rules. (#7701)
  Ensure etag is a string for GET room_keys/version response (#7691)
  Update m.id.phone to use 'phone' instead of 'number' (#7687)
  Fix "There was no active span when trying to log." error (#7698)
  Enable 3PID add/bind/unbind endpoints on r0 routes
  Discard RDATA from already seen positions. (#7648)
  Replace iteritems/itervalues/iterkeys with native versions. (#7692)
  Fix warnings about losing log context during UI auth. (#7688)
  Fix a typo when comparing the URI & method during UI Auth. (#7689)
  Remove "user_id" from GET /presence. (#7606)
  Increase the default SAML session expirary time to 15 minutes. (#7664)
  fix typo in sample_config.yaml (#7652)
  Take out a lock before modifying _CACHES (#7663)
  Add option to enable encryption by default for new rooms (#7639)
  Clean-up the fallback login code. (#7657)
babolivier added a commit that referenced this pull request Sep 1, 2021
@babolivier
Revert "Remove "user_id" from GET /presence. (#7606)"
5fbc036 
This reverts commit a3fbc23.
babolivier added a commit that referenced this pull request Sep 1, 2021
@babolivier
Merge pull request #56 from matrix-org/babolivier/dinsic_presence_use…
cdc67df 
…r_id

Revert "Remove "user_id" from GET /presence. (#7606)"
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees
No one assigned
Labels
A-Spec-Compliance places where synapse does not conform to the spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Half-Shot @erikjohnston @anoadragon453 @clokep @deepbluev7