dutchcoders / transfer.sh

Easy and fast file sharing from the command-line.
https://github.com/dutchcoders/transfer.sh
MIT License
15.09k stars 1.54k forks source link

Lint accept range #535

Closed aspacca closed 1 year ago

aspacca commented 1 year ago

i messed with #527

aspacca commented 1 year ago

@ochaton sorry for the mess :(

aspacca commented 1 year ago

Didnt test with Storj yet sadly. But i'll try to squeeze it in today.

I've tried five minutes ago: upload was fine, but when I try to open the url jsondecoding of the metadata fails with Error metadata: EOF

The donwloaded metadata seems empty or something similar (they are not on the bucket): that's even before than applying the range request when downloading the real file

btw: how do I delete the storj account I've created? :)

stefanbenten commented 1 year ago

Didnt test with Storj yet sadly. But i'll try to squeeze it in today.

I've tried five minutes ago: upload was fine, but when I try to open the url jsondecoding of the metadata fails with Error metadata: EOF The donwloaded metadata seems empty or something similar (they are not on the bucket): that's even before than applying the range request when downloading the real file

Can you check if the file actually exists in the bucket?

btw: how do I delete the storj account I've created? :)

Simple support ticket 👍

aspacca commented 1 year ago

Can you check if the file actually exists in the bucket?

they exists, and the metadata is a proper json: if i print the content from storj.Download i get (0x0, 0x0) or something similar

aspacca commented 1 year ago

I've tried five minutes ago: upload was fine, but when I try to open the url jsondecoding of the metadata fails with Error metadata: EOF

it was because we didn't end in https://github.com/storj/uplink/blob/main/download.go#L60-L62, but rather in https://github.com/storj/uplink/blob/main/download.go#L78-L82, and finally reaching https://github.com/storj/uplink/blob/main/private/stream/download.go#L72

it seems that if you want the full range either you pass nil as options *DownloadOptions in Project.DownloadObject() or you have to calculate the proper DownloadOptions.Offset and DownloadOptions.Length. not sure how it worked before and if it's related to bump of the storj dependencies

good to merge for me, please have a final look, @stefanbenten

ochaton commented 1 year ago

@aspacca maybe we should provide docker-compose.yml or something like that to do integration testing?

aspacca commented 1 year ago

@ochaton I've been thinking for a while about having integration tests: the problem is that only local and s3 storage are possible to test with incurring in actual infra costs for the integration :(

ochaton commented 1 year ago

@aspacca It seems that after refactoring we accidentally removed setting Content-Length header for Range downloads.

Caught on my installation during download for large range requests (64Mbytes in the middle of the file).

It is enough to add this line: https://github.com/ochaton/transfer.sh/blob/main/server/handlers.go#L1207

It is missing in upstream now https://github.com/dutchcoders/transfer.sh/blob/main/server/handlers.go#L1206

so now in this line https://github.com/dutchcoders/transfer.sh/blob/main/server/handlers.go#L1236 headers are sent to client without necessary headers.

This works, because, golang server sets Transfer-Encoding: chunked instead and this breaks some download managers (I'm using aria2c).

Could you patch this place in the upstream?

aspacca commented 1 year ago

@ochaton

I've removed https://github.com/ochaton/transfer.sh/blob/main/server/handlers.go#L1207, because from what I can see we have it already here: https://github.com/ochaton/transfer.sh/blob/main/server/handlers.go#L1257

so now in this line https://github.com/dutchcoders/transfer.sh/blob/main/server/handlers.go#L1236 headers are sent to client without necessary headers.

    // Changing the header map after a call to WriteHeader (or
    // Write) has no effect unless the HTTP status code was of the
    // 1xx class or the modified headers are trailers.

let's move all the headers before https://github.com/dutchcoders/transfer.sh/blob/main/server/handlers.go#L1236