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

Lint accept range #535

Merged
merged 21 commits into from
Mar 10, 2023
Merged

Lint accept range #535

merged 21 commits into from
Mar 10, 2023

Conversation

aspacca
Copy link
Collaborator

@aspacca aspacca commented Mar 1, 2023

i messed with #527

@aspacca
Copy link
Collaborator Author

aspacca commented Mar 1, 2023

@ochaton sorry for the mess :(

Copy link
Collaborator

@stefanbenten stefanbenten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -54,11 +53,13 @@ import (
textTemplate "text/template"
"time"

"github.com/dutchcoders/transfer.sh/server/storage"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove this empty line :)

Copy link
Collaborator Author

@aspacca aspacca Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, let's have a command for applying this kind of linting :)

server/handlers.go Outdated Show resolved Hide resolved
Comment on lines +7 to +10
"strconv"
"time"

"regexp"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering and empty line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go fmt is reordering imports by default :( maybe you have some configuration for it to get along with the codestyle?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i use directly gofmt: I don't think to have any configuration for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it indeed goimports -w . but it does not change anything of the above

@aspacca
Copy link
Collaborator Author

aspacca commented Mar 3, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

aspacca commented Mar 3, 2023

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
Copy link
Collaborator Author

aspacca commented Mar 5, 2023

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
Copy link
Contributor

ochaton commented Mar 6, 2023

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

@aspacca
Copy link
Collaborator Author

aspacca commented Mar 6, 2023

@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
Copy link
Contributor

ochaton commented Apr 5, 2023

@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
Copy link
Collaborator Author

aspacca commented Apr 5, 2023

@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

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.

3 participants