support multiple error responses for single status code

[lewisjkl]

Previously, the Smithy => Openapi conversion only supported one error response per status code. This PR adds support for multiple by combining each of the possible error responses into a single oneof.

[kubukoz]

we have something similar internally, @ghostbuster91 we should check if this conflicts with our own implementation (or if we hit any footguns before, which could reappear here)

[lewisjkl]

we have something similar internally, @ghostbuster91 we should check if this conflicts with our own implementation (or if we hit any footguns before, which could reappear here)

That's great, if you can let me know sooner than later that'd be great. I just have a use case internally that's waiting on this to be released

[ghostbuster91]

I don't recall any footguns regarding this change. FWIW we did it in a slightly different way - by configuring the smithyOpenApi converter to use OpenApiConfig.ErrorStatusConflictHandlingStrategy.ONE_OF strategy but that requires some adjustments in createDocumentSchema method.

[lewisjkl]

I don't recall any footguns regarding this change. FWIW we did it in a slightly different way - by configuring the smithyOpenApi converter to use OpenApiConfig.ErrorStatusConflictHandlingStrategy.ONE_OF strategy but that requires some adjustments in createDocumentSchema method.

I see, yeah when I looked at that it seemed to add an extra level of nesting by creating essentially a tagged union. But I guess you're saying that you fixed that with post processing in the createDocumentSchema method? I'm open to either implementation, I'm not sure whether or not there are advantages to one over the other

[ghostbuster91]

After seeing both of them I am leaning toward this one.

[kubukoz]

I only wish upstream was doing this already...

[lewisjkl]

The last commit I pushed is just so that the description field won't be pulled from a single error shape and be irrelevant to the combined oneof

[lewisjkl]

I actually realized that there is one more piece to this which is kind of tricky: header parameters. Consider if I have:

@error("client")
@httpError(404)
structure NotFound {
    @httpHeader("x-one")
    headerOne: String

    @required
    message: String
}

@error("client")
@httpError(404)
structure NotFoundTwo {
    @httpHeader("x-two")
    headerTwo: String

    @httpHeader("x-three")
    @required
    headerThree: String

    @required
    messageTwo: String
}

I can't think of a sane way to represent this in OpenAPI other than maybe having all of the headers be present where they are all optional. I have a PR in progress for this locally that I can push up on Monday if you all think that's a decent approach. Definitely open to other ideas though.

Here's an example of what the above would look like with my idea of making them all optional:

          "404": {
            "description": "404Response",
            "headers": {
              "x-one": {
                "schema": {
                  "type": "string"
                }
              },
              "x-three": {
                "schema": {
                  "type": "string"
                }
              },
              "x-two": {
                "schema": {
                  "type": "string"
                }
              }
            },
            "content": {
              "application/json": {
                "schema": {
                  "oneOf": [
                    {
                      "$ref": "#/components/schemas/NotFoundResponseContent"
                    },
                    {
                      "$ref": "#/components/schemas/NotFoundTwoResponseContent"
                    }
                  ]
                },
                "examples": {
                  "THREE": {
                    "value": {
                      "messageTwo": "Notfoundmessagetwo"
                    }
                  },
                  "TWO": {
                    "value": {
                      "message": "Notfoundmessage"
                    }
                  }
                }
              }
            }
          }

[Baccata]

I don't have a better idea than making the headers optional, to be honest. I think you can proceed

[lewisjkl]

The latest changes I made update this so that:

• If all responses have the same exact headers, it will not make them all optional (it will leave them as is)
• If responses do NOT have the same headers, it will make all headers optional

[kubukoz]

just saw smithy-lang/smithy#2532 in the release notes of Smithy - @lewisjkl I think it invalidates your problem from #204 (comment)

I didn't look into what they did with headers. Worth checking, maybe we can get rid of whatever alloy is doing :)

edit: and you didn't get that new behavior because #206 was merged after this PR (204)... this right here is why we need to keep versions fresh!

[lewisjkl]

just saw smithy-lang/smithy#2532 in the release notes of Smithy - @lewisjkl I think it invalidates your problem from #204 (comment)

I didn't look into what they did with headers. Worth checking, maybe we can get rid of whatever alloy is doing :)

edit: and you didn't get that new behavior because #206 was merged after this PR (204)... this right here is why we need to keep versions fresh!

From what I'm seeing, it looks like all they did was update AwsRestJson1Protocol.java which wouldn't apply to our protocol here. I think what they did was more-or-less equivalent to what we now have (other than I'm not sure what they did with the headers)

[kubukoz]

yeah, thanks for checking. Let's leave it be then.