Skip to content

Conversation

@Murderlon
Copy link
Member

@Murderlon Murderlon commented Sep 26, 2022

Supersedes #283

@Murderlon Murderlon requested a review from mitjap September 26, 2022 18:37
@mitjap
Copy link
Collaborator

mitjap commented Sep 26, 2022

I'd rename getOffset to getMetadata and create new function setMetadata or updateMetadata to modify existing metadata. In index.d.ts we need to modify IFile and add property size: number. With this ready this issue should be easy to fix :)

@mitjap mitjap force-pushed the fix-upload-complete-with-defer branch 2 times, most recently from dfeede8 to be44f6f Compare September 26, 2022 23:37
@mitjap
Copy link
Collaborator

mitjap commented Sep 26, 2022

This PR is almost complete. All that is needed is to implement declareUploadLength on S3 and GCS datastores and enable tests via shared.shouldDeclareUploadLendth().

Any idea why status 412 is sent when upload-length does not match? I think this error code is not appropriate and 400 would be much better.

INVALID_LENGTH: {
        status_code: 412,
        body: 'Upload-Length or Upload-Defer-Length header required\n',
    },

@Murderlon sorry for taking over this PR without consulting you. 😊

@mitjap mitjap force-pushed the fix-upload-complete-with-defer branch from be44f6f to 3222647 Compare September 26, 2022 23:49
@Murderlon
Copy link
Member Author

Any idea why status 412 is sent when upload-length does not match? I think this error code is not appropriate and 400 would be much better.

Yes we can change that, tus also does this:
https://github.com/tus/tusd/blob/6ca6ef69a21c60dc0d00bfc55c4a34fc6398cf1b/pkg/handler/unrouted_handler.go#L60:2

@Murderlon
Copy link
Member Author

I'll take a look at the S3 and GCS ones

@Murderlon
Copy link
Member Author

Murderlon commented Sep 27, 2022

Thanks for improving this too. I added it to S3 and GCS too. Two things:

  • We have a race condition in CI when running the tests in parallel across node versions. It seems the CRUD operations against GCS don't allow these parallel changes and we get rate limited. Not sure what to do about that, I'm considering testing against one node version for now.
  • Was quite confused for an hour that GCS has the very unobivous behavior when setting some meta data value to undefined, it actually ignores that and holds the previous value..

EDIT: I disabled parallel jobs in 43c45b7, which seems to help with rate limiting.

Regardless, this PR should be ready

@Murderlon Murderlon requested a review from mitjap September 27, 2022 18:28
@Murderlon Murderlon changed the title Fix upload complete emit when upload-defer-length is set Fix creation-defer-length extension for all stores Sep 27, 2022
@Murderlon Murderlon merged commit 04009ea into master Sep 28, 2022
@Murderlon Murderlon deleted the fix-upload-complete-with-defer branch September 28, 2022 16:18
@Murderlon
Copy link
Member Author

Merged it as I have more time for tus-node-server and I want to run a TS migration script without too many conflicts. Review still welcome, I can make the changes in a separate PR. I'll wait another day with releasing this in case you have feedback.

@mitjap
Copy link
Collaborator

mitjap commented Sep 28, 2022

I have no further comments. I think this can be released, 0.8.0 would be appropriate as there are some breaking changes.

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