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

Don't make oneOf errors tagged unions #2532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

JordonPhillips
Copy link
Contributor

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:

{
    "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:

{
    "foo": "bar"
}

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.

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.
@JordonPhillips JordonPhillips requested a review from a team as a code owner February 20, 2025 16:47
Copy link
Contributor

@yasmewad yasmewad left a 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.

@JordonPhillips
Copy link
Contributor Author

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?

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 bar and baz members form the HTTP payload. In OpenAPI you can't intermix headers and payload members like this though. We could resolve that problem by inlining an anonymous shape. But that's not ideal because OpenAPI tooling can make a mess of those, so we instead create a synthetic shape that only has payload members that gets converted to a named OpenAPI schema. Thus we'll always have refs in cases like this, and never anonymous schemas.

@yasmewad
Copy link
Contributor

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?

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 bar and baz members form the HTTP payload. In OpenAPI you can't intermix headers and payload members like this though. We could resolve that problem by inlining an anonymous shape. But that's not ideal because OpenAPI tooling can make a mess of those, so we instead create a synthetic shape that only has payload members that gets converted to a named OpenAPI schema. Thus we'll always have refs in cases like this, and never anonymous schemas.

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.

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.

OpenApi onErrorStatusConflict: OneOf transforms errors into a tagged union
2 participants