Make download_url() follow redirects (#3235) #3236
Conversation
Fix bug related to the incorrect processing of redirects. Follow the redirect chain until the destination is reached or the number of redirects exceeds the max allowed value (by default 10).
|
Hi @slipnitskaya! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Hey @slipnitskaya and thanks for the PR. I have one minor comment below. Otherwise LGTM!
Make max number of hops a function argument and assign its default value to 10
Add the maximum number of redirect hops parameter to download_url()
There was a problem hiding this comment.
LGTM, thanks a lot! @slipnitskaya we need to add tests for this. Do you want to add some or shall I take this after this PR is merged?
|
@pmeier Would be great if you can add the tests later. |
There was a problem hiding this comment.
Thanks for the PR! I added a few comments, please let me know what you think. The two comments marked with "Nit:" are non-blocking nice to haves that we can do on a future PR. I noticed that the tests are stalling and eventually fail, could you please have a look?
|
@datumbox I'll look into the failing tests. @slipnitskaya is it ok if I push changes directly to the PR? |
|
@pmeier cool, ping me when you are ready. :) |
|
@slipnitskaya I went ahead and made a few changes:
|
Summary: * Make download_url() follow redirects Fix bug related to the incorrect processing of redirects. Follow the redirect chain until the destination is reached or the number of redirects exceeds the max allowed value (by default 10). * Parametrize value of max allowed redirect number Make max number of hops a function argument and assign its default value to 10 * Propagate the max number of hops to download_url() Add the maximum number of redirect hops parameter to download_url() * check file existence before redirect * remove print * remove recursion * add tests * Reducing max_redirect_hops Reviewed By: datumbox Differential Revision: D25954556 fbshipit-source-id: 3b2c64592d5882b98e87acdb5efd95e9283d2862 Co-authored-by: Vasilis Vryniotis <[email protected]> Co-authored-by: Philip Meier <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>
Fix bug related to the incorrect processing of redirects (#3235).
Follow the redirect chain until the destination is reached or
the number of redirects exceeds the max allowed value (by default 10).