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

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

Closed
masnagam opened this issue Jul 14, 2023 · 11 comments · Fixed by #678
Closed

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

masnagam opened this issue Jul 14, 2023 · 11 comments · Fixed by #678

Comments

@masnagam
Copy link
Contributor

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 #675 (comment) occurs in axum.

@masnagam
Copy link
Contributor Author

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
Copy link
Owner

juhaku commented Jul 14, 2023

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
Copy link
Owner

juhaku commented Jul 14, 2023

@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
Copy link
Owner

juhaku commented Jul 14, 2023

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 #678 (comment)

@masnagam
Copy link
Contributor Author

masnagam commented Jul 15, 2023

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 99020a9 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 99020a9.

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

@juhaku
Copy link
Owner

juhaku commented Jul 15, 2023

Eh, this is parameter rabbit hole is getting deeper and deeper 🤣 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
Copy link
Contributor Author

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
Copy link
Owner

juhaku commented Jul 15, 2023

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
Copy link
Contributor Author

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 added a commit that referenced this issue Jul 22, 2023
Temporarily disable automatic parameter recognition. This is to mitigate
the current issues in `IntoParams` implementation furhter discussed in
issues #675 #677 #687. This commit restores the old behavior where the
parameters must be declared with `params(...)` attribute within the
`#[utoipa::path(...)]` attribute macro. The temporary disablement is
to allow furhter releases not be blocked by the changes in the master
branch.

The automatic parameter recognition still needs some work and most
likely another approach need to experiemented before it can be finally
completed.
@juhaku
Copy link
Owner

juhaku commented Jul 22, 2023

The old parameter behavior is restored in #696. I'll make new release soon with the changes. More about this here #675 (comment).

lithiumFlower pushed a commit to lithiumFlower/utoipa that referenced this issue Jul 23, 2023
Temporarily disable automatic parameter recognition. This is to mitigate
the current issues in `IntoParams` implementation furhter discussed in
issues juhaku#675 juhaku#677 juhaku#687. This commit restores the old behavior where the
parameters must be declared with `params(...)` attribute within the
`#[utoipa::path(...)]` attribute macro. The temporary disablement is
to allow furhter releases not be blocked by the changes in the master
branch.

The automatic parameter recognition still needs some work and most
likely another approach need to experiemented before it can be finally
completed.
@masnagam
Copy link
Contributor Author

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

Thank you for taking your time for this issue.

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 a pull request may close this issue.

2 participants