-
Notifications
You must be signed in to change notification settings - Fork 10
V0.2.0 preparation / Utoipa v5 support #45
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
Conversation
preparation for v0.2.0 as we have to rework the whole generic feature
Note: for now as we will create new ones when the core is restructured
Updated utoipa
Test fail because the |
Still some weird things that needs investigating: Without the new |
As long as responses (including their schemas) are not being auto-discovered we cannot remove schema discovery/put it behind a feature flag. This will not stop or impact the development just a note. |
@ProbablyClem I think this is ready. Can you please take a look at it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR thanks :
Thank you :) I would merge this pr. |
if !generic_params.is_empty() { | ||
return out; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenuxPlays is it intentional that this means that structs that derive ToResponse
but have a lifetime will no longer be detected? We have some code that looks like
#[derive(Debug, Clone, Serialize, ToSchema, ToResponse)]
pub(crate) struct TopicInfo<'a> {
#[schema(schema_with = kind_schema)]
topics: HashMap<&'a str, Kind>,
}
which we use to avoid allocating Strings
for these topics that are going to be serialized anyways as part of the response. In [email protected]
, this was discovered by utoipauto
and added to the #[openapi]
directive - we just had responses( (status = StatusCode::OK, response = TopicInfo) )
on the #[utoipa::path]
. In 0.2
, TopicInfo
disappears from the schema.
If I put (status = StatusCode::OK, description = "A list of known topics.", body = TopicInfo)
as the responses
instead, it shows up (only in the schemas
part) - unsure if that's utoipauto
or the new schema discovery in utoipa@5
. But I have also confirmed that if I use the old condition for the early return instead, TopicInfo
is included in the responses
as well.
Would there be any issues with only using the new, more strict check for schemas but keep the old check for responses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont really remember this code tbh.
But I think lifetimes aren't supported by utoipa in v5?
If they are then they should be auto discovered.
If there is a bug in the auto-discovery feature feel free to open a pr to fix it or an issue to document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think the code is faulty.
We should check for explicit generic parameters excluding lifetimes.
But I remember that something like this has already be done or came up?
Idk really remember tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you mean by "not supported"? Both ToSchema
and ToResponse
work fine with, for example, MyType<'a>
containing a &'a str
(which is correctly represented as schema type "string"). So as long as the type is part of the OpenAPI declaration at all, this has been working fine even in v4. Or maybe I'm missing something?
utoipa
very much claims support for generics in their README (as long as you use the derives). juhaku/utoipa#1034 also includes a type with a lifetime parameter in the examples, so I'd expect this to "just work" (except for maybe const generics) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait my memory is coming back.
We do this because schemas are getting autodiscovered.
Our Problem is that we cannot predict which type of generic you use.
I think that Utoipa converts this SomeType<String>
to SomeTypeString
.
So basically what the aliases macro did in v4.
In v4 we parsed the aliases macro and added all the types but this isn't possible in v5.
So the real issue is this:
juhaku/utoipa#1094
I am not sure if we can develop a work-around for now.
Atleast I don't have the time to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why do we need to predict anything here? Can't we (syn
) look at what kind of generic the macro is parsing?
What you're saying about the name change seems right (except that I think it becomes SomeType_String
), but I can't see how this is particularly related to lifetimes, which should just be ignored by the name conversion?
FWIW, I also opened #50 which just reverts the discovery check. It doesn't do anything about previously having handling for aliases
, honestly I haven't ever used it so I'm not sure what we would need to be doing there either.
The main goal is to get 100% compatible with utoipa v5.
closes: #44
TODOs
Changelog
Breaking
1.75.0
generic_full_path
featureAdded
Changes
Removed