Skip to content
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

MSC1802: Remove the '200' value from some federation responses #1802

Merged
merged 9 commits into from Sep 10, 2019

Conversation

@babolivier
Copy link
Member

babolivier commented Jan 14, 2019

Rendered

Resolves #1418

@babolivier babolivier added the proposal label Jan 14, 2019
Copy link
Contributor

Half-Shot left a comment

This seems very sensible, and definitely something I'd like to see in r0.

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Jan 14, 2019

Cool! Though given we can do this in a backwards compatible way, we should probably do so. Basically, in the same way as #1794 we can fix this in v2 federation API, and at some point in the future deprecate the v1 API

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Jan 14, 2019

Basically, in the same way as #1794 we can fix this in v2 federation API, and at some point in the future deprecate the v1 API

I didn't think about that option. I guess that's indeed better. I'll rewrite my proposal to use it rather than the current proposed solution.

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Feb 14, 2019

ftr this propsal is ready for review (I hadn't added the in-review label for it as I'd expect someone from the specs core team to do it)

babolivier added 2 commits Apr 29, 2019
@babolivier babolivier requested a review from turt2live Apr 29, 2019
@babolivier babolivier requested a review from turt2live May 1, 2019
Copy link
Member

turt2live left a comment

seems reasonable, now it just needs more feedback from others :D

@Half-Shot

This comment has been minimized.

Copy link
Contributor

Half-Shot commented Jun 9, 2019

#2102 plans to make a change so that federation endpoints expect canonical JSON rather than JSON be sent along the wire, in the interest of keeping things light for the recipient. I don't want to bring the bikeshed to this proposal, but if we are going to add a new endpoint version then it might be worth merging that rule with this proposal.

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Jun 10, 2019

Thanks for the heads up @Half-Shot, I'll keep an eye on #2102 to see how it evolves (because it's not clear right now if it's going to end up in the spec).

@Half-Shot

This comment has been minimized.

Copy link
Contributor

Half-Shot commented Jul 26, 2019

Myself and @anoadragon453 came across the delight of /send* endpoints again today /s. Can we get a few more eyes on this?

@anoadragon453

This comment has been minimized.

Copy link
Member

anoadragon453 commented Jul 26, 2019

Yes please.

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Jul 26, 2019

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Jul 26, 2019

I am assuming my past self isn't an idiot, so:

@mscbot reviewed

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Aug 16, 2019

It appears that synapse doesn't return [200, {...}] for /send, so I think that may be more a spec bug.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Aug 16, 2019

yup let's drop /send here and fix it in the spec instead.

Also: can we change the title, which makes the MSC sound much scarier than it is. Just "drop spurious '200' value from federation responses"?

Otherwise lgtm and:

@mscbot reviewed

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Aug 16, 2019

Have filed #2236

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Sep 5, 2019

It appears that synapse doesn't return [200, {...}] for /send, so I think that may be more a spec bug.

@erikjohnston are we sure that other servers (e.g. Construct) don't implement this? If so, I think it mandates a v2 endpoint, otherwise I'm fine with treating it as a spec bug. The latter is more likely (since they possibly wouldn't be able to federate with Synapse in this case?), but we should make sure of that.

Co-Authored-By: Matthew Hodgson <matthew@matrix.org>
@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Sep 5, 2019

It appears that synapse doesn't return [200, {...}] for /send, so I think that may be more a spec bug.

@erikjohnston are we sure that other servers (e.g. Construct) don't implement this? If so, I think it mandates a v2 endpoint, otherwise I'm fine with treating it as a spec bug. The latter is more likely (since they possibly wouldn't be able to federate with Synapse in this case?), but we should make sure of that.

I very much doubt there would be servers in production that implemented federation without testing it against synapse, so I think it should be classified as a spec bug.

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Sep 5, 2019

Looking at Construct's code (which I think is the only HS implementation that I've seen used in rooms apart from Synapse), I can't see the [200, {...}] response format being used anywhere near /send so sgtm.

@babolivier babolivier changed the title MSC1802: Standardised federation response formats MSC1802: Remove the '200' value from some federation responses Sep 5, 2019
babolivier added 3 commits Sep 5, 2019
…github.com:matrix-org/matrix-doc into babolivier/standardised-federation-response-format
@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Sep 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@babolivier babolivier added this to In progress in Homeserver Task Board via automation Sep 6, 2019
@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Sep 10, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit c00fe02 into master Sep 10, 2019
8 checks passed
8 checks passed
buildkite/matrix-doc Build #830 passed (1 minute, 1 second)
Details
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
Homeserver Task Board automation moved this from In progress to Done Sep 10, 2019
@turt2live turt2live added this to Implementation stages in Client-server r0.6 proposals Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
9 participants
You can’t perform that action at this time.