juhaku / utoipa

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

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

Closed dyc3 closed 1 month ago

dyc3 commented 1 month ago

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 commented 1 month ago

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.

dyc3 commented 1 month ago

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 commented 1 month ago

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: https://github.com/juhaku/utoipa/blob/8d5149ff8acf8af71af30bdb5baf29291e1c8462/utoipa-gen/src/lib.rs#L1702

The interesting part should be in here, what happens here: https://github.com/juhaku/utoipa/blob/8d5149ff8acf8af71af30bdb5baf29291e1c8462/utoipa-gen/src/lib.rs#L1757-L1770

dyc3 commented 1 month ago

Thanks! I'll take a look tomorrow.

dyc3 commented 1 month ago

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 commented 1 month ago

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. :thinking:

#[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 commented 1 month ago

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,
)  {}