juhaku / utoipa

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

[axum][v3.4.0] Option<Query<T>> causes a build error #677

Closed masnagam closed 1 year ago

masnagam commented 1 year ago

The following test can pass in v3.3.0:

    #[test]
    fn test_into_params_for_option_query_type() {
        #[utoipa::path(
            get,
            path = "/items",
            params(("id" = u32, Query, description = "")),
            responses(
                (status = 200, description = "success response")
            )
        )]
        #[allow(unused)]
        async fn get_item(id: Option<Query<u32>>) {}

        #[derive(OpenApi)]
        #[openapi(paths(get_item))]
        struct ApiDoc;
    }

But it fails building in v3.4.0 due to the following error:

error[E0277]: the trait bound `axum::extract::Query<u32>: IntoParams` is not satisfied
  --> src/lib.rs:17:38
   |
17 |         async fn get_item(id: Option<Query<u32>>) {}
   |                                      ^^^^^ the trait `IntoParams` is not implemented for `axum::extract::Query<u32>`

For more information about this error, try `rustc --explain E0277`.

Query<T> works fine in the both versions.

Using git bisect run ..., I confirmed that #666 causes this issue. Maybe, an issue similar to https://github.com/juhaku/utoipa/issues/675#issuecomment-1634996207 occurs in axum.

masnagam commented 1 year ago

The actual build error occurred in our project is: https://github.com/mirakc/mirakc/actions/runs/5539131651/jobs/10109770807?pr=1065#step:6:859

Other build errors can be fixed by simply adding #[derive(IntoParams) and #[into_params(...)].

juhaku commented 1 year ago

Oh bummer, I'll make a test for this case today and try to find a way to solve this issue, It somehow thinks the axum::Query is IntoParams type even though it should not be.

juhaku commented 1 year ago

@masnagam A quick fix for this is to wrap the actual parameter with the Option.

async fn get_item(id: Query<Option<u32>>) {}

This way the compile will work. And most likely it should mark the parameter as non-required. But if it is defined with manually as you did above, it will will be (or at least should be) required because of explisit type declaration without Option<T> wrapper.

#[utoipa::path(
         get,
         path = "/items",
         params(("id" = u32, Query, description = "")), // <-- here, not defined as Option<u32>
         responses(
             (status = 200, description = "success response")
         )
     )]
juhaku commented 1 year ago

There is actually a fundamental issue in axum and actix_web support that is, the automatic query parameter detection does not work with primitive query parameters just because primitive query parameter support has not been implemented for those frameworks. It is only expected to work on parameters which are derived from IntoParams.

See the comment for further explanation https://github.com/juhaku/utoipa/pull/678#issue-1805027576

masnagam commented 1 year ago

Thank you for taking your time for this issue.

A quick fix for this is to wrap the actual parameter with the Option

I tried this quick fix and confirmed that it can fix the build error but one of our unit tests fails.

Query<Option<T>> seems not to be treated as an optional in axum. As described in the document, an optional query parameter in axum has to be Option<Query<T>>.

I confirmed that 99020a918a5252292cc2a5cabec85aec53d5fd9e can fix the original issue I reported but the following test causes another (but a similar) build error:

    #[test]
    fn test_into_params_for_option_query_wrapped_u32() {
        #[derive(Deserialize, IntoParams)]
        #[into_params(names("id"))]
        struct Id(u32);

        #[utoipa::path(
            get,
            path = "/items",
            params(("id" = Option<u32>, Query, description = "")),
            responses(
                (status = 200, description = "success response")
            )
        )]
        #[allow(unused)]
        async fn get_item(id: Option<Query<Id>>) {}

        #[derive(OpenApi)]
        #[openapi(paths(get_item))]
        struct ApiDoc;
    }
error[E0277]: the trait bound `axum::extract::Query<Id>: IntoParams` is not satisfied
  --> src/lib.rs:42:38
   |
42 |         async fn get_item(id: Option<Query<Id>>) {  // use the inner type here
   |                                      ^^^^^ the trait `IntoParams` is not implemented for `axum::extract::Query<Id>`
   |
   = help: the trait `IntoParams` is implemented for `Id`

For more information about this error, try `rustc --explain E0277`.

And this is our case. We use a type wrapping a primitive type (the linked source file is for v3.3.0 and it works without #[derive(IntoParams)]).

The workaround we found is like this:

    #[test]
    fn test_into_params_for_option_query_wrapped_u32() {
        // We no longer need to implement the following traits on `Id` in this test case.
        //#[derive(Deserialize, IntoParams)]
        //#[into_params(names("id"))]
        struct Id(u32);

        #[utoipa::path(
            get,
            path = "/items",
            params(("id" = Option<u32>, Query, description = "")),
            responses(
                (status = 200, description = "success response")
            )
        )]
        #[allow(unused)]
        async fn get_item(id: Option<Query<u32>>) {  // use the inner type here
            // convert it into the outer type
            let id = id.map(|Query(id)| Query(Id(id)));
            // and then call the original function renamed to `do_get_item`
            do_get_item(id).await
        }

        async fn do_get_item(_id: Option<Query<Id>>) {}

        #[derive(OpenApi)]
        #[openapi(paths(get_item))]
        struct ApiDoc;
    }

This works fine with 99020a918a5252292cc2a5cabec85aec53d5fd9e.

I'm not familiar with procedual macros, generating code like the one above might solve this issue if possible.

juhaku commented 1 year ago

Eh, this is parameter rabbit hole is getting deeper and deeper :rofl: I can also make a quick fix for this, but it does not completely solve the issue which causes yet another side effect that is, then the id parameter cannot be defined in params(...) block anymore. Moreover this makes it impossible to add description and other attributes for the parameter.

This might indicate that there is actually a bigger rework to be done for what comes to the parameters that need to take place in some timeframe.

masnagam commented 1 year ago

True. It might be better to give up to automatically generate params for such a type for a while.

Currently, we face the build error that I reported even if we explicitly add params for the wrapper type in utoipa::path().

Automatically generating params for a type implemeing IntoParams is best, but I think it's enough at least for me to manually add params in utoipa::path(). However, I don't think it's good to modify a naive implementation to one like my workaround in order to avoid the build error.

Is there any workaround to avoid the build error without modifying the naive implementation? For example, specifying a particular filed in utoipa::path().

juhaku commented 1 year ago

Is there any workaround to avoid the build error without modifying the naive implementation? For example, specifying a particular filed in utoipa::path().

At the moment no, the only 2 options are to use previous version 3.3.0 or the workarounds discussed in this issue.

True. It might be better to give up to automatically generate...

Yeah, it might be as good to just wait for the better implementation that solves the edges that the current one is lacking.

masnagam commented 1 year ago

Thank you for your reply.

OK. I keep this ticket open and stop updating utoipa for a while.

utoipa is one of greate libraries used in our project. I would like to once again thank all the contributors to this project.

juhaku commented 1 year ago

The old parameter behavior is restored in #696. I'll make new release soon with the changes. More about this here https://github.com/juhaku/utoipa/issues/675#issuecomment-1646608089.

masnagam commented 1 year ago

We could upgrade to 3.4.3 without any changes in our code.

Thank you for taking your time for this issue.