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

@uppy/transloadit: Reset tus key in the file on error, so retried files are re-uploaded #4421

Merged
merged 5 commits into from
Apr 27, 2023

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Apr 19, 2023

Fixes #4412

@arturi arturi requested a review from aduh95 April 19, 2023 18:27
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Nice quick fix 👍

Some things I'm curious about:

  • Is tus an object? If so, do we loose the tus metadata send via Tus-Metadata? Would only deleting the id within tus do the trick too?
  • This problem does not exist in @uppy/tus?

@arturi
Copy link
Contributor Author

arturi commented Apr 20, 2023

Is tus an object? If so, do we loose the tus metadata send via Tus-Metadata? Would only deleting the id within tus do the trick too?

Yes, it is an object, but this is for @uppy/transloadit plugin only, so everything should be re-added when creating a new assembly on retry?

#attachAssemblyMetadata (file, status) {
// Add the metadata parameters Transloadit needs.
const meta = {
...file.meta,
assembly_url: status.assembly_url,
filename: file.name,
fieldname: 'file',
}
// Add Assembly-specific Tus endpoint.
const tus = {
...file.tus,
endpoint: status.tus_url,
// Include X-Request-ID headers for better debugging.
addRequestId: true,
}

I could only remove uploadUrl indeed for safety, just thought there's no point and it might keep some other side-effect we are not aware of. Maybe premature bug-fixing :)

This problem does not exist in @uppy/tus?

No, cause for @uppy/tus it’s desired behavior — if the file is already uploaded, don't re-upload again, right? For transloadit it failed because the upload is tied to the assembly.

@Murderlon
Copy link
Member

Makes sense. Do we want to test this? I think it should be doable in cypress:

  1. add file
  2. upload file but intercept with failure
  3. click retry
  4. check if the :id in the URL is different than the previous one?

@aduh95
Copy link
Member

aduh95 commented Apr 24, 2023

I've started working on the tests.

@arturi
Copy link
Contributor Author

arturi commented Apr 24, 2023

Ok, I was about to. But we don't need to intercept, as we can get failure with that test file (rce-poc-transloadit.eps) from the api? Probably intercept is cleaner, cause api can change to support that broken metadata somehow.

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks for creating a test for this. Test may be easier to follow/edit if we take out the massive responses into variables or something. But it's more of a nit.

e2e/cypress/integration/dashboard-transloadit.spec.ts Outdated Show resolved Hide resolved
@arturi arturi merged commit acf2c8e into main Apr 27, 2023
16 checks passed
@arturi arturi deleted the fix-transloadit-retry branch April 27, 2023 11:11
@github-actions github-actions bot mentioned this pull request May 2, 2023
github-actions bot added a commit that referenced this pull request May 2, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.1.1 | @uppy/status-bar       |   3.1.2 |
| @uppy/aws-s3-multipart |   3.3.0 | @uppy/transloadit      |   3.1.4 |
| @uppy/locales          |   3.2.1 | uppy                   |   3.9.0 |

- @uppy/aws-s3-multipart: allowedMetaFields: null means “include all” (Artur Paikin / #4437)
- @uppy/aws-s3-multipart: add `shouldUseMultipart ` option (Antoine du Hamel / #4205)
- @uppy/transloadit: Reset `tus` key in the file on error, so retried files are re-uploaded (Artur Paikin / #4421)
- meta: commit build file that was modified (Antoine du Hamel)
- meta: examples: add CORS settings for DigitalOcean Spaces (Antoine du Hamel / #4428)
- @uppy/aws-s3: deprecate `timeout` option (Antoine du Hamel / #4298)
- @uppy/aws-s3-multipart: make retries more robust (Antoine du Hamel / #4424)
- meta: fix badges on README (Antoine du Hamel / #4419)
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.

Retrying Transloadit upload reuses previous uploads
3 participants