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

Remove undocumented room_alias key from /createRoom response #10321

Closed
richvdh opened this issue May 16, 2018 · 14 comments · Fixed by #15093
Closed

Remove undocumented room_alias key from /createRoom response #10321

richvdh opened this issue May 16, 2018 · 14 comments · Fixed by #15093
Labels
A-Spec-Compliance places where synapse does not conform to the spec good first issue Good for newcomers O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@richvdh
Copy link
Member

richvdh commented May 16, 2018

we should either spec this or remove it.

Presumably it exists to save clients the bother of having to construct their own fully-qualified aliases, but given we don't do the same for other endpoints which create aliases, I'm not convinced it is worthwhile.

I'd therefore be in favour of removing it, but it's worth noting that sytest currently uses it in a non-trivial number of places
@turt2live
Copy link
Member

I've labelled this as an omission, although I'm not convinced that's the right direction for this particular issue.

turt2live referenced this issue in turt2live/matrix-doc Aug 15, 2018
@turt2live
Add general clarity to the /createRoom endpoint
45c68e3 
This commit does a number of things:
* Minor formatting/alignment changes
* Document the room_alias response key. This could be deprecated now, or forfeited, if needed.
* Remove the guest_can_join parameter - it is not actually supported
* Document the previously undocumented power_level_content_override parameter
* Clarify that the room_id is required on the response
* More clearly spell out which events are created as part of the request
* Clarify how the room alias becomes the canonical alias
* Clarify how the `visibility` may be used to determine a default preset to apply
* Document the `m.federate` creation content parameter, adding an option for the homeserver to define a default value

References:
* Preset being inferred by the visibility: https://github.com/matrix-org/synapse/blob/cd32c19a604549b4518d748c07149d140bc9e063/synapse/handlers/room.py#L172-L177
* Power level content overrides:
  * https://github.com/matrix-org/synapse/blob/master/synapse/handlers/room.py#L198
  * https://github.com/matrix-org/synapse/blob/master/synapse/handlers/room.py#L335-L359
* Aliases becoming canonical: https://github.com/matrix-org/synapse/blob/master/synapse/handlers/room.py#L366-L370
* `m.federate` landing in the create event: https://github.com/matrix-org/synapse/blob/master/synapse/handlers/room.py#L311-L315

Fixes https://github.com/matrix-org/matrix-doc/issues/1243
Fixes matrix-org#1471
Inspired by matrix-org#1213
@richvdh
Copy link
Member Author

richvdh commented Jul 6, 2021

synapse still returns this unspecced field.

@richvdh richvdh reopened this Jul 6, 2021
@richvdh richvdh transferred this issue from matrix-org/matrix-spec-proposals Jul 6, 2021
@turt2live
Copy link
Member

The referenced PR has a bit of a troubled history on this. What are you seeing as an end goal for this issue?

@richvdh
Copy link
Member Author

richvdh commented Jul 6, 2021

IMHO we should stop synapse returning the unspecced field.

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Jul 6, 2021 •
edited
Loading

Will that break clients which probably depend on this behaviour? Do such clients exist?

@richvdh
Copy link
Member Author

richvdh commented Jul 6, 2021

Will that break clients which probably depend on this behaviour?

yes, of course.

Do such clients exist?

well, that is the $1M question...

@turt2live
Copy link
Member

IMHO we should stop synapse returning the unspecced field.

OH. Github mobile said this issue got transferred from synapse to matrix-doc, but on web it's clearly the other way around. This makes sense.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. A-Spec-Compliance places where synapse does not conform to the spec labels Jul 12, 2021
@gnieto gnieto mentioned this issue Mar 22, 2022
Merged
@DMRobertson DMRobertson mentioned this issue Aug 25, 2022
Closed
@DMRobertson DMRobertson added the O-Uncommon Most users are unlikely to come across this or unexpected workflow label Aug 25, 2022
@DMRobertson
Copy link
Contributor

DMRobertson commented Aug 25, 2022 •
edited
Loading

I have asked the client developers' room here if this is known to be in use.

@DMRobertson DMRobertson added X-Consult-Clients Need to investigate if this change breaks clients Z-Cleanup Things we want to get rid of, but aren't actively causing pain labels Aug 25, 2022
@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Aug 25, 2022 •
edited
Loading

ruma also doesn't acknowledge this key, dropping it silently.

@bmarty
Copy link

bmarty commented Sep 5, 2022 •
edited
Loading

@DMRobertson I cannot edit your message but for Element Android (actually Android SDK), this is only using roomId

As iOS, we only use the roomId.

@DMRobertson
Copy link
Contributor

No known clients seem to use this field. I think we should remove it.

@DMRobertson DMRobertson changed the title synapse's /createRoom returns an undocumented room_alias key Remove undocumented room_alias key from /createRoom response Sep 5, 2022
@DMRobertson DMRobertson added the good first issue Good for newcomers label Sep 5, 2022
@DMRobertson
Copy link
Contributor

To fix this:

  • Remove these lines

synapse/synapse/handlers/room.py

Lines 1019 to 1021 in 84ddcd7

if room_alias:
result["room_alias"] = room_alias.to_string()

  • Add a changelog entry ending in .removal
  • Fixup any test failures

@DMRobertson DMRobertson removed the X-Consult-Clients Need to investigate if this change breaks clients label Sep 5, 2022
thezaidbintariq added a commit to thezaidbintariq/synapse that referenced this issue Jan 21, 2023
@thezaidbintariq
Solved issue matrix-org#10321
108e734
@clokep clokep mentioned this issue Jan 21, 2023
Closed
4 tasks
@thezaidbintariq
Copy link
Contributor

@clokep can you merge the PR?

@clokep
Copy link
Member

clokep commented Jan 21, 2023

@clokep can you merge the PR?

It is in the review queue and will get reviewed by a member of the team.

@DMRobertson DMRobertson mentioned this issue Feb 17, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
A-Spec-Compliance places where synapse does not conform to the spec good first issue Good for newcomers O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Milestone
No milestone
7 participants
@clokep @turt2live @richvdh @bmarty @DMRobertson @ShadowJonathan @thezaidbintariq