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

Fix HTTP.download stack overflow error on "/" paths #897

Merged
merged 5 commits into from Aug 17, 2022
Merged

Fix HTTP.download stack overflow error on "/" paths #897

merged 5 commits into from Aug 17, 2022

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 30, 2022

No description provided.

@quinnj quinnj requested a review from oxinabox July 30, 2022 15:38
@quinnj
Copy link
Member Author

quinnj commented Jul 30, 2022

cc: @oxinabox. I'm not super familiar with the download code, but this seems to be in the same spirit as what file_from_target is aiming to do. I'll try to figure out if I can see how we usually right tests for download stuff as well.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2022

Codecov Report

Merging #897 (d2f58be) into master (cf706bc) will increase coverage by 0.10%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
+ Coverage   80.12%   80.22%   +0.10%     
==========================================
  Files          36       36              
  Lines        2878     2903      +25     
==========================================
+ Hits         2306     2329      +23     
- Misses        572      574       +2     
Impacted Files Coverage Δ
src/ConnectionPool.jl 89.21% <ø> (+0.27%) ⬆️
src/Messages.jl 85.54% <0.00%> (-2.12%) ⬇️
src/download.jl 0.00% <0.00%> (ø)
src/clientlayers/ConnectionRequest.jl 78.94% <90.00%> (+2.32%) ⬆️
src/HTTP.jl 85.33% <100.00%> (+2.28%) ⬆️
src/Handlers.jl 84.14% <100.00%> (+0.29%) ⬆️
src/clientlayers/RetryRequest.jl 75.60% <0.00%> (-1.14%) ⬇️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@oxinabox
Copy link
Member

oxinabox commented Aug 1, 2022

I'll try to figure out if I can see how we usually right tests for download stuff as well.

We usually write tests using http://test.greenbytes.de/tech/tc2231/ which is like reference API for this particular part of the HTTP spec.

I think this one actually corresponds to the absolute path part of spec which is tested at:
http://test.greenbytes.de/tech/tc2231/#attabspath

For which the correct behavour is:

Either ignore the filename altogether, or discard the path information.

Though all web browsers:

Replaces the path separator with "_".

so we might want to match that.

quinnj and others added 2 commits August 1, 2022 06:02
Co-authored-by: Frames Catherine White <lyndon.white@invenialabs.co.uk>
@quinnj quinnj merged commit 91907fb into master Aug 17, 2022
@quinnj quinnj deleted the jq/896 branch August 17, 2022 05:42
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.

None yet

3 participants