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

Invalid command "CompletedMultipartUpload" (should be CompleteMultipartUpload) #1814

Closed
nurdism opened this issue Dec 21, 2020 · 16 comments · Fixed by #2835
Closed

Invalid command "CompletedMultipartUpload" (should be CompleteMultipartUpload) #1814

nurdism opened this issue Dec 21, 2020 · 16 comments · Fixed by #2835
Assignees
Labels
bug This issue is a bug.

Comments

@nurdism
Copy link

nurdism commented Dec 21, 2020

Not sure if I am just out of the loop or the documentation is wrong or what is up, but I am running into MalformedXML when using DigitalOcean spaces (which is supposed to be s3 compatible), in my debugging I noticed CompletedMultipartUpload doesn't have the d in Completed in the s3 documentation I am not sure if this does not work on an actual s3 instance. If the s3 API has changed can you please provide me a link to the change/notice so I can bug DigitalOcean to update spaces.

SDK version number
@aws-sdk/client-s3 3.0.0

Details of the browser/Node.js/ReactNative version
v14.8.0

@AllanZhengYP
Copy link
Contributor

HI @nurdism

The functions in protocols are all internal to the clients. Can you elaborate why this is a concern to you?

@AllanZhengYP AllanZhengYP added guidance General information and guidance, answers to FAQs, or recommended best practices/resources. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Dec 21, 2020
@nurdism
Copy link
Author

nurdism commented Dec 21, 2020

because it produces invalid XML and other S3 compatible services (like DigitalOcean spaces) can throw an error (MalformedXML) the correct xml should be:

<CompleteMultipartUpload>
  <Part>
    <PartNumber>1</PartNumber>
    <ETag>"..."</ETag>
  </Part>
</CompleteMultipartUpload>

but @aws-sdk/client-s3 along with @aws-sdk/lib-storage produces

<CompletedMultipartUpload>
  <Part>
    <PartNumber>1</PartNumber>
    <ETag>"..."</ETag>
  </Part>
</CompletedMultipartUpload>

essentially, changing const bodyNode = new __XmlNode("CompletedMultipartUpload"); to const bodyNode = new __XmlNode("CompleteMultipartUpload"); here will produce the correct XML.

this might be a bug in the codegen? (just speculaiting) as its correctly named here

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Dec 22, 2020
@skotambkar
Copy link

@AllanZhengYP This is an issue with how payload shapes use XmlName Trait. Might be helpful to look at aws/aws-sdk-go-v2#927

@AllanZhengYP AllanZhengYP added bug This issue is a bug. and removed guidance General information and guidance, answers to FAQs, or recommended best practices/resources. labels Dec 23, 2020
@AllanZhengYP AllanZhengYP self-assigned this Dec 23, 2020
@teajaymars
Copy link

Here's a hacky workaround I've used to get @aws-sdk/lib-storage working with DigitalOcean spaces. I've used the SDK's middleware to search-and-replace the tag. Hope this helps somebody until the library gets a fix:

client.middlewareStack.add(
  (next, _context) => (args: any) => {
    if (
      args.request &&
      args.request.body &&
      args.request.body.includes('CompletedMultipartUpload')
    ) {
      args.request.body = args.request.body.replace(
        /CompletedMultipartUpload/g,
        'CompleteMultipartUpload'
      )
    }
    return next(args)
  },
  {
    step: 'build',
  }
)

@nurdism
Copy link
Author

nurdism commented Feb 4, 2021

I tried @tomjrees solution but for some reason it would cause the upload promise to not resolve, so I ended up just creating a patch for this to be used with patch-package for anyone else running into this issue

@marcolarosa
Copy link

I tried @tomjrees solution as well and got the same result as @nurdism.

So two questions to the AWS folks:

  1. Is the middleware documentation incorrect?
  2. Is there an ETA on this as it's a show stopper for me? I would be happy to put in a workaround if someone knows of one that works.

paulreece42 added a commit to paulreece42/aws-sdk-js-v3 that referenced this issue May 26, 2021
See issue aws#1814 , I just did a search and replace so everything is consistent:

    $ find . -exec sed s/CompletedMultipartUpload/CompleteMultipartUpload/g {} \;
@csombok
Copy link

csombok commented Jun 10, 2021

I have the same issue that seems a showstopper for us.
I just noticed that the issue I raised has the same root cause: #1814

Any hint to implement workaround or use prerelease package would be highly appreciated.

@thenickdude
Copy link

@nurdism has the fix, by patching the package to fix AWS' bug using patch-package. It's working great for me after bumping the version to apply it to the newest SDK

@marcolarosa
Copy link

@thenickdude Patching on deployed systems before they start up is not a solution... Sorry. I like @tomjrees solution as I can apply that in my code to workaround the issue. Unfortunately though I couldn't get it to work. That said, there is a fix in the system if the AWS devs accept it: #2434

@jpntex
Copy link

jpntex commented Jun 12, 2021

@thenickdude I've used @tomjrees's middleware as a temporary workaround but got the same failed to resolve promise. It started working for me when I've added priority: 'high' to the middleware options.

{
  step: 'build',
  priority: 'high',
}

@uncvrd
Copy link

uncvrd commented Jul 15, 2021

Wow, luckily found this thread and @tomjrees solution "works" (for me) in the sense that the Upload will hang until timeout but still successfully upload once it times out...weird. but adding @jpntex 's solution of priority: high causes the upload to resolve immediately.

FWIW, I tried uploading directly to S3 without adding priority high and it uploads instantly, but when I switched over to my DO Spaces bucket is where it hits the timeout until adding priority

lonllua added a commit to lonllua/aws-sdk-js-v3 that referenced this issue Aug 3, 2021
@smagch
Copy link

smagch commented Aug 5, 2021

I've just run into the same issue, and tried SDK v2, which gave me meaningful error messages. And I figured out the reason why it didn't work.

The issue turned out to be mis-configuration of CORS settings. I specified AllowedHeaders, AllowedMethods, and AllowedOrigins But I didn't configured ExposeHeaders. Everything went well after I added ExposeHeaders: ["etag"] in CORS configuration.

I want SDK v3 to give more verbose error messages in this case, because I guess it's a common mistake anyone would make.

@brettstack
Copy link

@smagch you legend. Adding ExposeHeaders: ["etag"] helps. Can we get #2434 merged?

@AllanZhengYP
Copy link
Contributor

Hi, the mentioned PR doesn't fix the problem because it roots in the service model. Go SDK also has the same issue and fixed: aws/aws-sdk-go-v2#965. I will create a fix soon referring to their PR.

@AllanZhengYP
Copy link
Contributor

I find we cannot port the fix from Go v2 SDK directly. There's a design flaw that our shape serializers don't have reference to the parent XML node. I cannot fix it without a refactor to our serializer generation.

For this particular issue, the request's root node name is CompletedMultipartUpload because that's the shape's name:

"com.amazonaws.s3#CompletedMultipartUpload": {

However, in this case(when shape is document payload), we should have used the operation's member's xml name as name of the root node:

"MultipartUpload": {
"target": "com.amazonaws.s3#CompletedMultipartUpload",
"traits": {
"smithy.api#documentation": "<p>The container for the multipart upload request information.</p>",
"smithy.api#httpPayload": {},
"smithy.api#xmlName": "CompleteMultipartUpload"
}
},

When generating the seriliazer for the shape CompletedMultipartUpload, I don't have the reference to any trait of the member shape MultipartUpload. To fix it, we should generate the xml node inside of member serializer instead of shape serializer. I think this is the right change, but needs more time.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.