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

[s3-client] PutObjectCommandInput.Body accepts Blob but not ArrayBuffer #5361

Closed
3 tasks done
ImRodry opened this issue Oct 15, 2023 · 14 comments
Closed
3 tasks done
Assignees
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue wontfix We have determined that we will not resolve the issue. workaround-available This issue has a work around available.

Comments

@ImRodry
Copy link

ImRodry commented Oct 15, 2023

Checkboxes for prior research

Describe the bug

The types for PutObjectCommandInput.Body accept Blob as a valid type (when it isn't) and don't accept ArrayBuffer (and they should)

SDK version number

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Node.js v20.8.0

Reproduction Steps

const file = await fetch("https://your-url.com").then(r => r.arrayBuffer())
const command = new PutObjectCommand({
	// ...
	Body: file // Error: Type 'ArrayBuffer' is missing the following properties from type 'Uint8Array': BYTES_PER_ELEMENT, buffer, byteOffset, copyWithin, and 29 more.
})

Observed Behavior

There was a TS error when using ArrayBuffer and not when using Blob

Expected Behavior

ArrayBuffer should pass type checks and Blob shouldn't

Possible Solution

Just simply fix the types, I'm just not sure where they are

Additional Information/Context

No response

@ImRodry ImRodry added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 15, 2023
@yenfryherrerafeliz
Copy link
Contributor

Hi @ImRodry, if you are working on NodeJS runtime then, the Body needs to be provided as a Readable type, because is the type set for this parameter which you can confirm here and here. If you are working on browser runtime then, the Body can be provided as Blob, however, please be aware that Blob type from browser is different than the Blob type in NodeJs. So please, be sure that the value passed as the Body is a valid readable type and you should not have issues.

Please let me know if that helps!

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 16, 2023
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Oct 16, 2023
@ImRodry
Copy link
Author

ImRodry commented Oct 17, 2023

Hey @yenfryherrerafeliz! That is actually not true, you're looking at the wrong file. Those types come from here and are defined as string | Uint8Array | Buffer | Readable. Regardless of where they are defined they are wrong, as they should be accepting ArrayBuffer and not Blob to match what the code does

@yenfryherrerafeliz yenfryherrerafeliz added investigating Issue is being investigated and/or work is in progress to resolve the issue. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Oct 17, 2023
@yenfryherrerafeliz
Copy link
Contributor

@ImRodry, you are right, I was wrong about the typing. However, I still do not understand why this should be an issue?. The types we accept in NodeJS runtime are "string | Uint8Array | Buffer | Readable", which does not include ArrayBuffer. ArrayBuffer is a standard JavaScript object for representing binary data regardless environment, but we do not consider it as a type to be used here. Of course, in nodejs you can do the following:

Buffer.from(arrayBuffer)

and then you should not have any issues. Regarding not accepting Blob type, we actually do accept Blob type but just in browser runtime, and as I mentioned before Blob type in browser is not the same as in NodeJS.

I look forward to your response.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. and removed investigating Issue is being investigated and/or work is in progress to resolve the issue. labels Oct 17, 2023
@ImRodry
Copy link
Author

ImRodry commented Oct 17, 2023

The reason why I'm saying it should be accepted is that I am passing an ArrayBuffer obtained directly from Node's fetch library and it's working perfectly, but the types say it shouldn't, however, if I instead use res.blob() I get a runtime error even though the types pass without an issue.
Keep in mind that, as of Node.js v20, the fetch library is now implemented natively and follows the same spec as the browser's fetch, so I don't understand why there's such a discrepancy between the types. Also keep in mind that TS has no way of telling if a user is in a DOM or Node.js environment outside of its own internal libraries (as far as I'm aware, correct me if I'm wrong), so even if you have two interfaces for each of the environments, all of their types will apply to all users, which I believe is what's causing this issue 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 Oct 18, 2023
@yenfryherrerafeliz yenfryherrerafeliz added the investigating Issue is being investigated and/or work is in progress to resolve the issue. label Oct 18, 2023
@kuhe
Copy link
Contributor

kuhe commented Oct 18, 2023

Any SDK client has a config field called requestHandler that is not known at compile time. It may be one of the 4 Smithy and AWS SDK written handlers or one provided by the user.

The request and response payloads' acceptable range of types depends on the implementation. We use the superset union because we do not know the types ahead of time. Blob exists in later Node.js versions, but it is not an input type for the default Node.js http handler based on node:http.

The lack of ArrayBuffer in the declared list means that we don't support it, not that it must not work. You can use a type override to pass it in if you want to use that type.

With this change: smithy-lang/smithy-typescript#1046

You can use a type helper from the SDK to narrow the input and output payload types.

import { S3 } from "@aws-sdk/client-s3";
import { NodeJsClient } from "@smithy/types";

const s3 = new S3({}) as NodeJsClient<S3>;

await s3.putObject({
    Bucket: '',
    Key: '',
    Body: new Blob() // helper makes this a type error, must use Node.js http(s) payload types like Buffer/Uint8Array/string
})

const get = await s3.getObject({
    Bucket: '',
    Key: ''
});

const body = get.Body; // helper narrows this to SdkStream<IncomingMessage>

@yenfryherrerafeliz yenfryherrerafeliz added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. and removed investigating Issue is being investigated and/or work is in progress to resolve the issue. labels Oct 18, 2023
@ImRodry
Copy link
Author

ImRodry commented Oct 19, 2023

The lack of ArrayBuffer in the declared list means that we don't support it, not that it must not work. You can use a type override to pass it in if you want to use that type.

I'm sorry but that doesn't make any sense. How are we supposed to upload fetched files otherwise? I appreciate the PR though I'm not sure if it will work, but that is not the main goal of this issue at all, the goal is for the types to accept ArrayBuffer

@kuhe
Copy link
Contributor

kuhe commented Oct 19, 2023

Buffer.from(ArrayBuffer) is a constant time (0) operation and converts the ArrayBuffer to a supported input type.

We may support ArrayBuffer as an input type in the future, but it is not planned.

@ImRodry
Copy link
Author

ImRodry commented Oct 19, 2023

What’s stopping you from supporting it though? It already works

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 20, 2023
@yenfryherrerafeliz
Copy link
Contributor

Hi @ImRodry, I am going to mark this issue as a feature request so we can evaluate this further in the future. I appreciate the time you took reporting this and also pitching for why this should be a good type to accept. As workaround for now please use Buffer.from(arrayBuffer).

Please let me know if you have any questions.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added feature-request New feature or enhancement. May require GitHub community feedback. workaround-available This issue has a work around available. p2 This is a standard priority issue needs-review This issue/pr needs review from an internal developer. labels Oct 20, 2023
@ImRodry
Copy link
Author

ImRodry commented Oct 20, 2023

sure then, but I looked briefly at the Node.js code and it seems like doing Buffer.from() with an ArrayBuffer just creates a Uint8Array with the same data, thus why it's a constant time operation. There would be nothing else needed in order to support ArrayBuffers when you already support Buffers and Uint8Arrays

@RanVaknin RanVaknin added queued This issues is on the AWS team's backlog and removed needs-review This issue/pr needs review from an internal developer. labels Nov 15, 2023
@ImRodry
Copy link
Author

ImRodry commented Apr 7, 2024

Update: sending a ReadableStream (obtained through Response.body) also works though the types say it doesn't.
Please fix this soon

@kuhe kuhe added wontfix We have determined that we will not resolve the issue. and removed queued This issues is on the AWS team's backlog labels Apr 9, 2024
@kuhe
Copy link
Contributor

kuhe commented Apr 9, 2024

Because of our Node.js supported version range, we do not support ReadableStream as an input type in Node.js. This may change in the future when the minimum supported Node.js version has web streams.

@kuhe kuhe closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2024
@ImRodry
Copy link
Author

ImRodry commented Apr 10, 2024

Because of our Node.js supported version range, we do not support ReadableStream as an input type in Node.js. This may change in the future when the minimum supported Node.js version has web streams.

and why would you close this? There is still the initial issue I mentioned, the types are all wrong. It's also not a matter of supporting, it's already supported you just have to update the types

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 Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue wontfix We have determined that we will not resolve the issue. workaround-available This issue has a work around available.
Projects
None yet
Development

No branches or pull requests

5 participants