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

Only follow a limited number of redirects. #651

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Conversation

thetorpedodog
Copy link
Contributor

Based on a comment from the previous review, and a realization about how we can use urllib3’s response.url.

Copy link
Collaborator

@sgillies sgillies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, left a suggestion that you may like or not.

)
if not (200 <= probe.status < 300 or probe.status == 405):
# Once we reach the end, we should get either 405 Method Not Allowed
# or some form of OK (2xx). If not, that's an error.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# While we're probably at the place we want to end up posting to,
# we might still get redirected. This time, we need to handle redirects
# manually, since we have to rewind the request body each time.
redirects_left = 3
while True:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thetorpedodog your use of the walrus operation suggested to me that we could use more built-in Python features. Maybe this?

Suggested change
while True:
while redirects_left > 0:
...
else:
raise tce.TileDBCloudError.from_response(resp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing something like this, but ended up with the structure we have now, because all the dealing with the response needs to happen in a try/finally for each individual request, so this was the best structure.

HAVING SAID THAT this made me realize that I think we need to release the connection for the HEAD request as well!

@thetorpedodog thetorpedodog merged commit e6bbebb into main Sep 18, 2024
18 checks passed
@thetorpedodog thetorpedodog deleted the file-upload-redirect branch September 18, 2024 14:48
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

Successfully merging this pull request may close these issues.

2 participants