Skip to content
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

Unable to create schema with const generic param #1115

Closed
DenuxPlays opened this issue Oct 12, 2024 · 11 comments · Fixed by #1118 or #1120
Closed

Unable to create schema with const generic param #1115

DenuxPlays opened this issue Oct 12, 2024 · 11 comments · Fixed by #1118 or #1120
Labels
investigate Futher investigation needed before other action

Comments

@DenuxPlays
Copy link

Hey,
since a few commits (idk exactly when) the ToSchema derive for generic schemas with a constant generic is broken.
Example:
https://github.com/ProbablyClem/utoipauto/pull/45/files#diff-93ada2911f7e76502c64197e27153dc94a87ed57311b0643fd310d176ce93145R37

@juhaku
Copy link
Owner

juhaku commented Oct 12, 2024

Okay, I need to investigate this.

@juhaku juhaku added the investigate Futher investigation needed before other action label Oct 12, 2024
@juhaku
Copy link
Owner

juhaku commented Oct 12, 2024

Well this is a bit unfortunate as the const generics cannot be used anymore with ToSchema types.

#[derive(ToSchema)]
pub struct ArrayResponse<T: ToSchema, const N: usize> {
    array: [T; N],
}

// for this type the below is what gets generated. 
// #[derive(ToSchema)]
struct CombinedResponse<T: ToSchema, const N: usize> {
    pub array_response: ArrayResponse<T, N>,
}

This is a bit problematic since it will generate the schema with const N but a const cannot be used as type. And that is exactly what the rust compiler is complaining about. Below code I have marked the code points that are problematic. Since we are heavily leveraging traits in order to fully support generics (as much as possible) and automatic schema collection we need to be able to call each generic arguments schema and name dynamically upon their usage.

Perhaps we could stringify the const generic argument and use it as it's name. But we cannot get the schema for a const argument.

impl<T: ToSchema, const N: usize> utoipa::__dev::ComposeSchema for CombinedResponse<T, N>
where
    T: utoipa::ToSchema,
{
    fn compose(
        mut generics: Vec<utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>>,
    ) -> utoipa::openapi::RefOr<utoipa::openapi::schema::Schema> {
        utoipa::openapi::ObjectBuilder::new()
            .property(
                "array_response",
                utoipa::openapi::schema::RefBuilder::new().ref_location_from_schema_name(
                    std::borrow::Cow::Owned(format!(
                        "{}_{}",
                        <ArrayResponse<T, N> as utoipa::ToSchema>::name(),
                        std::borrow::Cow::<String>::Owned(
                            [
                                <T as utoipa::ToSchema>::name(),
                                <N as utoipa::ToSchema>::name(), // <-- for const this is not possible
                            ]
                            .to_vec()
                            .join("_")
                        )
                    )),
                ),
            )
            .required("array_response")
            .into()
    }
}

impl<T: ToSchema, const N: usize> utoipa::ToSchema for CombinedResponse<T, N>
where
    T: utoipa::ToSchema,
{
    fn name() -> std::borrow::Cow<'static, str> {
        std::borrow::Cow::Borrowed("CombinedResponse")
    }
    fn schemas(
        schemas: &mut Vec<(
            String,
            utoipa::openapi::RefOr<utoipa::openapi::schema::Schema>,
        )>,
    ) {
        schemas.extend([(
            String::from(std::borrow::Cow::Owned(format!(
                "{}_{}",
                <ArrayResponse<T, N> as utoipa::ToSchema>::name(),
                std::borrow::Cow::<String>::Owned(
                    [
                        <T as utoipa::ToSchema>::name(),
                        <N as utoipa::ToSchema>::name(),  // <-- for const this is not possible
                    ]
                    .to_vec()
                    .join("_")
                )
            ))),
            <ArrayResponse<T, N> as utoipa::__dev::ComposeSchema>::compose(
                [
                    <T as utoipa::PartialSchema>::schema(),
                    <N as utoipa::PartialSchema>::schema(),  // <-- for const this is not possible
                ]
                .to_vec(),
            ),
        )]);
        <ArrayResponse<T, N> as utoipa::ToSchema>::schemas(schemas);
    }
}

I am not sure whether this kind of situation is very common for users, but I am afraid that there is no way around this and this comes as mandatory caveat in order to support more dynamic generics and schema collection.

EDIT:
Though as the const generic is not used anywhere in the schema. of ArrayResponse we could just ignore the const argument totally and it will work. I'll try to publish a PR soon.

@DenuxPlays
Copy link
Author

