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

return 404 if room not found #3620

Merged
merged 2 commits into from
Aug 1, 2018

Conversation

fuzzmz
Copy link
Contributor

@fuzzmz fuzzmz commented Jul 27, 2018

Fixes #2952

Couldn't find any tests related to this though.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

1 similar comment
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@richvdh
Copy link
Member

richvdh commented Jul 31, 2018

@fuzzmz : please could you add a note for the changelog as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#changelog ?

@fuzzmz
Copy link
Contributor Author

fuzzmz commented Jul 31, 2018

@richvdh done.

@krombel
Copy link
Contributor

krombel commented Jul 31, 2018

@fuzzmz You need to Sign-off as well. Look at the same document @richvdh linked already

@fuzzmz fuzzmz force-pushed the return-404-room-not-found branch from db65a15 to 60a68df Compare July 31, 2018 13:31
@fuzzmz
Copy link
Contributor Author

fuzzmz commented Jul 31, 2018

Sorry for making this harder than it should be.

Rebased on the latest version of develop, squashed all the changes and signed-off.


Serban Constantin <serban.constantin at gmail dot com>
* Small bug fix
Copy link
Contributor

Choose a reason for hiding this comment

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

That file is in general for people who contributed a feature. I would say, that your "Small bug fix" does not count.
If it would it should contain all contributors.

Copy link
Member

Choose a reason for hiding this comment

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

as the CONTRIBUTING.rst file says, anyone making a PR is welcome to add themselves, for any contribution. It's not the size that that matters...

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. Then nvm

@@ -159,7 +159,7 @@ def __init__(self, hs):
def on_GET(self, request, room_id):
room = yield self.store.get_room(room_id)
if room is None:
raise SynapseError(400, "Unknown room")
raise SynapseError(404, "Unknown room")
Copy link
Member

Choose a reason for hiding this comment

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

ok this looks good to me, but the error code will still be M_UNKNOWN. Really it ought to be M_NOT_FOUND as per https://matrix.org/docs/spec/client_server/unstable.html#get-matrix-client-r0-directory-list-room-roomid.

I think that there's a NotFoundError or something you can raise instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this in the latest commit to raise NotFoundError instead of the generic one.

@richvdh
Copy link
Member

richvdh commented Jul 31, 2018

Couldn't find any tests related to this though.

The fact that there aren't any tests is no doubt how the bug happened in the first place. I've raised matrix-org/sytest#476 :/

@richvdh
Copy link
Member

richvdh commented Jul 31, 2018

@matrixbot test this please

@richvdh
Copy link
Member

richvdh commented Jul 31, 2018

now you need to isort -rc synapse :/

Per the Client-Server API[0] we should return
`M_NOT_FOUND` if the room isn't found instead
of generic SynapseError.

This ensures that /directory/list API returns
404 for room not found instead of 400.

[0]: https://matrix.org/docs/spec/client_server/unstable.html#get-matrix-client-r0-directory-list-room-roomid

Signed-off-by: Serban Constantin <[email protected]>
@fuzzmz fuzzmz force-pushed the return-404-room-not-found branch from edc737c to 70af98e Compare July 31, 2018 18:49
@richvdh
Copy link
Member

richvdh commented Aug 1, 2018

@matrixbot: test this please

@richvdh richvdh merged commit b8d7d39 into matrix-org:develop Aug 1, 2018
@fuzzmz fuzzmz deleted the return-404-room-not-found branch March 23, 2021 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants