No fail when checksum file without Content-Length

cavaliergopher / grab

A download manager package for Go

BSD 3-Clause "New" or "Revised" License

1.38k stars 151 forks source link

No fail when checksum file without Content-Length #27

Closed setnicka closed 4 years ago

setnicka commented 6 years ago

When there is no Content-Length header the default value is -1. So when there is -1 we cannot do check of the response length.

cavaliercoder commented 6 years ago

Would you consider writing tests the cover the following cases?

standard transfer where Content-Length is unknown
resume previous transfer where Content-Length is unknown

cavaliercoder commented 6 years ago

Thanks for raising this. It definitely needs addressing. Possible values for ContentLength are documented in https://golang.org/pkg/net/http/#Response

setnicka commented 6 years ago

I changed checks - now it directly checks HTTPResponse.ContentLength. In addition I implemented tests to check behavior for normal and resumed downloads.

I think it now should cover all cases (if I am not missing something).

setnicka commented 6 years ago

Hello, would you find some time to review this pull request? :-)

cavaliercoder commented 6 years ago

Thanks for your patience with this. Your code looks great, but there are so many possible states for this package - I need to be really cautious. I'm certain my tests are failing to cover some nuanced edge-case with this scenario. I'll try take a thorough look this weekend and get it merged.

setnicka commented 6 years ago

Hi @cavaliercoder, something new about it? :-)

cavaliercoder commented 6 years ago

Since rebasing, a couple of the checksum tests are failing, so we'll need to address that. Your test looks good, but I'd like to split it out from the existing TestChecksums. That test is more about how failed/valid checksums are handled, rather than ensuring a file can be resumed (tested elsewhere) or deal with edge-case server responses (this issue).

I'll adjust this and merge shortly! Thanks for the great work.

KennethNH commented 5 years ago

When merge?

cavaliercoder commented 4 years ago

Hey @setnicka, I went ahead and fixed the underlying issue here in 8835fd25e1df4ff402859a6ea8fd1002d9f1d7f7.

I apologize for not merging your request here. I tried quite a few ways to resolve conflicts with some more recent refactors and fix some outstanding edge cases. In the end, it made more sense just to fix the issues by hand where they now live. As a small consolation, I've attributed the fix to you. https://github.com/cavaliercoder/grab/commit/8835fd25e1df4ff402859a6ea8fd1002d9f1d7f7#diff-51565ad50d7426213c2f33579f6f0e71R712

Thanks for your understanding and excellent efforts here.