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

Fix event schemas for /sync #1573

Merged
merged 7 commits into from Aug 31, 2018

Conversation

3 participants
@turt2live
Member

turt2live commented Aug 27, 2018

Rendered: see 'docs' status check


This commit clarifies the required keys for each type of event that appears in sync, fixes the core event schema not declaring 'content' as required, and includes a mention that events may not have a room_id when appearing in /sync.

Fixes #595
Fixes #909

Fix event schemas for /sync
This commit clarifies the required keys for each type of event that appears in sync, fixes the core event schema not declaring 'content' as required, and includes a mention that events may not have a room_id when appearing in /sync.

Fixes #595
Fixes #909

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

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 27, 2018

description: The globally unique event identifier.
type: string
sender:
description: Contains the fully-qualified ID of the user who *sent*

This comment has been minimized.

@ara4n

ara4n Aug 28, 2018

Member

why the asterisks? i found myself distracted and wondering what the subtlety was.

This comment has been minimized.

@turt2live

turt2live Aug 28, 2018

Member

This is copy/pasted from the room_event.yaml - happy to remove them though.

state_key:
description: A unique key which defines the overwriting semantics for this piece
of room state. This value is often a zero-length string. The presence of this
key makes this event a State Event. The key MUST NOT start with '_'.

This comment has been minimized.

@ara4n

ara4n Aug 28, 2018

Member

hm, why is _ special?

This comment has been minimized.

@turt2live

turt2live Aug 28, 2018

Member

also copy-pasted from it's parent file. It's not special as far as I know - will remove it.

@@ -10,5 +10,6 @@ properties:
type: string
required:
- type
- content

This comment has been minimized.

@ara4n

ara4n Aug 28, 2018

Member

is content really required? i think we consciously made it optional on the basis that you could have an event type that just had no content beyond its type (although I agree it's a bit stupid, and I don't have a problem with mandating it, so long as we've vaguely understood why it was optional before)

This comment has been minimized.

@turt2live

turt2live Aug 28, 2018

Member

I can't think of an event that has no content, and this doesn't require the object to actually contain anything. I honestly thought it was a mistake that it wasn't included and expect that client authors (for instance) would assume that the field exists, at their own detriment.

This comment has been minimized.

@richvdh

richvdh Aug 31, 2018

Member

We do enforce its presence when submitting messages over the C-S API in synapse. +1 to requiring its presence here.

Take out the underscore restriction from state events
It's not needed anymore, and we should remove it while we're in the area.

Includes other misc changes to the schema layout.

@turt2live turt2live requested a review from ara4n Aug 28, 2018

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 31, 2018

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

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 31, 2018

@richvdh please take a quick look at the recent changes - had to make some corrections to ensure the project builds.

@turt2live turt2live moved this from Reviewer approved to In review (just the PRs) in August 2018 r0 Aug 31, 2018

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 31, 2018

@turt2live turt2live merged commit 2577898 into matrix-org:master Aug 31, 2018

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
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 31, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/sync-event-fields branch Aug 31, 2018

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