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

Add 206, 303, 501 return codes for /sequence/{id} in refget-openapi.yaml.. #330

Merged
merged 2 commits into from
Aug 15, 2018

Conversation

raskoleinonen
Copy link
Contributor

Suggestion to add 206, 303, 501 return codes for /sequence/{id} defined in the specs.

@raskoleinonen raskoleinonen changed the title Update refget-openapi.yaml Add 206, 303, 501 return codes for /sequence/{id}. Aug 15, 2018
@raskoleinonen raskoleinonen changed the title Add 206, 303, 501 return codes for /sequence/{id}. Add 206, 303, 501 return codes for /sequence/{id} in refget-openapi.yaml.. Aug 15, 2018
@andrewyatz
Copy link
Contributor

Same again :). Looks great just need a more descriptive commit message and it'll go in

@jmarshall
Copy link
Member

It would be nice to keep the list of codes in numerical order. Also probably wants to be 302/303/307 rather than just 303.

@raskoleinonen
Copy link
Contributor Author

It is not straightforward to modify the commit message (https://help.github.com/articles/changing-a-commit-message/). Might be easier if you reject this pull request and I'll just create a new one.

I can fix the numerical order at the same time.

About 302/303/307.

Specs says in 'Protocol essentials': 302, 303 and 307 are all valid response codes to use.
And later in 'Method: get sequence by ID': '... or 303 redirecting the client to where it can retrieve the sequence'
without mentioning 302 or 307.

@jmarshall
Copy link
Member

jmarshall commented Aug 15, 2018

And later in 'Method: get sequence by ID': '... or 303 redirecting the client to where it can retrieve the sequence' without mentioning 302 or 307.

303 was relaxed in Protocol essentials to 3XX(302/303/307) in a late commit to PR #327; the text in the Method paragraph should say 3XX rather than 303, but I guess it wasn't updated at the same time. (See Edits from PR to finalise changes - Changed 303 redirect to 3XX and flagged 302, 303 and 307 as all valid redirects near the bottom of 900b9e4.)

(It is pretty straightforward to update a PR consisting of just one commit: git commit --amend which you'd be doing anyway; then git push -f rasko patch-1 (or whatever your remote is called). Just adds a -f compared to adding another commit to the PR. Or just push a second commit to the PR and I'm sure @andrewyatz can tidy them up when merging it 😄)

@andrewyatz
Copy link
Contributor

Aye can you add the other missing 30X codes (302 and 307).

@raskoleinonen
Copy link
Contributor Author

Done and ordered return codes in numerical order.

While checking 30X return codes for my own interest I stumbled also on:
301 Moved Permanently

@andrewyatz
Copy link
Contributor

Yes. 301 would be a pretty bad thing to send through on this API unless we were sure that URL location would never ever move. We specifically think 301 is possibly harmful to the API though someone may provide a 301 to indicate the server location has permanently moved. But not for this trick of redirecting a client to sequence.

@andrewyatz andrewyatz merged commit ecf37f8 into samtools:master Aug 15, 2018
@andrewyatz
Copy link
Contributor

Merged. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants