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: support multiple error headers #3852

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Conversation

DanielBauman88
Copy link
Contributor

@DanielBauman88 DanielBauman88 commented Aug 10, 2022

Issue

smithy-lang/smithy#1170

Description

This change adds support for multiple values in the x-amzn-errortype header. The reason for this is that API-Gateway always adds its own x-amzn-errortype header on gateway responses. This header that APIG adds and does not let the customer configure will almost never match the specific errors that customers model for their API. As an aside, I have logged a feature request for APIG to support configuration of this header - however this is unlikely to be delivered any time soon.

Anyone using this package today to generate clients of their own therefore cannot use gateway responses and have a functioning SDK for their errors. It isn't possible to avoid gateway responses for things like auth/throttling when those are configured on the apig.

This change takes the first header value from x-amzn-errortype if multiple headers are provided. It has no effect when a single header is provided.

An example curl output for a response that this works for:

HTTP/2 401
date: Tue, 09 Aug 2022 21:24:20 GMT
content-type: application/json
content-length: 92
x-amzn-requestid: xxxx
access-control-allow-origin: *
x-amzn-errortype: UnauthorizedError <- set by customer
x-amzn-errortype: UnauthorizedException <- set by API Gateway
x-amz-apigw-id: xxxx
x-amzn-trace-id: Root=xxxx

In this instance this code will select the first header value.

Testing

I built and inspected the codegen/generic-client-test/build/smithyprojections/generic-client-test/echo-service/typescript-codegen/src/protocols/Aws_restJson1.ts file.

Contents is as expected:

        const sanitizeErrorCode = (rawValue: string | number): string => {
            let cleanValue = rawValue;
            if (typeof cleanValue === "number") {
                cleanValue = cleanValue.toString();
            }
            if (cleanValue.indexOf(",") >= 0) {
                cleanValue = cleanValue.split(",")[0];
            }
            if (cleanValue.indexOf(":") >= 0) {
                cleanValue = cleanValue.split(":")[0];
            }

I didn't find tests exercising this client in the repository.

Additional context

There are a few other things that can be considered here

Do we need to support commas in header values, data.code or data["__type"]

I suspect not. A comma isn't valid in a fully qualified structure name for smithy/openapi.
There's code to split on a ":". I don't know what that is for and if we'd ever expect commas before that colon.

Alternatives

I've considered a few alternatives which I'll enumerate below and would be happy to implement.

  1. Fetch the x-amzn-errortype string via the service provider interface. Implement a default interface that returns x-amzn-errortype if no provider is specified. This is entirely backwards compatible and allows for customers to use a different header which I think is a net benefit for eventually extracting functionality from the sdk package into the generic smithy implementation. This is actually my ideal solution but it requires a little more code. If you like this please let me know and I'll make this into a PR. See example code here https://github.com/aws/aws-sdk-js-v3/pull/3853/files
  2. Check data.code before the headers. I have no idea if this would be a backwards compatible change for the SDKs and I suspect not - but listing for completeness. It would be easy to implement. This would work for our use-case because the body is entirely under our control - apig doesn't insert things like it does in the headers.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This change takes the first header value from x-amzn-errortype
if multiple headers are provided.

An example curl output for a response that this works for:
```
HTTP/2 401
date: Tue, 09 Aug 2022 21:24:20 GMT
content-type: application/json
content-length: 92
x-amzn-requestid: xxxx
access-control-allow-origin: *
x-amzn-errortype: UnauthorizedError
x-amzn-errortype: UnauthorizedException
x-amz-apigw-id: xxxx
x-amzn-trace-id: Root=xxxx
```
In this instance this code will select the first header value.

See related - smithy-lang/smithy#1170
@DanielBauman88 DanielBauman88 marked this pull request as ready for review August 10, 2022 20:04
@DanielBauman88 DanielBauman88 requested a review from a team as a code owner August 10, 2022 20:04
DanielBauman88 added a commit to DanielBauman88/aws-sdk-js-v3 that referenced this pull request Aug 10, 2022
Copy link
Contributor

@kuhe kuhe left a comment

Choose a reason for hiding this comment

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

  • run codegen for clients

I can do that and merge when able

@DanielBauman88
Copy link
Contributor Author

DanielBauman88 commented Sep 5, 2022

  • run codegen for clients

I can do that and merge when able

Hey @kuhe, did you request changes with this comment? Trying to understand if I'm missing something in the ui.

@kuhe
Copy link
Contributor

kuhe commented Sep 6, 2022

merging to run codegen on my own branch. I think the build errors are due to smithy being out of date on this branch, and will override.

@kuhe kuhe merged commit e2c4714 into aws:main Sep 6, 2022
kuhe added a commit to kuhe/aws-sdk-js-v3 that referenced this pull request Sep 6, 2022
@kuhe
Copy link
Contributor

kuhe commented Sep 6, 2022

#3904

kuhe added a commit that referenced this pull request Sep 6, 2022
@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 Sep 21, 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.

4 participants