juhaku / utoipa

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

GenericType resolution inspecificity #903

Closed LockedThread closed 4 months ago

LockedThread commented 4 months ago

Recently, I was trying to integrate utoipa into my project and ran into an issue. See below for an example.

Example

Code

#[derive(Deserialize, Serialize, ToSchema)]
pub struct Location {
    pub map: Map,
}

#[derive(Deserialize, Serialize, ToSchema)]
pub struct Map {
  pub id: u32
}

Error

error: proc-macro derive panicked
  --> apps/api/src/http/models.rs:73:41
   |
73 | #[derive(Debug, Deserialize, Serialize, ToSchema)]
   |                                         ^^^^^^^^
   |
   = help: message: ComponentSchema Map type should have children

This error occurs because of the get_generic_type function using only the name of the type, not the actual type. See here: https://github.com/juhaku/utoipa/blob/613b3ad5bf67978cd66b9464c5127be7aebe378d/utoipa-gen/src/component.rs#L272.

The most simple solution that would work for this specific edge case is to ensure that the segment passed into the get_generic_type function is actually for a generic type. However, if my Map struct was a generic type that fix would not work.

I looked into how we could do import introspection, but there are a lot of edge cases to consider when implementing that. This is my first time using the syn library, so I don't feel confident implementing that portion by myself.

If anyone has a better solution please feel free to contribute. In the mean time I am making a PR with my proposed half-baked solution.

juhaku commented 4 months ago

I don't have any better options for the fix. I believe that this is good enough fix just to check whether the type actually has arguments. Though just came to my mind that it would probably be good to check whether there actually is type arguments, e.g. ignoring the lifetimes and consts, but that can be another PR if there is need to. For now it can be as is IMO.

LockedThread commented 4 months ago

I don't have any better options for the fix. I believe that this is good enough fix just to check whether the type actually has arguments. Though just came to my mind that it would probably be good to check whether there actually is type arguments, e.g. ignoring the lifetimes and consts, but that can be another PR if there is need to. For now it can be as is IMO.

Alright, sounds good. Do you want to keep this open for tracking future changes or do you think my PR solved it?

juhaku commented 4 months ago

Alright, sounds good. Do you want to keep this open for tracking future changes or do you think my PR solved it

This can be closed. In future if there is a need, one can open a new issue. (Need to get the list of open issues down quite a bit :laughing: )