juhaku / utoipa

Simple, Fast, Code first and Compile time generated OpenAPI documentation for Rust
Apache License 2.0
2.2k stars 168 forks source link

`#[schema(as = ...)]` infects the code with `#[schema(value_type = ...)]` and `#[param(value_type = ...)]` #993

Closed JohnScience closed 5 days ago

JohnScience commented 3 weeks ago

First of all, thank you for utoipa!

Context

We use utoipa to generate the OpenAPI spec for a web service, which - among other things - proxies requests for a category of Providers of data.

For that reason, we have to expose third-party OpenAPI types in ours.

Since there are multiple Providers in the category and they all deal with the same domain, their API names overlap.

So provider1::Thing is not the same thing as provider2::Thing.

To discriminate them in the OpenAPI spec, we use paths like provider1.Thing and provider2.Thing.

provider1/mod.rs:

#[derive(ToSchema, ...)]
#[schema(as = provider1::Thing)]
pub struct Thing {
  // ...
}

and a similar thing in provider2/mod.rs.

Problem

Unfortunately, if we try to use Thing from provider1/mod.rs in another type...

provider1/mod.rs:

#[derive(ToSchema, ... )]
pub struct Stuff {
  pub thing: Thing,
  // ...
}

Expectation: Everything works just fine. Reality:: the type of field thing in the OpenAPI spec appears as Thing rather than provider1.Thing even though provider1::Thing implements ToSchema and has a #[schema(as = ...)] attribute, which can cause either collisions or unknown identifier errors. Workaround: We have to write the code like this.

#[derive(ToSchema, ... )]
pub struct Stuff {
  #[schema(value_type = provider1::Thing)]
  pub thing: Thing,
  // ...
}

and for bigger structs, it nearly doubles the size of the struct, making the code less readable.

Not only that, in combination with axum, it forces me to define a custom Path type because if we use Path extractor from axum,

#[utoipa::path(
        ...
    )]
    pub async fn get_thing(
        State(ctx): State<Arc<Context>>,
        Path(path_params): Path<(Uuid, Uuid, ThingId)>,
    ) -> impl IntoResponse {
        // ...
    }

Expectation: Everything works fine. Reality: The type provider1::ThingId appears in the OpenAPI spec as ThingId even though ThingId (trust me) implements ToSchema and has a #[schema(as = ...) attribute, which can cause either collisions or unknown identifier errors. Workaround: We have to define a whole new type:

mod get_thing {
        use utoipa::IntoParams;
        use uuid::Uuid;

        use crate::provider1::ThingId;

        #[derive(IntoParams, ...)]
        pub struct PathParams {
            pub first_uuid: Uuid,
            pub second_uuid: Uuid,
            #[param(value_type = provider1::ThingId)]
            pub thing_id: ThingId,
        }
    }

so that we can use it

#[utoipa::path(
        ...
    )]
    pub async fn get_thing(
        State(ctx): State<Arc<Context>>,
        path_params: get_thing::PathParams,
    ) -> impl IntoResponse {
        let PathParams {
            first_uuid,
            second_uuid,
            thing_id,
        } = path_params;
        // ...
    }

and have the correct OpenAPI schema.

Possible solutions

  1. Just rely on the workarounds.

Pros: It's a no-op for the maintainers. Cons: It induces pain for the utoipa users who have to use #[schema(as = ...)] types for struct fields.

  1. Add an opt-in feature for the code that would infer the OpenAPI names of types from the Rust types.

Pros: It considerably eases the pain for the utoipa users who have to use #[schema(as = ...)] types for struct fields. Cons: This adds a feature (=complexity). It also burdens the users who have to use #[schema(as = ...)] types for struct fields with the need to discover and then enable the feature.

  1. Make a breaking change that would define the "smart" behavior as the default one.

Pros: It completely eliminates the pain for the utoipa users who have to use #[schema(as = ...)] types for struct fields. Cons: It is a subtle breaking change without a quick fix.

Note: I don't see a world where someone would rely on the original behavior without understanding the risk. It's a weird behavior after all.

  1. Make a breaking change that would define the "smart" behavior as the default one but add a feature that would "dumb down" the OpenAPI type name inference.

Pros: It completely eliminates the pain for the utoipa users who have to use #[schema(as = ...)] types for struct fields. Cons: It still is a subtle breaking change but with a quick fix.

UPDATE:

There's another dimension: utoipa can temporarily start using both "smart" and "dumb" inference and in case of discrepancies emit warnings. This can set stage for a smooth breaking change. However, it can lead to a temporary feature creep, which will be resolved when the breaking changes are landed.

Suggestion

I like the idea with running both types of inferences by default because it allows to relatively safely introduce a breaking change. However, opt-in and opt-out behaviors are complicated due to the additive nature of features.

juhaku commented 2 weeks ago

First of all, thank you for utoipa!

:slightly_smiling_face: Appreciated

@JohnScience Thank you for a comprehensive post and detailed description of an issue.

True the current implementation has some room for improvement. This is a good place to introduce breaking changes to the current implementation since there is coming plenty of them when the 5.0.0 finally lands. At the moment it is getting towards beta stage but there is plenty of improvements to add before it can ultimately land.

It would be good if it actually could reuse already existing name from the original type where the as = ... attribute is defined. This should be possible to do, and then would eliminate the need for sprinkling #[schema(as = ...)] allover the codebase. I'll take a look at this in coming time.

juhaku commented 2 hours ago

Related #862