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

Refactor enums processing #1059

Merged
merged 1 commit into from
Sep 21, 2024
Merged

Refactor enums processing #1059

merged 1 commit into from
Sep 21, 2024

Conversation

juhaku
Copy link
Owner

@juhaku juhaku commented Sep 21, 2024

This commit refactors enum processing from ground up. This fully unifies the how each enum variant schema is resolved and the way variant schemas are generated. This will result easier debugging, changing and updating the enum processing in future and most of all the enums now will behave consistently due to removing bunch of duplication and adding correct abstractions instead.

This commit also unifies previously known SimpleEnum and ReprEnum to a single enum to furhter simplify the code. Also ComplexEnum is now known by MixedEnum.

This commit implements discriminator with support for custom mapping. Discriminator can only be used with #[serde(untagged)] enum having only unnamed field variants with one schema reference implementing ToSchema trait. It cannot be used with primitive types nor with inlined schemas.

Removed #[serde(tag = ...)] as discriminator support.

Update docs and add support for missing features for enum variants such as Title, Deprecated, MinProperties and MaxProperties.

Breaking changes

  • #[serde(tag = ...)] will not be used as discriminator.

Example of new discriminator syntax. It can be defined with discriminator field or optionally with custom mappping.

#[schema(discriminator = "bar_type")]
#[serde(untagged)]
enum BarInternal {
    Baz(BarBarInternal),
    FooBar(FooInternal),
}

// with custom mapping
#[schema(discriminator(property_name = "bar_type", mapping(
    ("bar" = "#/components/schemas/BarBarInternal"),
    ("foo" = "#/components/schemas/FooInternal"),
)))]
#[serde(untagged)]
enum BarInternal {
    Baz(BarBarInternal),
    FooBar(FooInternal),
}

Fixes #621 Fixes #324
Closes #617 Closes #346

@1lutz
Copy link
Contributor

1lutz commented Sep 21, 2024

Does that mean BarBarInternal and FooInternal structs need to include a string field bar_type?

@juhaku
Copy link
Owner Author

juhaku commented Sep 21, 2024

Does that mean BarBarInternal and FooInternal structs need to include a string field bar_type?

Yes, as it is by the OpenAPI spec.

@juhaku juhaku force-pushed the chore-refactor-enums branch 3 times, most recently from 84c5403 to 55d64c5 Compare September 21, 2024 13:59
This commit refactors enum processing from ground up. This fully unifies
the how each enum variant schema is resolved and the way variant schemas
are generated. This will result easier debugging, changing and updating
the enum processing in future and most of all the enums now will behave
consistently due to removing bunch of duplication and adding correct
abstractions instead.

This commit also unifies previously known `SimpleEnum` and `ReprEnum` to
a single enum to furhter simplify the code. Also `ComplexEnum` is now
known by `MixedEnum`.

This commit implements discriminator with support for custom mapping.
Discriminator can only be used with `#[serde(untagged)]` enum having
only unnamed field variants with one schema reference implementing
`ToSchema` trait. It cannot be used with primitive types nor with
inlined schemas.

Removed `#[serde(tag = ...)]` as discriminator support.

Update docs and add support for missing features for enum variants such
as `Title`, `Deprecated`, `MinProperties` and `MaxProperties`.

 ### Breaking changes

* `#[serde(tag = ...)]` will not be used as discriminator.
@juhaku juhaku force-pushed the chore-refactor-enums branch from 55d64c5 to ac15071 Compare September 21, 2024 14:03
@juhaku juhaku merged commit e13cfe1 into master Sep 21, 2024
22 checks passed
@juhaku juhaku deleted the chore-refactor-enums branch September 21, 2024 14:11
@1lutz
Copy link
Contributor

1lutz commented Sep 21, 2024

Then I don't like this solution. I don't think there must be a 1:1 relation between the OpenAPI Schema and Rust code. You normally would not write code like in the following:

struct Dog {
    pet_type: String
}

@juhaku
Copy link
Owner Author

juhaku commented Sep 21, 2024

That is true, however the enums work as previously they did, just the #[serde(tag = ...)] does not generate the discriminator field anymore. This at the beginning was wrong because the schema that is generated with the tag does include polymorphism with inlined schemas. This is wrong as the discriminator will not be adhered to inlined schemas.

This previous "broken" discriminator implementation has caused errors the users have been facing and led to manual mapping and hacks anyway. Thus it is better to remove the broken implementation in the first place.

The assessment of supporting #[serde(tag = ...)] as a discriminator needs to be done in future and is in a better state after this PR code wise. To fully support such case still needs further work and planning of how to correctly leverage the #[serde(tag = ...)] attribute to generate the discriminator field in such a way that the generated spec is correct and will not cause issues.

@1lutz
Copy link
Contributor

1lutz commented Sep 21, 2024

When you assess the support of #[serde(tag = ...)] as a discriminator, you may look into what I programmed for my company: https://github.com/geo-engine/geoengine/blob/46a5336c0ec194fe67594b3727fad784873d3b38/services/src/util/apidoc.rs#L155

It takes the "broken" enums with inline schemas and a discriminator, generates new variant schemas with a nice name (or in some case edits existing schemas), links them in the enum schema and adapts the discriminator on-the-fly. There are tests below which show the results of the transformation.

Maybe you would like to integrate something like this in the future.

@juhaku
Copy link
Owner Author

juhaku commented Sep 21, 2024

That is pretty neat. Thanks you for providing me link. It is always great to see other peoples solutions for emerging inspiration.

Sure something like this could be evaluated at least to the level of supporting unnamed field variants having one ToSchema reference. For what comes to the inlined variants like named field variant, it might be quite hard at compile time to convert it to a schema that suppose to be then added to the spec. Before even thinking such possibility I need to first solve the problem of registering schemas from their usage such as definition on #[utoipa::path(....)] macro and nested schemas within ToSchema types. This would most likely give me fundamentals to work on such functionality.

But implementing this kind of transformer internally should not be too big of a thing though. Nevertheless there is a need to assess whats feasible and whats possible.

@ChristianBeilschmidt
Copy link

ChristianBeilschmidt commented Feb 5, 2025

Removed #[serde(tag = ...)] as discriminator support.

Why? This is the common case for using serde to deserialize API calls with enums.

Is there a workaround to not change all our enums to untagged just because of OpenAPI specs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
3 participants