Okay
But couldn't we find the Actual Type of the Const generic ( in this case usize)?

The actual code is just a Test case to Test utoipauto Crate.
So ignoring it will Compile our Tests but will not actually resolve this issue.

@juhaku
Copy link
Owner

juhaku commented Oct 12, 2024

But couldn't we find the Actual Type of the Const generic ( in this case usize)?

In theory we could find the type bound from the container type's generic params.

True, but const generics have not been supported anyways, they have been ignored throughout the whole lifespan of utoipa. Because the const generic represents actually a value but not a type itself. E.g. if we have the following. The const N: usize while being usize it represents a value and in this case the size of an array. The usize type, while could be resolved, does not affect the schema of a type, the schema will still be an array of T without any knowledge of the N size restrictions.

struct Array<T, const N: usize> {
    array: [T; N],
}

Still resolving the type could be a way forward and perhaps is one step closer to support const generics (if possible) deemed necessary / useful.

This seems to be bit more evolved thus I'll try to work on the PR as a next thing, but it wont be quick fix.

If we resolve the usize in above case, we most likely cannot use it in schema anyways, unless we have access to the value upon usage, This still would need some experimenting whether we could resolve upon usage size limit for an array on the generated schema. This size limit then could be used to tell array min_items and max_items value perhaps.

@DenuxPlays
Copy link
Author

That Sounds good.
But I think this issue has not a high priority (but again I cannot speak for the whole community).
Maybe this is a thing for a 5.x Release.

For now a quick fix would be to ignore the const so that it Compiles and document it nicely

@juhaku
Copy link
Owner

juhaku commented Oct 12, 2024

Yup, I will ignore the param for now and start preparing for 5.0.0 release. The const generic can be experimented in 5.x later on.

juhaku added a commit that referenced this issue Oct 12, 2024
This commit will filter out const generics from being added to the
schema composing because the value is not used in anyway.

Perhaps in some future release the real support of const generics could
be experimented.

Fixes #1115
juhaku added a commit that referenced this issue Oct 12, 2024
This commit will filter out const generics from being added to the
schema composing because the value is not used in anyway.

Perhaps in some future release the real support of const generics could
be experimented.

Fixes #1115
juhaku added a commit that referenced this issue Oct 12, 2024
This commit will filter out const generics from being added to the
schema composing because the value is not used in anyway.

Perhaps in some future release the real support of const generics could
be experimented.

Fixes #1115
@juhaku
Copy link
Owner

juhaku commented Oct 12, 2024

@DenuxPlays There is PR #1118 which fixes this by ignoring the const generics allowing the build to compile.

juhaku added a commit that referenced this issue Oct 12, 2024
This commit will filter out const generics from being added to the
schema composing because the value is not used in anyway.

Perhaps in some future release the real support of const generics could
be experimented.

Fixes #1115
@DenuxPlays
Copy link
Author

Not really fixed:

image

I've updated the mr where the acceptance tests are failing: ProbablyClem/utoipauto#45

@DenuxPlays
Copy link
Author

@juhaku This needs to be reopened

@juhaku juhaku reopened this Oct 13, 2024
@juhaku
Copy link
Owner

juhaku commented Oct 13, 2024

Oh man, that likely needs some further experimenting around the const generics.

juhaku added a commit that referenced this issue Oct 14, 2024
This commit fixes path rewrite which previously ignored other that Type
generic arguments totally from the path. This changes it to rewrite only
the Type generic arguments as previously while keeping the other generic
arguments as is in the path.

Fixes #1115
juhaku added a commit that referenced this issue Oct 14, 2024
This commit fixes path rewrite which previously ignored other that Type
generic arguments totally from the path. This changes it to rewrite only
the Type generic arguments as previously while keeping the other generic
arguments as is in the path.

Fixes #1115
juhaku added a commit that referenced this issue Oct 14, 2024
This commit fixes path rewrite which previously ignored other that Type
generic arguments totally from the path. This changes it to rewrite only
the Type generic arguments as previously while keeping the other generic
arguments as is in the path.

Fixes #1115
@juhaku
Copy link
Owner

juhaku commented Oct 14, 2024

Finally fixed it "again", I had a half baked implementation of path rewrite in place which did ignore all other generic arguments type Type generic arguments. #1120

juhaku added a commit that referenced this issue Oct 14, 2024
This commit fixes path rewrite which previously ignored other that Type
generic arguments totally from the path. This changes it to rewrite only
the Type generic arguments as previously while keeping the other generic
arguments as is in the path.

Fixes #1115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Futher investigation needed before other action
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants