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(protocols): fix header serde, handle unset union payloads #1417

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Sep 24, 2024

this is a set of changes revealed by re-enabling the skipped protocol tests in aws/aws-sdk-js-v3#6512

@kuhe kuhe requested review from a team as code owners September 24, 2024 21:15
@kuhe kuhe changed the title test/protocols test(protocols): ignore comma delimiter spacing for certain headers Sep 24, 2024
@kuhe kuhe changed the title test(protocols): ignore comma delimiter spacing for certain headers test(protocols): ignore comma delimiter spacing for certain headers, handle unset union payloads Sep 25, 2024
Location.PAYLOAD, "data", binding.getMember(), target));
}
);
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

old:

  const data: Record<string, any> | undefined = __expectUnion(await parseBody(output.body, context));
  contents.nested = de_UnionPayload(data, context);

(does not handle empty output correctly)

new:

  const data: Record<string, any> | undefined = await parseBody(output.body, context);
  if (Object.keys(data ?? {}).length) {
    contents.nested = __expectUnion(de_UnionPayload(data, context));
  }

@kuhe kuhe force-pushed the test/protocols branch 3 times, most recently from e2e28cf to d757a15 Compare September 27, 2024 15:51
part = `"${part.replace(/"/g, '\\"')}"`;
}
return part;
}
Copy link
Contributor Author

@kuhe kuhe Sep 27, 2024

Choose a reason for hiding this comment

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

This function filters the input/serialization side of list/set shape headers. A simple .join(",") is not enough, since if the individual elements contain either the delimiter , or ", they must be quoted. The internal quotes must also be escaped.

* @param value - header string value.
* @returns value split by commas that aren't in quotes.
*/
export const splitHeader = (value: string): string[] => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This takes over .split(",") on the deserialization side. Splitting by commas is not enough, because commas within quotes do not count as delimiters, and once split, the items wrapped by double quotes should have those quotes removed.

Behavior specified here: https://github.com/smithy-lang/smithy/blob/main/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy#L229-L232

{
        id: "RestJsonInputAndOutputWithQuotedStringHeaders",
        documentation: "Tests responses with string list header bindings that require quoting",
        protocol: restJson1,
        code: 200,
        headers: {
            "X-StringList": "\"b,c\", \"\\\"def\\\"\", a"
        },
        params: {
            headerStringList: ["b,c", "\"def\"", "a"]
        }
}

""", header, value);
} else {
writer.write("expect(r.headers[$S]).toBe($S);", header, value);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose that we ignore comma delimiter spacing rather than changing the upstream Smithy test assertion or our and other language implementations.

@kuhe kuhe changed the title test(protocols): ignore comma delimiter spacing for certain headers, handle unset union payloads fix(protocols): fix header serde, handle unset union payloads Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants