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

feat(codegen): general data mapping function #3830

Merged
merged 4 commits into from Aug 2, 2022
Merged

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Jul 27, 2022

No clients diff: https://github.com/aws/aws-sdk-js-v3/compare/main...kuhe:aws-sdk-js-v3:s3/codegen-no-clients-diff?expand=1

Problem:

Object field names are often repeated multiple times per line in code that is only transferring fields from one object to another (mapping), with repetitive checks.

Examples

Request headers

const headers = {
    "content-type": "application/xml",
    ...(isSerializableHeaderValue(input.ChecksumCRC32) && { "x-amz-checksum-crc32": input.ChecksumCRC32 }),
    ...(isSerializableHeaderValue(input.ChecksumCRC32C) && { "x-amz-checksum-crc32c": input.ChecksumCRC32C }),
    ...(isSerializableHeaderValue(input.ChecksumSHA1) && { "x-amz-checksum-sha1": input.ChecksumSHA1 }),
    ...(isSerializableHeaderValue(input.ChecksumSHA256) && { "x-amz-checksum-sha256": input.ChecksumSHA256 }),
    ...(isSerializableHeaderValue(input.RequestPayer) && { "x-amz-request-payer": input.RequestPayer }),
    ...(isSerializableHeaderValue(input.ExpectedBucketOwner) && {
        "x-amz-expected-bucket-owner": input.ExpectedBucketOwner,
    }),
    ...(isSerializableHeaderValue(input.SSECustomerAlgorithm) && {
        "x-amz-server-side-encryption-customer-algorithm": input.SSECustomerAlgorithm,
    }),
    ...(isSerializableHeaderValue(input.SSECustomerKey) && {
        "x-amz-server-side-encryption-customer-key": input.SSECustomerKey,
    }),
    ...(isSerializableHeaderValue(input.SSECustomerKeyMD5) && {
        "x-amz-server-side-encryption-customer-key-md5": input.SSECustomerKeyMD5,
    }),
};

Response headers

if (output.headers["x-amz-expiration"] !== undefined) {
    contents.Expiration = output.headers["x-amz-expiration"];
}
if (output.headers["etag"] !== undefined) {
    contents.ETag = output.headers["etag"];
}
if (output.headers["x-amz-checksum-crc32"] !== undefined) {
    contents.ChecksumCRC32 = output.headers["x-amz-checksum-crc32"];
}
if (output.headers["x-amz-checksum-crc32c"] !== undefined) {
    contents.ChecksumCRC32C = output.headers["x-amz-checksum-crc32c"];
}
if (output.headers["x-amz-checksum-sha1"] !== undefined) {
    contents.ChecksumSHA1 = output.headers["x-amz-checksum-sha1"];
}

Body deserialization

if (data["Bucket"] !== undefined) {
    contents.Bucket = __expectString(data["Bucket"]);
  }
  if (data["ChecksumCRC32"] !== undefined) {
    contents.ChecksumCRC32 = __expectString(data["ChecksumCRC32"]);
  }
  if (data["ChecksumCRC32C"] !== undefined) {
    contents.ChecksumCRC32C = __expectString(data["ChecksumCRC32C"]);
  }
  if (data["ChecksumSHA1"] !== undefined) {
    contents.ChecksumSHA1 = __expectString(data["ChecksumSHA1"]);
  }
  if (data["ChecksumSHA256"] !== undefined) {
    contents.ChecksumSHA256 = __expectString(data["ChecksumSHA256"]);
  }
  if (data["ETag"] !== undefined) {
    contents.ETag = __expectString(data["ETag"]);
  }
  if (data["Key"] !== undefined) {
    contents.Key = __expectString(data["Key"]);
  }
  if (data["Location"] !== undefined) {
    contents.Location = __expectString(data["Location"]);
  }

... and more.

Proposed solution:

A general purpose data mapping function. It accepts transfer instructions in a brevity form:

{
    key: value, // unfiltered form
    key: [filter, value], // immediate filtered form
    key: [(value) => boolean, () => value] // lazy form
}

Where the key's value in the target object is created with a combination of the filter and the value, both of which can be value expressions or lazy functional providers.

source: https://github.com/aws/aws-sdk-js-v3/blob/800e8e97ea1f184111e2a85208b07d68fb7297b2/packages/smithy-client/src/object-mapping.ts

Also targeted were the following repeated patterns:

Resolved path construction, XML node creation, and default error deserialization. These were turned into helper functions.

Effects

The change for S3 in for example dist-cjs and dist-es:
dist-cjs/protocols -> 568kb to 440kb
dist-es/protocols -> 692kb to 556kb

overall reduction is approximately 0.1mb out of 1.2 to 1.3mb.

@kuhe
Copy link
Contributor Author

kuhe commented Jul 27, 2022

smithy counterpart: smithy-lang/smithy-typescript#576

@kuhe kuhe marked this pull request as ready for review July 29, 2022 15:01
@kuhe kuhe requested a review from a team as a code owner July 29, 2022 15:01
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Looks great! Just some nits

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Ship it! 🐑 🇮🇹

@kuhe kuhe merged commit 9417eae into aws:main Aug 2, 2022
@kuhe kuhe deleted the s3/codegen branch August 2, 2022 15:25
@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 Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants