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

Add general clarity to the /createRoom endpoint #1518

Merged
merged 7 commits into from Aug 21, 2018

Conversation

2 participants
@turt2live
Member

turt2live commented Aug 15, 2018

Rendered: see 'docs' status check


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:

Fixes #1243
Fixes #1471
Inspired by #1213
Fixes #681

turt2live added some commits Aug 15, 2018

Add general clarity to the /createRoom endpoint
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 #1243
Fixes #1471
Inspired by #1213

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 15, 2018

@turt2live turt2live added this to In review in August 2018 r0 via automation Aug 15, 2018

@richvdh

This endpoint might get a prize for being the most byzantine API in the whole of matrix, and there is some competition :/

@@ -196,10 +223,17 @@ paths:
type: string
description: |-
The created room's ID.
room_alias:

This comment has been minimized.

@richvdh

richvdh Aug 20, 2018

Member

it sounds like we need to reach an agreement over what to do with this. I can't entirely remember why I was in favour of removing it, but either way I'd like us to discuss it before committing to it.

This comment has been minimized.

@turt2live

turt2live Aug 20, 2018

Member

#1243 makes it sound like your opinion was that we aren't this helpful in other endpoints.

@@ -89,6 +96,11 @@ paths:
``private`` visibility if this key is not included. NB: This
should not be confused with ``join_rules`` which also uses the
word ``public``.
If no ``preset`` is specificed, the server may use the visbility

This comment has been minimized.

@richvdh

richvdh Aug 20, 2018

Member

I think this might be clearer specified against the preset property rather than here.

This comment has been minimized.

@richvdh

richvdh Aug 20, 2018

Member

Also, a "may" specification here is kinda useless imho; the only reason for speccing it would be if the client could rely on it happening. Assuming we don't want to deprecate this behaviour, this should be at least a "should".

This comment has been minimized.

@turt2live

turt2live Aug 20, 2018

Member

will go with 'should' and move it to the preset. This is one of those cases where I was looking at synapse and ran across the if statement that does this and thought it was worth including.

@@ -145,6 +160,14 @@ paths:
The server will clobber the following keys: ``creator``. Future
versions of the specification may allow the server to clobber
other keys.
properties:

This comment has been minimized.

@richvdh

richvdh Aug 20, 2018

Member

speccing this fails to make it clear that m.federate is just one example of a property that can be set here. It might be better just to say "extra keys... such as m.federate" and link to the m.room.create event schema spec.

event. This object is applied on top of the generated power
level event prior to it being sent to the room. Defaults
to overriding nothing.
$ref: "definitions/event-schemas/power_level_content_schema.yaml"

This comment has been minimized.

@richvdh

richvdh Aug 20, 2018

Member

Can we just link to the m.room.power_levels event rather than duplicating the whole content in the spec - not least because anyone implementing this endpoint needs to inspect the two sections carefully to make sure that they actually are the same.

This comment has been minimized.

@turt2live

turt2live Aug 20, 2018

Member

tbh I'm not a huge fan of linking between different sections, as it leads to infinite new tabs when trying to implement something. Would a link + keeping the $ref be okay?

This comment has been minimized.

@turt2live

turt2live Aug 20, 2018

Member

actually it kinda makes more sense to put it as a link only, similar to m.federate. Will do that instead.

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 20, 2018

I have removed room_alias from the PR for now. I'll leave this open for a day or so (assuming approved) to give people a chance at disagreeing strongly with not having a room_alias response field.

@turt2live turt2live requested a review from richvdh Aug 20, 2018

@richvdh

This comment has been minimized.

Member

richvdh commented Aug 21, 2018

hum, why is there no docs status ? :'(

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 21, 2018

Good question. The docs are here though: https://515-24998719-gh.circle-artifacts.com/0/root/project/scripts/gen/index.html (as of writing this comment)

August 2018 r0 automation moved this from In review to Reviewer approved Aug 21, 2018

@richvdh

an example for power_levels_content wouldn't have gone amiss, but lgtm

@turt2live turt2live merged commit e4f5c3d into matrix-org:master Aug 21, 2018

3 checks passed

ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 21, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/create-room-improvements branch Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment