Do not inline non-primitive type parameters by default #1184
Conversation
|
@juhaku I attempted to fix the Here is the difference: 43c43
< "type": "object"
---
> "type": "string"
97a98,123
> "properties": {
> "accounts": {
> "items": {
> "oneOf": [
> {
> "type": "null"
> },
> {
> "$ref": "#/components/schemas/Account"
> }
> ]
> },
> "type": "array"
> },
> "foo_bar": {
> "$ref": "#/components/schemas/Foobar"
> },
> "name": {
> "type": "string"
> }
> },
> "required": [
> "name",
> "foo_bar",
> "accounts"
> ],I think the reference is wrong:
Should I fix the reference? |
|
True it is indeed wrong, I definitely would appreciate if you want to take the initiative to enter to the rabbit hole of generics and fix the broken references as well. 😆 For now I have no idea as of yet that what is causing that misalignment. |
|
But maybe we can just leave it as is. Like without the |
@juhaku that's the neat part : my PR fixes it AFAIK. But I just wanted your approval before changing the reference JSON.
My point exactly. The result with those changes is IMO better and fixes that broken test/reference. So if I just change the reference JSON:
But you can't have 2. without 3. So if that's ok with you I will:
|
|
Yes, that is ok for me, thanks for the work 🙂 👍 |
|
If I check only with Of course, we do not want refs to known/native generics. So I will add a test for |
|
Sure, it also seems that the it still creates a reference to diff --git a/utoipa-gen/tests/testdata/openapi_schemas_resolve_inner_schema_references b/utoipa-gen/tests/testdata/openapi_schemas_resolve_inner_schema_references
index 7368bc5..718e792 100644
--- a/utoipa-gen/tests/testdata/openapi_schemas_resolve_inner_schema_references
+++ b/utoipa-gen/tests/testdata/openapi_schemas_resolve_inner_schema_references
@@ -28,7 +28,7 @@
{
"properties": {
"One": {
- "type": "string"
+ "$ref": "#/components/schemas/String"
}
},
"required": [
@@ -40,7 +40,7 @@
"properties": {
"Many": {
"items": {
- "type": "object"
+ "$ref": "#/components/schemas/String"
},
"type": "array"
}
@@ -57,33 +57,7 @@
{
"properties": {
"One": {
- "properties": {
- "accounts": {
- "items": {
- "oneOf": [
- {
- "type": "null"
- },
- {
- "$ref": "#/components/schemas/Account"
- }
- ]
- },
- "type": "array"
- },
- "foo_bar": {
- "$ref": "#/components/schemas/Foobar"
- },
- "name": {
- "type": "string"
- }
- },
- "required": [
- "name",
- "foo_bar",
- "accounts"
- ],
- "type": "object"
+ "$ref": "#/components/schemas/Yeah"
}
},
"required": [
@@ -95,7 +69,7 @@
"properties": {
"Many": {
"items": {
- "type": "object"
+ "$ref": "#/components/schemas/Yeah"
},
"type": "array"
}
@@ -231,6 +205,9 @@
],
"type": "object"
},
+ "String": {
+ "type": "string"
+ },
"ThisIsNone": {
"default": null
},
|
@juhaku weird, that change should fix it: |
|
Yup that fixes, it but if you remove the |
JMLX42
force-pushed
the
fix/generic-type-param-inlining
branch
from
872f17c
to
fcb0ae7
Compare
Correct. But if you add the |
|
Great |
|
The only failing test AFAIK is 43c43
< "type": "string"
---
> "type": "object"
98,123d97
< "properties": {
< "accounts": {
< "items": {
< "oneOf": [
< {
< "type": "null"
< },
< {
< "$ref": "#/components/schemas/Account"
< }
< ]
< },
< "type": "array"
< },
< "foo_bar": {
< "$ref": "#/components/schemas/Foobar"
< },
< "name": {
< "type": "string"
< }
< },
< "required": [
< "name",
< "foo_bar",
< "accounts"
< ],And I'm a bit puzzled by this one... |
JMLX42
force-pushed
the
fix/generic-type-param-inlining
branch
from
fcb0ae7
to
dce8e51
Compare
I have created a separate test: |
|
True, that is a bit weird, I am trying to debug it as well. |
|
Could it be specific to how enum schemas are handled? |
|
FYI here is the expanded code: mod response {
use std::ops::Deref;
use utoipa::{openapi::schema::ArrayItems, ToSchema};
pub enum Element<T> {
One(T),
Many(Vec<T>),
}
impl<T> utoipa::__dev::ComposeSchema for Element<T>
where
T: utoipa::ToSchema,
{
fn compose(
mut generics: Vec<utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>>,
) -> utoipa::openapi::RefOr<utoipa::openapi::schema::Schema> {
Into::<
utoipa::openapi::schema::OneOfBuilder,
>::into(utoipa::openapi::OneOf::with_capacity(2usize))
.item(
utoipa::openapi::schema::Object::builder()
.property(
"One",
{
let _ = <T as utoipa::PartialSchema>::schema;
if let Some(composed) = generics.get_mut(0usize) {
std::mem::take(composed)
} else {
utoipa::openapi::schema::RefBuilder::new()
.ref_location_from_schema_name(
::alloc::__export::must_use({
let res = ::alloc::fmt::format(
format_args!("{0}", <T as utoipa::ToSchema>::name()),
);
res
}),
)
.into()
}
},
)
.required("One"),
)
.item(
utoipa::openapi::schema::Object::builder()
.property(
"Many",
utoipa::openapi::schema::ArrayBuilder::new()
.items({
let _ = <T as utoipa::PartialSchema>::schema;
if let Some(composed) = generics.get_mut(0usize) {
std::mem::take(composed)
} else {
utoipa::openapi::schema::RefBuilder::new()
.ref_location_from_schema_name(
::alloc::__export::must_use({
let res = ::alloc::fmt::format(
format_args!("{0}", <T as utoipa::ToSchema>::name()),
);
res
}),
)
.into()
}
}),
)
.required("Many"),
)
.into()
}
}
impl<T> utoipa::ToSchema for Element<T>
where
T: utoipa::ToSchema,
{
fn name() -> std::borrow::Cow<'static, str> {
std::borrow::Cow::Borrowed("Element")
}
fn schemas(
schemas: &mut Vec<
(String, utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>),
>,
) {
schemas.extend([]);
<T as utoipa::ToSchema>::schemas(schemas);
<T as utoipa::ToSchema>::schemas(schemas);
}
}
} |
Could be, but hard to tell right away. |
fn schemas(
schemas: &mut Vec<
(String, utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>),
>,
) {
schemas.extend([]);
<T as utoipa::ToSchema>::schemas(schemas);
<T as utoipa::ToSchema>::schemas(schemas);
}Not sure if it's related, but the Otherwise the code generated for the |
Yeah, it should not be an issue, but could also be fixed though, it in theory should result same spec but just redundantly calls the compose twice. There is no check whether the same reference call is already placed so it blindly adds schema reference call for each variant of the enum. Perhaps this could be fixed to the enum schema references implementation. |
|
Alignment aside, the generated code is the same for the
Could it be that |
Could be, as what let v = <Element<Yeah> as utoipa::__dev::ComposeSchema>::compose(
[<Yeah as utoipa::PartialSchema>::schema()].to_vec(),
);
dbg!(&v);This will result the invalid code for somehow. |
|
It might have something to do with the impl Default for ArrayItems {
fn default() -> Self {
Self::RefOrSchema(Box::new(Object::with_type(SchemaType::AnyValue).into()))
}
} |
IMO that impl ComposeSchema for String {
fn compose(_: Vec<utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>>) -> utoipa::openapi::RefOr<utoipa::openapi::schema::Schema> {
schema!( String ).into()
}
}But I cannot find the |
My bad. In the broken case, |
I've added a |
But changing its impl does not change the outcome of the test: impl Default for ArrayItems {
fn default() -> Self {
dbg!("ArrayItems::default");
Self::False
}
}So IMO the problem is elsewhere. No? |
|
Now I know what happens 🤦 the problem is |
That makes perfect sense. Why taking away the generic in the first place? Shouldn't that trigger the same problem for a struct with multiple field typed with the same type parameter? |
|
The problem is here: |
|
If I change the implementation to this: diff --git a/utoipa-gen/src/component.rs b/utoipa-gen/src/component.rs
index 70fb0a5..f6f1f68 100644
--- a/utoipa-gen/src/component.rs
+++ b/utoipa-gen/src/component.rs
@@ -1228,7 +1228,8 @@ impl ComponentSchema {
}
} else {
quote_spanned! {type_path.span()=>
- <#rewritten_path as utoipa::PartialSchema>::schema()
+ // <#rewritten_path as utoipa::PartialSchema>::schema()
+ <#rewritten_path as utoipa::__dev::ComposeSchema>::compose(generics.clone())
}
};
object_schema_reference.tokens = items_tokens.clone();I get following error: 1. the trait bound `P: utoipa::__dev::ComposeSchema` is not satisfied
the trait `utoipa::__dev::ComposeSchema` is not implemented for `P` [E0277]Because This kind of gets to point where the generics implementation in general needs some adjustment e.g. it should go impl<T: Sized, P> utoipa::__dev::ComposeSchema for Person<T, P>
where
T: utoipa::ToSchema,
P: utoipa::ToSchema,
{
fn compose(
mut generics: Vec<utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>>,
) -> utoipa::openapi::RefOr<utoipa::openapi::schema::Schema> {
{
let mut object = utoipa::openapi::ObjectBuilder::new();
object = object
.property("field", {
let _ = <T as utoipa::PartialSchema>::schema;
if let Some(composed) = generics.get_mut(0usize) {
composed.clone()
} else {
utoipa::openapi::schema::RefBuilder::new()
.ref_location_from_schema_name(format!(
"{}",
<T as utoipa::ToSchema>::name()
))
.into()
}
})
.required("field");
object = object
.property(
"t",
<P as utoipa::__dev::ComposeSchema>::compose(generics.clone()),
)
.required("t");
object
}
.into()
}
} |
|
Another problem: I did all of this because I wanted my responses to properly use ref for there type parameter. Here is my code: #[derive(Debug, Serialize, ToSchema)]
pub struct SuccessResponsePayload<T: ResourceObject> {
#[schema(inline = false)]
pub data: Vec<T>,
#[schema(ignore = Self::ignore_included)]
#[serde(skip_serializing_if = "Option::is_none")]
pub included: Option<Vec<T::RelationshipValue>>,
#[schema(inline)]
pub links: LinksObject<T>,
}
impl<T: ResourceObject> SuccessResponsePayload<T> {
fn ignore_included() -> bool {
!T::HAS_RELATIONSHIPS
}
}
#[derive(Debug, Serialize, IntoResponses, ToSchema)]
#[serde(untagged)]
pub enum Response<T: ResourceObject> {
#[response(
status = 204,
content_type = CONTENT_TYPE,
description = "Success response with no content.",
)]
Empty,
#[response(
status = 200,
content_type = CONTENT_TYPE,
description = "Success response."
)]
Success(#[to_schema] SuccessResponsePayload<T>),
#[response(
status = "4XX",
content_type = CONTENT_TYPE,
description = "Client error response.",
example = json!(EXAMPLE_ERROR_NOT_FOUND)
)]
ClientError(#[to_schema] ClientErrorResponse),
#[response(
status = "5XX",
content_type = CONTENT_TYPE,
description = "Server error response.",
example = json!(EXAMPLE_ERROR_INTERNAL_SERVER_ERROR)
)]
ServerError(#[to_schema] ServerErrorResponse),
}And yet I get: "responses": {
"200": {
"description": "Success response.",
"content": {
"application/vnd.api+json": {
"schema": {
"type": "object",
"required": [
"data",
"links"
],
"properties": {
"data": {
"type": "array",
"items": {
"allOf": [
{
"$ref": "#/components/schemas/AccessorSparseIndicesResourceIdentifierObject"
},
{
"type": "object",
"required": [
"attributes",
"relationships"
],
"properties": {
"attributes": {
"$ref": "#/components/schemas/AccessorSparseIndicesResourceSparseAttributesObject"
},
"relationships": {
"$ref": "#/components/schemas/AccessorSparseIndicesResourceSparseRelationshipsObject"
}
}
}
],
"description": "An object pointing to a buffer view containing the indices of deviating accessor values. The number of indices is\nequal to `accessor.sparse.count`. Indices **MUST** strictly increase."
}
},For some reason,
The #[derive(ToSchema)]
#[schema(as = path::MyType<T>)]
struct Type<T> {
#[schema(inline)] // <= this is the change
t: T,
}
#[derive(ToSchema)]
struct Person<T: Sized, P> {
field: T,
#[schema(inline)]
t: P,
} |
|
Yeah those
That works because the |
|
I pressed the wrong button 🤦 |
No problem. I'll open a separate PR for that. It should be easy enough when this is done.
Can the generated code check if the type is in |
That's probably the expected goal of But the same check is not performed for the |
utoipa/utoipa-gen/src/component.rs Line 1220 in c45215c The check is actually here it checks whether it has chidren, but in above case at that point it sees P as type which does not have children and not Type<i32>.
|
Yes that's what I figured. But that's done at the macro time, not at runtime. Can't we have a better check at runtime using |
I mean at some point, some code generates a |
Yeah, not sure whether there is any way to actually get around this. But when the
object = object
.property("t", {
let _ = <P as utoipa::PartialSchema>::schema;
if let Some(composed) = generics.get_mut(1usize) {
composed.clone()
} else {
utoipa::openapi::schema::RefBuilder::new()
.ref_location_from_schema_name(format!(
"{}",
<P as utoipa::ToSchema>::name()
))
.into()
}
})
.required("t"); |
|
What's more is that when type type is E.g. wrapped with another type like in below example. If it is inlined with #[derive(ToSchema)]
struct Wrapper {
value: i32
}
#[derive(ToSchema)]
struct Person<T: Sized, P> {
field: T,
#[schema(inline)]
t: P,
} |
|
If we change the diff --git a/utoipa-gen/src/component.rs b/utoipa-gen/src/component.rs
index 70fb0a5..216e22d 100644
--- a/utoipa-gen/src/component.rs
+++ b/utoipa-gen/src/component.rs
@@ -1227,8 +1227,20 @@ impl ComponentSchema {
<#rewritten_path as utoipa::__dev::ComposeSchema>::compose(#composed_generics.to_vec())
}
} else {
- quote_spanned! {type_path.span()=>
- <#rewritten_path as utoipa::PartialSchema>::schema()
+ let index = container.generics.get_generic_type_param_index(type_tree);
+ dbg!("inline got index", index, &rewritten_path);
+ if index.is_some() {
+ quote_spanned! {type_path.span()=>
+ if let Some(composed) = generics.get(#index) {
+ composed.clone()
+ } else {
+ <#rewritten_path as utoipa::PartialSchema>::schema()
+ }
+ }
+ } else {
+ quote_spanned! {type_path.span()=>
+ <#rewritten_path as utoipa::PartialSchema>::schema()
+ }
}
};
object_schema_reference.tokens = items_tokens.clone(); |
|
@juhaku how about:
? |
|
@JMLX42 I guess that is what we need to do. Otherwise this will never end. 😆 |
JMLX42
force-pushed
the
fix/generic-type-param-inlining
branch
from
4ca70bd
to
9298bd7
Compare
JMLX42
force-pushed
the
fix/generic-type-param-inlining
branch
from
9298bd7
to
f292479
Compare
|
|
@juhaku ready for review! |
|
I guess with this the |
Fixes #1182