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

Rocket: Incorrectly derived query parameters defined as <foo..> #1070

Closed
dyc3 opened this issue Sep 30, 2024 · 7 comments · Fixed by #1089
Closed

Rocket: Incorrectly derived query parameters defined as <foo..> #1070

dyc3 opened this issue Sep 30, 2024 · 7 comments · Fixed by #1089
Labels
bug Something isn't working investigate Futher investigation needed before other action

Comments

@dyc3
Copy link

dyc3 commented Sep 30, 2024

I'm trying to add pagination query parameters to some endpoints in my Rocket 0.5 project.

#[derive(Debug, Serialize, Deserialize, FromForm, ToSchema, IntoParams)]
#[into_params(parameter_in = Query, style = Form)]
pub struct PageParams {
    pub page: u64,
    pub per_page: u64,
}

I originally defined my endpoint like this:

#[utoipa::path(
    context_path = "/user/api_keys",
    responses(
        (status = 200, body = Paginated<Item>),
        (status = 400, body = ()),
    ),
)]
#[get("/list?<page..>")]
async fn list_items(
    page: Option<PageParams>,
) -> Result<Paginated<Item>>, Status> {

This cased the following to be emitted in the parameters field in the spec:

- name: page
  in: query
  required: false
  schema:
    allOf:
      - type: 'null'
      - $ref: '#/components/schemas/PageParams'

To attempt to work around it, I figured out how I'm actually supposed to use IntoParams.

I originally defined my endpoint like this:

#[utoipa::path(
    context_path = "/user/api_keys",
    params(
        PageParams,
    ),
    responses(
        (status = 200, body = Paginated<Item>),
        (status = 400, body = ()),
    ),
)]
#[get("/list?<page..>")]
async fn list_items(
    page: Option<PageParams>,
) -> Result<Paginated<Item>>, Status> {

This emits the parameters correctly, but it still emits the incorrect, auto derived page parameter too.

parameters:
      - name: page
        in: query
        required: true
        schema:
          type: integer
          format: int64
          minimum: 0
        style: form
      - name: per_page
        in: query
        required: true
        schema:
          type: integer
          format: int64
          minimum: 0
        style: form
      - name: page
        in: query
        required: false
        schema:
          allOf:
          - type: 'null'
          - $ref: '#/components/schemas/PageParams'
@juhaku
Copy link
Owner

juhaku commented Sep 30, 2024

Right, I suppose this is with the latest beta since there is at least the openapi 3.1 going on. Those IntoParams should not implement ToSchema but need to investigate why the query params are not filtered out correctly with rocket.

@juhaku juhaku added the investigate Futher investigation needed before other action label Sep 30, 2024
@dyc3
Copy link
Author

dyc3 commented Sep 30, 2024

Ah yes, I forgot to mention that I am using 5.0.0-beta.0. If you give me a couple code pointers, I'd be happy to send a PR.

@juhaku
Copy link
Owner

juhaku commented Oct 1, 2024

Okay, sure. First you should run the code with default-features = false and use debug feature at least (for debugging the types) and then with the rocket_extras feature flag and macros feature flag. E.g. I am running with following features.

{
    "cargo": {
        "buildScripts": {
            "rebuildOnSave": false
        },
        "noDefaultFeatures": true,
        "features": [
            "debug",
            "chrono",
            "time",
            "rocket_extras",
            "uuid",
            "decimal",
            "smallvec",
            "macros",
            "repr",
            "config"
        ]
    }
}

Then the code for this lives in ext.rs and ext/rocket.rs in utoipa-gen. Good idea is to create a test to test/path_derive_rocket.rs for scenario above and then start debugging e.g. with dbg!(..) macro what happens in utoipa-gen/src/lib.rs path attribute macro when it resolves the query parameters for rocket. This is the entry point:

pub fn path(attr: TokenStream, item: TokenStream) -> TokenStream {

The interesting part should be in here, what happens here:

utoipa/utoipa-gen/src/lib.rs

Lines 1757 to 1770 in 8d5149f

let (arguments, into_params_types, body) =
match PathOperations::resolve_arguments(&ast_fn.sig.inputs, path_args, body) {
Ok(args) => args,
Err(diagnostics) => return diagnostics.into_token_stream().into(),
};
let parameters = arguments
.into_iter()
.flatten()
.map(Parameter::from)
.chain(into_params_types.into_iter().flatten().map(Parameter::from));
path_attribute.update_parameters_ext(parameters);
path_attribute.update_request_body(body);

@dyc3
Copy link
Author

dyc3 commented Oct 1, 2024

Thanks! I'll take a look tomorrow.

@dyc3
Copy link
Author

dyc3 commented Oct 2, 2024

I don't think this is as trivial to fix as I thought.

The parameter resolution just takes into account the function arguments. There's no way to get the contents of PageParams without accessing the ToSchema impl to get the actual fields, which AFAIK requires the parameters to be resolved at runtime.

@juhaku
Copy link
Owner

juhaku commented Oct 3, 2024

Right, if it gets to that, then for sure that is not really possible in any realistic terms. And yes because from code like below, there is no way to distinct the page param from a struct that implements IntoParams from primitive types. Or is it? Maybe we can filter out the parameters that are not primitive types from being added as params to the path. 🤔

#[utoipa::path(
   context_path = "/user/api_keys",
   params(
       PageParams,
   ),
   responses(
       (status = 200, body = Paginated<Item>),
       (status = 400, body = ()),
   ),
)]
#[get("/list?<page..>")]
async fn list_items(
   page: Option<PageParams>,
) -> Result<Paginated<Item>>, Status> {

juhaku added a commit that referenced this issue Oct 4, 2024
This commit fixes the trailing query parameters wrapped in `Option<T>`.
Option wrapped query parameters were mistaken to value parameters and
treated incorrectly.

Fixes #1070
@juhaku juhaku added the bug Something isn't working label Oct 4, 2024
@juhaku
Copy link
Owner

juhaku commented Oct 4, 2024

There was actually bug in resolving trailing query parameters. If the page was defined without Option<PageParams> it would have worked. The following will work, but there is PR #1089 that fixes this for rocket allowing it to be wrapped with Option.

#[get("/list?<page..>")]
async fn list_items(
   page: PageParams,
)  {}

juhaku added a commit that referenced this issue Oct 4, 2024
This commit fixes the trailing query parameters wrapped in `Option<T>`.
Option wrapped query parameters were mistaken to value parameters and
treated incorrectly.

Fixes #1070
@juhaku juhaku moved this to In Progress in utoipa kanban Oct 4, 2024
@juhaku juhaku closed this as completed in a282141 Oct 4, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in utoipa kanban Oct 4, 2024
@juhaku juhaku moved this from Done to Released in utoipa kanban Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigate Futher investigation needed before other action
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants