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

S2S v1/send_join: What is the integer in the response? #1514

Open
joepie91 opened this issue May 8, 2023 · 5 comments
Open

S2S v1/send_join: What is the integer in the response? #1514

joepie91 opened this issue May 8, 2023 · 5 comments
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@joepie91
Copy link

joepie91 commented May 8, 2023

Link to problem area: https://spec.matrix.org/v1.6/server-server-api/#put_matrixfederationv1send_joinroomideventid

Issue

Spec says:

200 response

Array of integer, Room State.

... however, only the Room State is specified, and it's not specified what the integer is exactly. Is it always 200, like in the example?

@joepie91 joepie91 added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label May 8, 2023
@richvdh
Copy link
Member

richvdh commented May 9, 2023

Is it always 200, like in the example?

yes. That was a bug in synapse, which got fixed in send_join v2.

@richvdh richvdh closed this as completed May 9, 2023
@joepie91
Copy link
Author

joepie91 commented May 9, 2023

I'm not sure why this is closed, the problem in the documentation still seems to exist? As in, it still shows this in the documentation without explaining what it means or what it should be set to in eg. a homeserver implementation.

@richvdh richvdh reopened this May 9, 2023
@richvdh
Copy link
Member

richvdh commented May 9, 2023

I closed it because I thought your question was answered.

Honestly I'm inclined to the feeling we should just remove v1/send_join rather than spend time clarifying its documentation. Having said that, a PR to clarify the documentation would be welcome.

@joepie91
Copy link
Author

I closed it because I thought your question was answered.

It is, but that won't fix the documentation for the next homeserver developer to come along :) Hence why I figured issues should remain open until the answer has made it into the documentation. (I think that's how clarification issues were always handled in the past? Unless that has changed)

Honestly I'm inclined to the feeling we should just remove v1/send_join rather than spend time clarifying its documentation.

I don't think that's possible in this case; because v2/send_join states that (older?) homeservers may reject a v2 join, and in that case you should revert to a v1 join - meaning that it is still required to be implemented for a functioning homeserver (on both ends) and therefore should still be in the spec. At least until it gets formally deprecated, but I imagine that that might cause backwards compatibility issues.

@richvdh
Copy link
Member

richvdh commented May 11, 2023

I closed it because I thought your question was answered.

It is, but that won't fix the documentation for the next homeserver developer to come along :) Hence why I figured issues should remain open until the answer has made it into the documentation. (I think that's how clarification issues were always handled in the past? Unless that has changed)

Yes, you're right, the issue should stay open.

Honestly I'm inclined to the feeling we should just remove v1/send_join rather than spend time clarifying its documentation.

I don't think that's possible in this case; because v2/send_join states that (older?) homeservers may reject a v2 join, and in that case you should revert to a v1 join - meaning that it is still required to be implemented for a functioning homeserver (on both ends) and therefore should still be in the spec. At least until it gets formally deprecated, but I imagine that that might cause backwards compatibility issues.

To be clear: it's already deprecated. As https://spec.matrix.org/v1.6/server-server-api/#put_matrixfederationv1send_joinroomideventid says: "Servers should instead prefer to use the v2 /send_join endpoint." I've opened #1518 to add a missing 'deprecated' label here.

As for removing it altogether: see #1519.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

2 participants