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 #14884

Closed
wants to merge 4 commits into from

Conversation

thezaidbintariq
Copy link
Contributor

@thezaidbintariq thezaidbintariq commented Jan 21, 2023

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Fixes #10321

@thezaidbintariq thezaidbintariq requested a review from a team as a code owner January 21, 2023 11:16
@clokep clokep changed the title Solved Issue #10321. Contributed by @thezaidbintariq Remove undocumented room_alias key from /createRoom response Jan 21, 2023
@clokep
Copy link
Member

clokep commented Jan 21, 2023

@thezaidbintariq Could you please sign-off on your contribution?

Comment on lines 1 to 4
Removed if room_alias:
result["room_alias"] = room_alias.to_string()
from synapse/synapse/handlers/room.py
Contributed by @thezaidbintariq.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Removed if room_alias:
result["room_alias"] = room_alias.to_string()
from synapse/synapse/handlers/room.py
Contributed by @thezaidbintariq.
Remove undocumented `room_alias` key from `/createRoom` response. Contributed by @thezaidbintariq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Zaid Bin Tariq [email protected]

@thezaidbintariq
Copy link
Contributor Author

Signed-off-by: Zaid Bin Tariq [email protected]

@thezaidbintariq
Copy link
Contributor Author

@clokep did I sign-off correctly?

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

@thezaidbintariq Your sign-off is correct 👍

It would be good for someone to shout in the Matrix Client Developers Room about this change before including it in a release of Synapse - just to ensure that no client desperately relies on this (unspecced) field.

changelog.d/10321.removal Outdated Show resolved Hide resolved
@thezaidbintariq
Copy link
Contributor Author

@anoadragon453 can you merge the PR?

@clokep clokep requested a review from a team January 21, 2023 19:27
@clokep
Copy link
Member

clokep commented Jan 23, 2023

It would be good for someone to shout in the Matrix Client Developers Room about this change before including it in a release of Synapse - just to ensure that no client desperately relies on this (unspecced) field.

This was done at https://matrix.to/#/!XXSJTvRPInupfUgQVb:matrix.org/$xxmBTJfTuP2icgn0X2xfDsygfUR3JbsFYQAMBE1eMPs?via=pixie.town&via=matrix.org&via=element.io

@anoadragon453
Copy link
Member

It would be good for someone to shout in the Matrix Client Developers Room about this change before including it in a release of Synapse - just to ensure that no client desperately relies on this (unspecced) field.

This was done at matrix.to/#/!XXSJTvRPInupfUgQVb:matrix.org/$xxmBTJfTuP2icgn0X2xfDsygfUR3JbsFYQAMBE1eMPs?via=pixie.town&via=matrix.org&via=element.io

I bumped the question as nobody had responded yet.

@clokep
Copy link
Member

clokep commented Jan 25, 2023

This is failing due to complement requiring the room_alias field, it lokos like this was removed from matrix-org/sytest#1216, but never ported to complement, see https://github.com/matrix-org/complement/blob/4418d2c05009cb0754403a9a8565b8b701633a5b/tests/csapi/apidoc_room_create_test.go#L39

@clokep clokep removed the request for review from a team January 25, 2023 12:14
DMRobertson pushed a commit to matrix-org/complement that referenced this pull request Feb 16, 2023
@DMRobertson
Copy link
Contributor

This is failing due to complement requiring the room_alias field, it lokos like this was removed from matrix-org/sytest#1216, but never ported to complement, see https://github.com/matrix-org/complement/blob/4418d2c05009cb0754403a9a8565b8b701633a5b/tests/csapi/apidoc_room_create_test.go#L39

I have opened matrix-org/complement#616 for this.

@DMRobertson
Copy link
Contributor

It looks like this makes a backwards-incompatible change to the module API too:

return room_id_and_alias["room_id"], room_id_and_alias.get("room_alias", None)

Will need to

  • update the tests
  • change the module API to be backwards compatible (probably not too bad), or else announcing a breaking change

@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Feb 16, 2023
@DMRobertson
Copy link
Contributor

(I am happy to take this over, to get this across the line)

DMRobertson pushed a commit to matrix-org/complement that referenced this pull request Feb 17, 2023
@DMRobertson
Copy link
Contributor

Complement should now pass now that matrix-org/complement#616 has landed

@DMRobertson
Copy link
Contributor

Hmm. On reflection it looks like there's a bit more internal plumbing to be done here than we'd anticipated. I'm going to have a go at this in #15093.

@DMRobertson
Copy link
Contributor

Hmm. On reflection it looks like there's a bit more internal plumbing to be done here than we'd anticipated. I'm going to have a go at this in #15093.

This has just merged. Thank you for your efforts looking into this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove undocumented room_alias key from /createRoom response
4 participants