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

upload-success not setting uploadURL correctly #5388

Closed
2 tasks done
pekeler opened this issue Aug 2, 2024 · 13 comments · Fixed by #5505
Closed
2 tasks done

upload-success not setting uploadURL correctly #5388

pekeler opened this issue Aug 2, 2024 · 13 comments · Fixed by #5505
Assignees
Labels

Comments

@pekeler
Copy link

pekeler commented Aug 2, 2024

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

Using self hosted Companion and uppy-svelte, we used to use upload-success to get the path of the uploaded file to S3 from result.body.key which worked great using version 3. Now, after upgrading to v4.1 (and the companion to v5) and seeing that upload-success has changed such that we're supposed to be getting the path from result.uploadURL, we find that the uploadURL is always just set to the bucket's base URL without anything for the uploaded file itself.

Inspecting the networking traffic between browser and companion, we see that the response from companion has the filepath under fields.key. So it seems there's some mismatch between the latest companion and the latest client lib?

https://uppy.io/docs/uppy/#upload-success

Expected behavior

result.uploadURL is the complete file's path on S3 (i.e. a bucket-url/UUID-filename)

Actual behavior

result.uploadURL is just the bucket's URL

@pekeler pekeler added the Bug label Aug 2, 2024
@Murderlon
Copy link
Member

Hi, I just tested locally and I do get the complete file path. Are you on latest versions of everything?

@pekeler
Copy link
Author

pekeler commented Aug 6, 2024

Yes, npm outdated shows that everything is up-to-date.

(This is using DigitalOcean Spaces as S3 storage - though this has worked fine for us with previous versions of Uppy).

@Murderlon
Copy link
Member

It works for me for S3 and we don't officially support or test against DigitalOcean Spaces. If you can reproduce on S3 that would be great. Otherwise you may have to contribute yourself to fix it.

@timothygachengo
Copy link

I have the same issue with AWS S3 where it returns only the base bucket URL.

@augustosamame
Copy link

augustosamame commented Aug 24, 2024

+1

I also use AWS S3 and I can see the uppy uploaded file in S3 cache folder.
However, Im also only not getting the base bucket URL in uploadURL key.

not using Companion but Shrine plugin

"@uppy/aws-s3": "^4.0.3",
"@uppy/core": "^4.1.2",
"@uppy/dashboard": "^4.0.3",
"@uppy/locales": "^4.0.3",

gem "shrine", "~> 3.6.0"

this is a complete example response (notice the weird "": "" key in the reponse body key which might be a clue):

{
"source": "Dashboard",
"id": "sample/image/jpeg-2v-2v-1e-image/jpeg-91516-1671405542014",
"name": "sample_image.jpeg",
"extension": "jpeg",
"meta": {
"relativePath": null,
"name": "sample_image.jpeg",
"type": "image/jpeg"
},
"type": "image/jpeg",
"data": {
"lastModified": 1724512075822,
"lastModifiedDate": "2024-08-24T15:07:55.822Z",
"name": "sample_image.jpeg"
},
"progress": {
"uploadStarted": 1724512075828,
"uploadComplete": true,
"percentage": 100,
"bytesUploaded": 64790,
"bytesTotal": 64790
},
"size": 64790,
"isGhost": false,
"isRemote": false,
"preview": "blob:http://localhost:3000/727c9392-9254-4115-84e9-65d37a09ee56",
"response": {
"body": {
"location": "https://devtech-edukaierp-dev.s3.amazonaws.com/",
"": ""
},
"status": 200,
"uploadURL": "https://devtech-edukaierp-dev.s3.amazonaws.com/"
},
"uploadURL": "https://devtech-edukaierp-dev.s3.amazonaws.com/",
"isPaused": false
}

@texpert
Copy link

texpert commented Aug 25, 2024

When using @uppy/aws-s3 and doing non multi-part uploads, the nonMultipartUpload method is used from HTTPCommunicationQueue.

Seems that every other uploading method in HTTPCommunicationQueue is doing

const { uploadId, key } = await this.getUploadId(file, signal)

, and then adding the key to the returned result.

No such luck with the nonMultipartUpload method.

@Sushant-Borsania
Copy link

The multipart upload (shouldUseMultipart = true) returns the full URL but the nonMultipartUpload doesn't. It's just a bucket URL. :(

@needcaffeine
Copy link

needcaffeine commented Sep 7, 2024

+1 that this is broken for:

  • aws s3
  • non-multipart upload

@Murderlon
Copy link
Member

I'm trying locally with shouldUseMultipart: false but I always get the full URL back? Not sure how to reproduce.

@janklimo
Copy link
Contributor

Seeing the same behavior in v4. @Murderlon here are the file and data objects I get in the upload-success event, perhaps it helps:

Screenshot 2024-10-17 at 12 54 07

@2lives
Copy link

2lives commented Oct 21, 2024

I encounter the same regardless of whether or not I set shouldUseMultipart to false:
image

Only became an issue once upgrading from the following package versions:
image

to updated uppy package versions:
image

upload success file:
image

complete:
image

hope this helps lead to a resolution.

@mifi
Copy link
Contributor

mifi commented Nov 4, 2024

Managed to reproduce, working on a fix

@mifi
Copy link
Contributor

mifi commented Nov 8, 2024

the problem seems to appear when the s3 bucket does not have the correct CORS policy configured as described in the docs - in particular "ExposeHeaders": ["ETag", "Location"]. in previous versions upppy this was kind of OK because Uppy would log a warning to the browser console so that we know what's wrong:

'AwsS3/Multipart: Could not read the Location header. This likely means CORS is not configured correctly on the S3 Bucket. See https://uppy.io/docs/aws-s3-multipart#S3-Bucket-Configuration for instructions.',

but apparently that null/undefined check broke in a refactor (another reason to have strict types #5336). I've now created a PR #5505 that restores the behavior where it will warn when CORS is not configured properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.