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

feat: discover schema with lifetimes and const generics #42

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

Calum4
Copy link
Contributor

@Calum4 Calum4 commented Sep 8, 2024

Fixed as issue where schema with lifetimes were ignored due to the overly eager .generics.params.is_empty() found here and here.

Disclaimer that I'm not at all familiar with the inner workings of the crate so there may be unintended consequences I've overlooked.

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also run cargo fmt to fix the ci

utoipauto-core/src/discover.rs Outdated Show resolved Hide resolved
@Calum4
Copy link
Contributor Author

Calum4 commented Sep 9, 2024

Apologies! I presumed fmt wasn't in use after running it and seeing a litany of changed files in my diff. Little did I know it was git converting the files to CRLF, sigh.

Should be sorted now

@DenuxPlays
Copy link
Collaborator

I still do not get it.

How does your change add functionality?

You filter the list so it gets smaller how does this is better then a simple .is_empty() on the same unfiltered list?

@Calum4
Copy link
Contributor Author

Calum4 commented Sep 9, 2024

I have created a new branch with the fix removed to demonstrate the problem.

For your convenience, the output of running tests:

thread 'default_features::test::test_lifetimes' panicked at utoipauto\tests\default_features\test.rs:146:14:
no components
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    default_features::test::test_lifetimes

test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `-p utoipauto --test test`

As you can see from this line, if there are any generics on a struct the value of is_generic is true. This includes lifetimes which are considered generic.

Struct with lifetimes

#[derive(ToSchema)]
pub struct LifetimeStructSchema<'a> {
    foo: &'a str,
}

Output of dbg!(&s.generics.params);

[utoipauto-core\src\discover.rs:89:17] &s.generics.params = [
    GenericParam::Lifetime(
        LifetimeParam {
            attrs: [],
            lifetime: Lifetime {
                apostrophe: #143 bytes(7503..7574),
                ident: Ident {
                    ident: "a",
                    span: #143 bytes(7503..7574),
                },
            },
            colon_token: None,
            bounds: [],
        },
    ),
]

Now if we look at the definition of parse_from_attr(), the struct from the example above reaches this evaluation however due to the value of is_generic being true, does not meet the condition and therefore the schema is not discovered.

In order to fix this, my solution is to filter out of s.generics.params any lifetimes. I do this because it is my understanding rather than broadly checking for all generics, your intention is instead to check for type and const generics.

Perhaps I should also rename is_generic to clarify the updated meaning.

@DenuxPlays
Copy link
Collaborator

Ahh okay now I understand.

But then I would rather check if we have any GenericParam::Type as this is the Type of Generic we want.
We do not want to include Lifetimes or Consts.

@Calum4 Calum4 marked this pull request as draft September 9, 2024 19:11
@Calum4
Copy link
Contributor Author

Calum4 commented Sep 9, 2024

I'll take a further look at this when I get time, hopefully tomorrow. Currently this does not compile for whatever reason

error[E0107]: missing generics for struct `ConstGenericStructSchema`
  --> utoipauto\tests\default_features\test.rs:62:1
   |
62 | #[utoipauto(paths = "./utoipauto/tests/default_features")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 1 generic argument
   |
note: struct defined here, with 1 generic parameter: `N`
  --> utoipauto\tests\default_features\const_generics.rs:4:12
   |
4  | pub struct ConstGenericStructSchema<const N: usize> {
   |            ^^^^^^^^^^^^^^^^^^^^^^^^ --------------
   = note: this error originates in the attribute macro `utoipauto` (in Nightly builds, run with -Z macro-backtrace for more info)
help: add missing generic argument
   |
62 | #[utoipauto(paths = "./utoipauto/tests/default_features")]<N>
   |                                                           +++

@DenuxPlays
Copy link
Collaborator

Very weird

I am also enhancing the acceptance tests to cover generics.
If you need any help let me know. :)

@Calum4
Copy link
Contributor Author

Calum4 commented Sep 10, 2024

I've managed to track down the issue.

#[derive(utoipa::ToSchema)]
pub struct ConstGenericStructSchema<const N: usize> {
    foo: [u16; N],
}

The struct above once discovered is internally represented as crate::ConstGenericStructSchema. This once inserted into the openapi attribute is problematic, demonstrated below.

Current output which fails to compile:

#[openapi(components(schemas(crate::ConstGenericStructSchema)))]

Modified output which successfully compiles:

#[openapi(components(schemas(crate::ConstGenericStructSchema<0>)))]

Best I can see, the value of N is totally disregarded in the final openapi representation:

{
  "properties": {
    "foo": {
      "items": {
        "format": "int32",
        "minimum": 0,
        "type": "integer"
      },
      "type": "array"
    }
  },
  "required": [
    "foo"
  ],
  "type": "object"
}

I do not understand the implications of my proposed solution, however I would suggest we instead internally represent the struct as crate::ConstGenericStructSchema<x> where x is the default value of const generic's type. In this case usize::default() -> crate::ConstGenericStructSchema<0>.

I'm happy to code this up, if you deem it an acceptable solution

@DenuxPlays
Copy link
Collaborator

No not at all

Generic schemas are meant to used with the aliases attribute.
This is mentioned in utopia.

@DenuxPlays
Copy link
Collaborator

DenuxPlays commented Sep 10, 2024

OpenApi does not support generic Schemas afaik.
So we have to hard code all the generics via the alias macro sadly.

@Calum4
Copy link
Contributor Author

Calum4 commented Sep 10, 2024

Ah well, it appears my lack of knowledge on utoipauto and utoipa in general has shown itself.

I'll see what I can figure out with this new info, thanks

@DenuxPlays
Copy link
Collaborator

No I find it very confusing too.
You keep saying that you have limited knowledge but you figured out quit a bit 👍

@DenuxPlays
Copy link
Collaborator

Your last commit looks really good on the first glance.
I am working on an acceptance test for generics.

Also I am reworking how special imports are processed.

I have to think how to develop everything in parallel and merge them with one semi-big pr. (Otherwise the acceptance tests will fail!)

But that's a task for tomorrow let me know when you finish this pr.

For now:
Thank you for all your work :)

@Calum4
Copy link
Contributor Author

Calum4 commented Sep 10, 2024

Thank you for all your work :)

No problem at all, thanks for your work on the crates! This has presented me a good excuse to finally learn proc macros

I've managed to get lifetimes, const and type generics all to work nicely in isolation, however when mixing lifetimes with either results in a compile error

From my initial looks it might be a difficult one to solve, but I'll take a stab at it and let you know how it goes

@Calum4

This comment was marked as outdated.

@Calum4 Calum4 marked this pull request as ready for review September 10, 2024 23:08
@Calum4
Copy link
Contributor Author

Calum4 commented Sep 10, 2024

Okay, tests pass and I believe this is good to go. Thanks for your help on this!

@DenuxPlays
Copy link
Collaborator

No thank you for doing this :)

I'll take a look when I find more time.

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@DenuxPlays
Copy link
Collaborator

@ProbablyClem Would be great if you can look at this too.
Maybe I missed something

@Calum4 Calum4 changed the title feat: parse schema with lifetimes feat: discover schema with lifetimes and const generics Sep 11, 2024
@DenuxPlays
Copy link
Collaborator

Okay
The unit tests work for now.
I will later add an acceptance test.

@Calum4 Thank you for your work :)

@DenuxPlays DenuxPlays merged commit b7d8525 into ProbablyClem:main Sep 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants