Responses schema fixes

[afranke]

Fixes #2237.

Corrects the response schemas for:

PUT /user/{user_id}/account_data/{account_dataType}
PUT /user/{user_id}/rooms/{roomId}/account_data/{type}
PUT /directory/list/room/{roomId}
PUT /sendToDevice/{eventType}/{txnId}
POST /account/3pid
POST /account/3pid/add
POST /account/3pid/bind

Preview: https://pr3650--matrix-org-previews.netlify.app

[turt2live]

It looks like some of these are introducing response bodies rather than clarifying them - evidence to support that synapse has always done this would be appreciated, otherwise an MSC is likely required.

[afranke]

I have now checked in Synapse and can confirm that all these responses have a return 200, {}.

PUT /user/{user_id}/account_data/{account_dataType}
PUT /user/{user_id}/rooms/{roomId}/account_data/{type}
PUT /room_keys/version/{version}
PUT /directory/list/room/{roomId}
PUT /sendToDevice/{eventType}/{txnId}

Is that enough for you?

[turt2live]

for the endpoints we mostly just need evidence that it's always returned these things prior to 1.0 (roughly).

Adjusted links to account for time:
PUT /user/{user_id}/account_data/{account_dataType}
PUT /user/{user_id}/rooms/{roomId}/account_data/{type}
PUT /room_keys/version/{version} (intent carried from unstable implementation)
PUT /directory/list/room/{roomId}
PUT /sendToDevice/{eventType}/{txnId}

concern addressed

[richvdh]

for the endpoints we mostly just need evidence that it's always returned these things prior to 1.0 (roughly).

Adjusted links to account for time:
PUT /user/{user_id}/account_data/{account_dataType}
PUT /user/{user_id}/rooms/{roomId}/account_data/{type}
PUT /room_keys/version/{version} (intent carried from unstable implementation)

The link here is for the DELETE handler, though much the same applies to the PUT handler (added in synapse#4580. That PR formed the basis for MSC1219, which explicitly says that this endpoint "returns the empty JSON object".

PUT /directory/list/room/{roomId}
PUT /sendToDevice/{eventType}/{txnId}

Not mentioned here are:

• POST /account/3pid: very old
• POST /account/3pid/add, POST /account/3pid/bind. Both added by MSC2290, which doesn't seem to say what format the response should be. I think it's fair to say this is an error in Spec MSC2290: 3PID binding split #2328 which added these endpoints.

[richvdh]

this is fine except... it doesn't seem to have worked? None of the modified endpoints (with the exception of /account/3pid, which has a non-empty body) show a response body in the preview.

[richvdh]

also, you might want to merge in main, to pick up 97a8b0b, to fix the CI

[afranke]

this is fine except... it doesn't seem to have worked? None of the modified endpoints (with the exception of /account/3pid, which has a non-empty body) show a response body in the preview.

As I said in the review-conversation above, it does work: the artifact contains the expected bits and this can be verified by opening the file with a viewer.

also, you might want to merge in main, to pick up 97a8b0b, to fix the CI

This goes beyond my understanding of Github Pull Requests and Reviews, in the sense that I don’t know how to do that in a way that I am certain will not break something for someone here. With Gitlab, I know that I could just rebase the branch on top of main and everything would be fine for everyone, but I know doing this with Github makes everyone unhappy.

Since we know for a fact the red thing in CI is fixed in main, has nothing to do with the changes here, and this PR will end up “Squashed and merged” anyway, do we really need to worry about this?

[richvdh]

do we really need to worry about this?

no, but it's easier just to do it than verify the cause of the CI failure:

git fetch
git merge origin/main
git push

and yes, please don't rebase!

[afranke]

It looks like some of these are introducing response bodies rather than clarifying them - evidence to support that synapse has always done this would be appreciated, otherwise an MSC is likely required.

Considering this has been confirmed to be synapse’s behaviour, is clarification indeed the type I need to add for the changelog?

[richvdh]

Considering this has been confirmed to be synapse’s behaviour, is clarification indeed the type I need to add for the changelog?

Yes I think so. Thanks!

[richvdh]

For the record, I think this ends up being pretty much a no-op. An object with no properties is the same as one with an empty properties, and the auto-generated example looks the same as the one we are creating here.

[afranke]

Maybe, but that wouldn’t be consistent with what we have for other cases. Is there anything to change here or can we mark this resolved?

[richvdh]

nope, just calling it out explicitly for future reference. I'd leave the thread open for future viewers.

[richvdh]

(have updated the PR description to make it clear which endpoints we're fixing here)

[richvdh]

LGTM other than the changelog.