Schema options in params(...) are ignored

[ghost]

Hi utoipa folks! First of all, thank you very much for your great work on this library, it has helped us a bunch to manage our API spec!

We're just trying to update from v3.3.0 to v4.1.0, and it seems that additional schema options in params(...) inside of the #[utoipa::path(...)] now get ignored 🥲
We're using axum and have the axum feature in utoipa turned on.

For example, we have an endpoint similar to this (reduced to essential stuff):

#[utoipa::path(
    get,
    path = "/user/{user_id}",
    params(
        ("user_id", min_length = 1),
    )
)]
#[tracing::instrument(err(Debug))]
async fn get_user(
    AxumState(state): AxumState<State>,
    Path(user_id): Path<String>,
) -> Result<Json<User>> {
  // . . .

With v3.3.0, this used to emit something like this in the OAPI doc:

  /user/{user_id}:
    get:
      operationId: get_user
      parameters:
      - name: user_id
        in: path
        required: true
        schema:
          type: string
          minLength: 1

With v4.1.0, the minLength: 1 gets removed from the OAPI doc output :(

Randomly, I also tried adding the nullable annotation to the same parameter, i.e. turning it into:

    params(
        ("user_id", min_length = 1, nullable),
    )

In v4.1.0, this didn't affect the output at all.
In v3.3.0, this changed the output as expected:

  /user/{user_id}:
    get:
      operationId: get_user
      parameters:
      - name: user_id
        in: path
        required: true
        schema:
          type: string
          nullable: true
          minLength: 1

Is this a known issue? I looked around in the issues, but couldn't find one that exactly seems to match this problem (but could be that the description/title is too different.)
The only slightly similar issue I could find is #693, but I'm not sure if that one might be a separate problem affecting only format 🤔

If there's something we can do to help isolate this, please let me know!

[juhaku]

Thanks for reporting this. The issue you referred might be something similar. E.g. In one the parameters behave differently when the actix_web or other integration feature flag is enabled. This most likely needs some investigation whether the case is same here as well. There are quite a few variations when it comes to the attribute parsing and schema generation so it might be that there is some edgecase that does not handle the parsed attributes correctly. E.g. by "removing / nulling" the attributes even when it should not do that.

[ghost]

Thanks for your quick response, and sorry for reacting so late myself.

E.g. by "removing / nulling" the attributes even when it should not do that.
Yes, that's what it feels like 🤔

Since this issue removes attributes from our API spec that we need to keep, this currently blocks us from upgrading to utoipa 4.

Is there some way I can help with this?
I'm not familiar with utoipa's codebase, and I also don't have experience with writing (and debugging) macros and code generation in Rust 😅
So I don't think it's realistic I can try to isolate and fix this bug directly. 🙇

[ghost]

Hey again @juhaku,

I did some investigation around the code, and I think I've found one way to fix a simple test case (like the one I mentioned above, a string parameter with a min_length feature), but I'm not sure how robust it is, so you should definitely also look at this yourself (and we should maybe add more test cases).

The simple test case can look like this:

// in utoipa-gen/tests/path_derive_axum_test.rs

#[test]
fn path_param_primitive_arg_with_additional_settings() {
    #[utoipa::path(
        get,
        path = "/items/{id}",
        params(
            ("id", min_length = 1),
        ),
        responses(
            (status = 200, description = "success response")
        )
    )]
    #[allow(unused)]
    async fn get_item(id: Path<String>) {}

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

    let doc = serde_json::to_value(ApiDoc::openapi()).unwrap();
    let operation = doc.pointer("/paths/~1items~1{id}/get").unwrap();

    println!("{:?}", operation.pointer("/parameters"));
    assert_json_eq!(
        operation.pointer("/parameters"),
        json!([
            {
                "in": "path",
                "name": "id",
                "required": true,
                "schema": {
                    "type": "string",
                    "minLength": 1, // without the fix, this property will be missing
                }
            }
        ])
    )
}

As you suggested, the main issue seems to be that some information from the explicit parameter info is overwritten with information extracted from axum:
the merge function in Parameter doesn't update the features fields of parameter_schema and self.

Basically, it needs to repeat these three lines from the Parse function:

utoipa/utoipa-gen/src/path/parameter.rs

Lines 262 to 265 in 1ea9cf8

	parameter.features = (schema_features.clone(), parameter_features);
	if let Some(parameter_schema) = &mut parameter.parameter_schema {
	parameter_schema.features = schema_features;
	}

I'm not sure if we also need to move the features from other.features to self.features first.

The above test case works with this adjusted version of the merge function:

    pub fn merge(&mut self, other: Parameter<'p>) {
        match (self, other) {
            (Self::Value(value), Parameter::Value(other)) => {
                value.parameter_schema = other.parameter_schema;

                // not sure if this is needed:
                let (mut schema_features, mut param_features) = other.features;
                value.features.0.append(&mut schema_features);
                value.features.1.append(&mut param_features);

                // same logic as in Parse():
                if let Some(pf) = &mut value.parameter_schema {
                    pf.features = value.features.0.clone();
                }
            }
            (Self::IntoParamsIdent(into_params), Parameter::IntoParamsIdent(other)) => {
                *into_params = other;
            }
            _ => (),
        }
    }

---

I'd also like some feedback from exploring and fiddling with the code (but I'm aware that I just dove in for some very isolated things, so feel free to ignore this if you think otherwise):
The modeling of the "features" properties was quite confusing for me, and seems to be somewhat fragile.

As a pure refactoring, I recommend making ValueParameter.features easier to understand by splitting the tuple, so each of the two vectors can have its own name. For example, turning it into two fields (schema_features: Vec<Feature>, param_features: Vec<Feature>) could work. Or if the two should remain connected (e.g. sometimes used in an Option), a sub-struct with named parameters would help.

Furthermore, it seems to be errorprone that the schema_features need to be copied into the parameter_schema (but only if parameter_schema is not None?). If possible, it would be good to keep it only in one place.
However, I realize that this might be difficult to do, depending on how the code works around it.

Finally, I wonder if it would make the code easier to understand to split Feature into separate types, one for parameter features and one for schema features. However, I also realize this might make the code more annoying in some places; I especially don't know how this interacts with parsing the macro input.

[ghost]

Hi @juhaku! Happy new year! 🎉

Just wanted to check in, are there any updates on this?
Unfortunately this is blocking us from upgrading to utoipa v4 :(

[juhaku]

Hi @juhaku! Happy new year! 🎉

Just wanted to check in, are there any updates on this? Unfortunately this is blocking us from upgrading to utoipa v4 :(

Hello, and sorry for this taking so long, 😞 I am now taking this under work.

[juhaku]

The above test case works with this adjusted version of the merge function:

   pub fn merge(&mut self, other: Parameter<'p>) {
       match (self, other) {
           (Self::Value(value), Parameter::Value(other)) => {
               value.parameter_schema = other.parameter_schema;

               // not sure if this is needed:
               let (mut schema_features, mut param_features) = other.features;
               value.features.0.append(&mut schema_features);
               value.features.1.append(&mut param_features);

               // same logic as in Parse():
               if let Some(pf) = &mut value.parameter_schema {
                   pf.features = value.features.0.clone();
               }
           }
           (Self::IntoParamsIdent(into_params), Parameter::IntoParamsIdent(other)) => {
               *into_params = other;
           }
           _ => (),
       }
   }

True, that indeed fixes this issue with params, thanks for noticing. That above both is needed since that will be needed in ToTokens implementation ValueParameter to check whether user has defined features for parameter type without defining parameter type itself. Though there is only need to copy features to updated ParameterSchema.