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

Elements API redirect code will fail #96

Open
gravesm opened this issue Jun 22, 2018 · 2 comments
Open

Elements API redirect code will fail #96

gravesm opened this issue Jun 22, 2018 · 2 comments

Comments

@gravesm
Copy link

gravesm commented Jun 22, 2018

The code in question: https://github.com/MITLibraries/solenoid/blob/master/solenoid/elements/models.py#L112

requests by default will follow redirects, so in this case we should never see a 303 status code because the request will either succeed, raise a TooManyRedirects exception or fail for some other reason. If we expect Elements to send a 3xx response we will need to explicitly disable requests's default redirect behavior by adding allow_redirects=False to the request's arguments.

Note in this case, however, that https://github.com/MITLibraries/solenoid/blob/master/solenoid/elements/models.py#L90 would likely also fail, because the next request in the redirect chain will be a GET not a PATCH as this is standard behavior in HTTP.

@hakbailey
Copy link
Contributor

Per Elements documentation, "In normal operation, most responses will be success responses, in the HTTP 2xx range, or very occasionally a 303 (see other) redirect, which itself should return in the HTTP 2xx range." I can't find more explicit documentation than this.

@gravesm Do we even need to have our own follow_redirect method here, or should we simply allow requests to perform its default behavior and flag the record for follow-up?

@gravesm
Copy link
Author

gravesm commented Aug 30, 2018

Based on the documentation, I think we can just let requests do its thing, get rid of that custom redirect method and that's it. I think we should also add a response.raise_for_status() here, and probably elsewhere in the codebase where we are making requests. This would also mean looking further up the chain to see where these methods are called and thinking about how we want to handle exceptions. I'd suggest in the PR you make for this issue just fix the redirect thing and add the raise_for_status() call. Maybe create another issue to better address how exceptions are handled, if that makes sense?

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

No branches or pull requests

2 participants