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

fix(integ-tests): Uint8Arrays are not decoded properly #27009

Merged
merged 8 commits into from
Sep 6, 2023

Conversation

otaviomacedo
Copy link
Contributor

AWS SDK v3 has changed some data types to Uint8Array. If a value of this type was provided by the user, it would not be decoded properly back to a Uint8Array before making the call.

Recognize when there is an encoded Uint8Array and decode it back properly.

Closes #26798.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team September 5, 2023 09:49
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Sep 5, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 5, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 5, 2023

if (parsed != null && !Array.isArray(parsed) && typeof parsed === 'object') {
const entries = Object.entries(parsed);
if (isByteArray(entries)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

But what if the payload is a dictionary that looks like a stringified bytearray, but is actually supposed to be a normal dictionary?

I agree it might be a little edgy, but can we think of a different solution that considers it? Like adding a marker property when we encode, so we can use it when we decode?

@mrgrain remember this? can you think of another way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't even understand how we end up here. Where is that Uint8Array coming from, and how did it end up JSON.stringify()ed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is being passed directly by the user.

const kinesisStream: IStream = new Stream(stack, "TestKinesisStream");

 const buf = Buffer.from(JSON.stringify({ name: "world" }));
 const data = new Uint8Array(
   buf.buffer,
   buf.byteOffset,
   buf.byteLength / Uint8Array.BYTES_PER_ELEMENT
 );

 testCase.assertions.awsApiCall("Kinesis", "putRecord", {
   StreamName: kinesisStream.streamName,
   Data: data,
   PartitionKey: "testKey",
 });

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there is a reason why untyped arguments that go over the network should really be JSON strings 🙈
I wonder if this is even a SDKv3 bug. I know we changed the handling here, but this might have not worked before (but I don't know).

I'll guess we have to put a marker on it if we want to support it. Also should probably base64 encode it during transit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah a Uint8Array cannot be expected to be sent over the wire just like that.

Either:

  • We reject the value, but allow the use case by making the equality testing a bit smarter in the assertions library -- perhaps we can test the equality of a Uint8Array to a string (assuming utf-8 encoding), or an array of numbers.
  • Either that, or we marshal the Uint8Array explicitly over the wire with a type tag, and reconstruct it on the other end.

Honestly, I'm feeling most for the first solution. Morally, what we are trying to do is test equality of the buffer to a certain string. The fact that we need to encode/decode that string/byte array could be an implementation detail that that the assertions module just takes care of.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

See my last comment

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 6, 2023
* signals to the decoder that "foo" should be handled separately
* and treated as an array of bytes.
*/
readonly specialTypes?: Record<string, string>;
Copy link
Contributor

@rix0rrr rix0rrr Sep 6, 2023

Choose a reason for hiding this comment

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

This solution of marking certain parameters as having a different type cannot be applied recursively (or at least not conveniently).

Can we not do an object with a type tag, that we can apply recursively?

encode({
  string: 'string',
  obj: { field: 'value' },
  weirdarray: new Uint8Array([1, 2, 3]),
})
// => 
{
  string: 'string',
  obj: { field: 'value' },
  weirdarray: {
    '$type': 'ArrayBufferView',
    elements: [1, 2, 3],
  },
}

EDIT: or, as you're doing today:

{
  '$type': 'ArrayBufferView',
  bytes: '\x01\x02\x03',
}

@mergify
Copy link
Contributor

mergify bot commented Sep 6, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: e357c91
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 47ab5c8 into main Sep 6, 2023
9 checks passed
@mergify mergify bot deleted the otaviom/decode-uint8array branch September 6, 2023 11:44
@mergify
Copy link
Contributor

mergify bot commented Sep 6, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify bot pushed a commit that referenced this pull request Sep 11, 2023
…ry (#27092)

#27009 introduced support for `Uint8Array` if users want to send data with the same types as those declared in the SDK v3 API. But existing tests, that use strings, will break.

Apply type coercion when necessary.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mikewrighton pushed a commit that referenced this pull request Sep 14, 2023
AWS SDK v3 has changed some data types to Uint8Array. If a value of this type was provided by the user, it would not be decoded properly back to a Uint8Array before making the call.

Recognize when there is an encoded Uint8Array and decode it back properly.

Closes #26798.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mikewrighton pushed a commit that referenced this pull request Sep 14, 2023
…ry (#27092)

#27009 introduced support for `Uint8Array` if users want to send data with the same types as those declared in the SDK v3 API. But existing tests, that use strings, will break.

Apply type coercion when necessary.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(integ-tests-alpha): awsApiCall failing for Kinesis putRecord - cannot properly pass Data
5 participants