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

Improve language around handling redirects #152

Open
garethsb opened this issue Apr 19, 2022 · 3 comments
Open

Improve language around handling redirects #152

garethsb opened this issue Apr 19, 2022 · 3 comments

Comments

@garethsb
Copy link
Contributor

Adding this as an issue here but with the expectation that https://github.com/orgs/AMWA-TV/teams/nmos-architecture-review will move it somewhere more general.

IS-04 and IS-05 say things like:

For Controllers performing GET and HEAD requests, using these methods SHOULD correctly handle a 301 (Moved Permanently) response.

When a 301 is supported, the Controller MUST follow the redirect in order to retrieve the required response payload.

E.g. at https://github.com/AMWA-TV/is-05/blob/v1.1.x/docs/APIs.md#get-and-head-requests

This seems like strange conformance language: "SHOULD handle a 301 [and if it does] MUST follow the redirect".

At least automated test cases seem possible...

@garethsb
Copy link
Contributor Author

Summarizing @AMWA-TV/nmos-architecture-review Slack thread:

I'm slightly wary of putting "automatically follow" since best practice is probably not to follow HTTPS -> HTTP, to give up if you get stuck in a loop, etc. So maybe the simplest thing is just to delete the second sentence with the MUST and leave it as "correctly handle"? E.g. see the Remarks section in HttpClientHandler.AllowAutoRedirect Property (System.Net.Http)

But what does "correctly handle" imply? That your application doesn't explode?

The obvious spec reference would be RFC7231
But that only says "The user agent MAY use the Location field value for automatic redirection"

@peterbrightwell
Copy link
Contributor

peterbrightwell commented Jun 23, 2023

Architecture Review Group review: place on backlog

@cristian-recoseanu
Copy link
Contributor

Is the change as simple as merging the two sentences:

Clients performing requests using these methods SHOULD correctly handle a 301 (Moved Permanently) response by following the redirect in order to retrieve the response payload.

This gets rid of the second MUST which was conditional and weakened by the preceeding SHOULD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants