-
Notifications
You must be signed in to change notification settings - Fork 225
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
Don't make oneOf errors tagged unions #2532
base: main
Are you sure you want to change the base?
Conversation
OpenAPI only lets you have one error per HTTP response code. This is a problem when converting things because Smithy has no such limitation. To address that, we added a toggle that lets you disambiguate by making the body contents a `oneOf`, which is an untagged union. The problem is that we made this a tagged union by leaning on how Smithy converts Smithy's union shape, which is tagged. To illustrate the problem, the OpenAPI modeled body technically now declares the following body valid: ```json { "Error1" { "foo": "bar" } } ``` But the model for the structure is: ``` @error("client") @HttpError(400) structure Error1 { foo: String } ``` And an actual valid body would be: ```json { "foo": "bar" } ``` This commit turns the OpenAPI body into an untagged union instead, which is how it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Could there be a case, where we can have one and the other i.e. a ref, and an object defined at the same time?
Or this is the expected output to OpenAPI ? This is referring to the JSON that you've added for test.
There's nothing stopping you from intermixing them in OpenAPI if that's what you're asking. You could have this for example: {
"oneOf": [
{ "$ref": "#/components/schemas/Foo" },
{ "type": "string" }
]
} In most cases we won't do that in our conversions though, because you effectively have to re-define shapes if you do. In this case in particular we're referencing HTTP payload definitions. These aren't necessarily whole shapes though, like here: structure HttpRequest {
@httpHeader("x-foo")
foo: String
bar: String
baz: String
} In this case the |
I see, thanks for this context. I was asking with the perspective that we needed that intermixed test-case or not. If this is just our conversion output, then I agree we won't need to have intermixed type output in OpenAPI. |
OpenAPI only lets you have one error per HTTP response code. This is a problem when converting things because Smithy has no such limitation. To address that, we added a toggle that lets you disambiguate by making the body contents a
oneOf
, which is an untagged union. The problem is that we made this a tagged union by leaning on how Smithy converts Smithy's union shape, which is tagged.To illustrate the problem, the OpenAPI modeled body technically now declares the following body valid:
But the model for the structure is:
And an actual valid body would be:
This commit turns the OpenAPI body into an untagged union instead, which is how it should be.
The question is: is this a backwards compatibility concern somehow? I think not, but maybe it should be a new flag?
Resolves #2512
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.