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

Regression to Blob types with respect to SDKv2 #5181

Closed
2 tasks
rix0rrr opened this issue Sep 6, 2023 · 8 comments
Closed
2 tasks

Regression to Blob types with respect to SDKv2 #5181

rix0rrr opened this issue Sep 6, 2023 · 8 comments
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue queued This issues is on the AWS team's backlog response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@rix0rrr
Copy link

rix0rrr commented Sep 6, 2023

Describe the feature

It seems that input fields marked as "Blob" types used to permissively accept types in SDKv2, but no longer do so in SDKv3.

As an example, Kinesis PutRecord:

SDKv2

  export interface PutRecordInput {
    StreamName?: StreamName;

    Data: Data;

    // ....
  }
  export type Data = Buffer|Uint8Array|Blob|string;
}

https://github.com/aws/aws-sdk-js/blob/ecd7c9c2ad37560326e1645b83ad981dbee7f4f9/clients/kinesis.d.ts#L343

SDKv3

export interface PutRecordInput {
  StreamName?: string;

  Data: Uint8Array | undefined;

  // ...
}

Data: Uint8Array | undefined;

Use Case

Why does this matter?

We are the CDK, migrating from SDKv2 to SDKv3. There are multiple places where we are driving the SDK not with our own, type-checked values, but using values we get from users and pass on to the SDK.

  • Users may have been passing us strings where BLOBs were expected, which works perfectly well with SDKv2, but is now all of a sudden broken for SDKv3.
  • Users may have set up AWS API calls from EventBridge events using a Lambda that uses the SDK to make an AWS API call, using some of the event JSON to form the payload to the AWS API call. Those used to work fine as strings, but are now broken. What's worse, the JSON events are going to contain the strings that they contain, and there's both no way to represent Uint8Arrays in JSON, as well as no ability for the user to control the format of the event in the first place.

If we knew that a Uint8Array was expected, it would be trivial for us to do a run-time conversion, but unfortunately we have no insight into the type that is expected at run time (remember, this is long past the TypeScript type checking stage). Therefore, we can't do the conversion, and have no other solution than to pass on the wrongly typed value and have the call fail.

Proposed Solution

Can the SDK not follow Postel's Law here, and also accept the other types you used to accept in SDKv2, doing an under-the-covers conversion as necessary?

Alternatively, provide us with a run-time inspectable table telling us the types of all fields, so that we can do that conversion as necessary?

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

latest

Environment details (OS name and version, etc.)

Nodejs 20

@rix0rrr rix0rrr added feature-request New feature or enhancement. May require GitHub community feedback. needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2023
@rix0rrr
Copy link
Author

rix0rrr commented Sep 6, 2023

Also, I'm not quite sure why PutRecord#Data and Invoke#Payload are being rendered differently here. Looking at the Smithy models, both are modeled as a type: "blob":

Kinesis:

"com.amazonaws.kinesis#Data": {

Lambda:

"com.amazonaws.lambda#Blob": {

@trivikr
Copy link
Member

trivikr commented Sep 6, 2023

The smithy blob type is uninterpreted binary data, so we're not accepting string anymore in JS SDK v3.

In the past, users requested to get string type for output, like s3.getObject, and we had provided mixin in #3977 which allows consumers to transform Unit8Array to relevant types.

We can add a similar fix for accepting string in input and do the conversion internally.
We'll discuss this with the rest of the JS SDK team, and update this issue.

@trivikr trivikr added needs-triage This issue or PR still needs to be triaged. p2 This is a standard priority issue queued This issues is on the AWS team's backlog and removed needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2023
@trivikr
Copy link
Member

trivikr commented Sep 6, 2023

The automatic conversion of blob types was implemented in #4836, which was published in https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.357.0

This is applicable to TypeScript types of blob. The SDK is not doing any conversion from string to Uint8Array.
For output, we did add a helper method to transform to string.

Verified using the following code:

// invokeLambdaFn.ts
import { Lambda } from "@aws-sdk/client-lambda";

const client = new Lambda({ region: "us-west-2" });
client.invoke({
  FunctionName: "test-function-name",
  Payload: JSON.stringify({ foo: "bar" }),
});
$ npx tsc --version
Version 5.1.6

v3.356.0

TypeScript TS2322 error is thrown

$ npx tsc invokeLambdaFn.ts
invokeLambdaFn.ts:7:3 - error TS2322: Type 'string' is not assignable to type 'Uint8Array'.

7   Payload: JSON.stringify({ foo: "bar" }),
    ~~~~~~~

  node_modules/@aws-sdk/client-lambda/dist-types/models/models_0.d.ts:3373:5
    3373     Payload?: Uint8Array;
             ~~~~~~~
    The expected type comes from property 'Payload' which is declared here on type 'InvokeCommandInput'


Found 1 error in invokeLambdaFn.ts:7

v3.357.0

No error is thrown by TypeScript

$ npx tsc invokeLambdaFn.ts

Is there a specific API call in CDK which we can use for testing? One which fails on sending a string for a blob type.

@trivikr trivikr added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Sep 6, 2023
@rix0rrr
Copy link
Author

rix0rrr commented Sep 7, 2023

AwsApi and AwsCustomResource, for example. But there's not really anything to test for you right now.

Thanks for the effort, let's hope this makes it to Lambda NodeJS 18.x runtime soon!

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

kuhe commented Sep 11, 2023

there may be a workaround involving providing a custom implementation to client.config.base64Encoder that converts strings to Uint8Array before calling the default base64Encoder.

@kuhe
Copy link
Contributor

kuhe commented Jun 27, 2024

we updated the default base64 encoder to accept strings and not just byteArrays. Is this still an issue in the latest SDK version?

@kuhe kuhe added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jun 27, 2024
Copy link

github-actions bot commented Jul 8, 2024

This issue has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jul 8, 2024
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 Jul 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue queued This issues is on the AWS team's backlog response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants