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

Check for headers correctly in download #391

Merged
merged 5 commits into from
Mar 14, 2019

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Mar 2, 2019

There was a problem that @mschauer found with

download("https://www.dropbox.com/s/uy6j8wru1nkqhw5/coal.csv?dl=1")

returning the wrong filename.
It was coming back with "coal.csv?dl=1",
but the correct name according to headers is "coal.csv".

I took a look in the headers and realized that they were casing all in lowercase.
Which is allowed, but the download function was doing a case sensitive check on the list of headers,
rather than using the header function on the response.

This PR fixes that.

Could add a test based on the above, but seems a bit fragile.

@oxinabox
Copy link
Member Author

oxinabox commented Mar 2, 2019

On the way past, I added a thing to break early if the stream eof as it seems that sometimes (all the time?) at first time the stream iofunction is called, the stream is not actually ready,
which means it fails to read the filename from the header, so it defaults the file name,
then makes an empty file there, as it has no body to write to it.

In the cases where the filename is part of the URL this doesn't matter as the original blank file is overwritten. But when it has to be found from the header, it means you get an extra incorrectly named empty file left behind

@mschauer
Copy link
Contributor

mschauer commented Mar 2, 2019

Can confirm that this solves the problems with Dropbox links. 👍

@oxinabox
Copy link
Member Author

oxinabox commented Mar 3, 2019

Julia 1.0 tests failing on linux because
JuliaLang/MbedTLS.jl#193

@mschauer
Copy link
Contributor

mschauer commented Mar 5, 2019

In the meanwhile I added your branch as dependency, https://github.com/mschauer/PointProcessInference.jl/blob/master/Manifest.toml

But somehow it still can go wrong, as if DataDeps does use its own dependency

For example, when running

pkg> add https://github.com/mschauer/PointProcessInference.jl

julia> observations, parameters, λinfo = PointProcessInference.loadexample("coal")

I got the old behaviour.

@fredrikekre
Copy link
Member

PointProcessInference.jl/Manifest.toml is not used when you add a package, so you will simply get the released version of HTTP.

src/download.jl Outdated Show resolved Hide resolved
@mschauer
Copy link
Contributor

mschauer commented Mar 5, 2019

Thank you I was not aware of that. To be of help, I went through the code and found no issues (I also checked that the calls to header do the right thing when the key is not found.)

Co-Authored-By: oxinabox <oxinabox@ucc.asn.au>
@oxinabox
Copy link
Member Author

oxinabox commented Mar 5, 2019

Til this is merged you can always get back on this branch by making sure it is added in your current active envirioment.

@mschauer
Copy link
Contributor

Just keeping this on your screen, as you suggested, oxinabox.

@oxinabox
Copy link
Member Author

oxinabox commented Mar 12, 2019

@quinnj @samoconnor can we merge this?
The CI failure is not related (I think it might be the MBEDTLS problem)

src/download.jl Outdated Show resolved Hide resolved
@samoconnor samoconnor merged commit 66cd222 into JuliaWeb:master Mar 14, 2019
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.

4 participants