juhaku / utoipa

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

OpenAPI schema generation silently allows missing schemas #465

Open jayvdb opened 1 year ago

jayvdb commented 1 year ago

I am using the OpenAPI schema generation, and the generated OAS doc for the following includes schema SchemaA correctly, however it refers to SchemaB which isn't present in the generated OAS, making it invalid. There should be some failure to indicate that SchemaA cant be included in the OAS without SchemaB.

#[derive(OpenApi)]
#[openapi(
    security(
        ("api_key" = [])
    ),

    paths(
        foo::bar,
    ),
    components(
        schemas(
            SchemaA,
        ),
    )
)]
pub struct ApiDoc;

Adding SchemaB in the above solves the problem.

This also occurred in the 2.x release, so it isnt a regression.

juhaku commented 1 year ago

This is something that indeed needs some thought. That is, there is currently no way to know what schemas have been added to the OpenAPI and what are not. I am planning to investigate possibility to automatically add these to the schema but there are no guarantees whether it is possible in any feasible way.

Difficulty here comes from that OpenApi derive macro does not know and it has no access to the internals of the types so it does not keep track on what has been added and whether some schema depends on another schema.

Only thing I can think of is if ToSchema types are stored in one central location and then the check could be done within ToSchema derive macro instead by checking whether schema was added there or not. Then the macro can emit error for that specific field itself.

Yet if is possible have schemas automatically added to the OpenApi then there would be no need for this kind of check.

But if possible it would be good to have such an safety mechanism to check whether schema is correct or not.

djrenren commented 1 year ago

I think what you'd have to do is thread a registry of some sort, through the path_item invocations (or have a separate function that you generate like: schemas(reg: &mut Registry). The registry is essentially just a HashMap<std::any::TypeId, Schema>. Then when you build the #[openapi] you'll need to invoke the schemas fn for all the paths involved. You'd also need a similar schemas function for things that implement IntoParams and IntoResponses. Those functions would be invoked by the schemas function for the path.

djrenren commented 1 year ago

I guess this could be less awful by just having a trait HasSchemas and impl it for everything that would ref or contain a schema.

juhaku commented 1 year ago

Yeah, I was considering something like a centralized store for ToSchema and other derive macro results as well what then could added to the added to the OpenApi.

I guess this could be less awful by just having a trait HasSchemas and impl it for everything that would ref or contain a schema.

@djrenren Could you elaborate a bit on this. If this trait would be implemented in compile time same time with ToSchema when it should be called? I guess somewhere before the Modify trait is being called.

djrenren commented 1 year ago

yeah basically right here:

https://github.com/juhaku/utoipa/blob/2986e5aa285c3a562f7c6c5d39c39a4e0984f8c8/utoipa-gen/src/openapi.rs#L442

I'm not sure what would happen if there were conflicts between manually described components and auto-discovered ones. probably would just want to error out at instantiation.

juhaku commented 1 year ago

Yeah, here it is a runtime panic and error cannot be targeted to specific span. Unless the spans are collected as well. Though the question remains that how to deal with manually defined components.

Kinrany commented 1 year ago

The problem would also be solved by collecting all the types that need to be referenced in components automatically. Is that possible?

juhaku commented 1 year ago

The problem would also be solved by collecting all the types that need to be referenced in components automatically. Is that possible?

Yes that is possible, but there is a but. That only will work to the extend of single crate. I have been planning to do something like this for other purposes and it could be leveraged here as well but I haven't got a hold of it yet. While I haven't tested it with cross crate references I don't belive it will work because they are seprate processes does not know anything about each others existense. In such cases there still is a need to manually point a schema location what to register as a schema. This kind of situation is very common when API is bigger than a simple demo API.

But if cross crate references are possible, that would make it a lot simplier. Perhaps I need to test it out sometime.

Kinrany commented 1 year ago

I don't think the API should use any kind of global registration mechanism, to be clear. It should be enough to use the recursive nature of types with derived traits. If we could somehow collect types from the endpoint handler functions (which we already list out for schema declarations), that should allow for many schemas to exist in parallel.

jayvdb commented 1 year ago

Note https://github.com/rxdiscovery/utoipa_auto_discovery by @rxdiscovery is planning to solve this, but hasnt done so yet.

qrilka commented 3 months ago

I found a similar problem with requestBody in path: type was renamed but utoipa doesn't complain about that in any way.

jfrader commented 1 month ago

Is there anyway to make this complain now ? Its very hard to trust code reviews would catch this before its consumed by another project like in my case. I even tried to use a different library just for validation on the outputted JSON schema but failed. Maybe someone has a better idea?