ProbablyClem / utoipauto

Rust Macros to automate the addition of Paths/Schemas to Utoipa crate, simulating Reflection during the compilation phase
Apache License 2.0
108 stars 10 forks source link

Add support for generic schemas #19

Closed DenuxPlays closed 6 months ago

DenuxPlays commented 6 months ago

TODOs:

Full module path

DenuxPlays commented 6 months ago

@ProbablyClem I need your help on a decision. With the current implementation the user gets force to either provide the full path in aliases or import it where the utoipauto macro gets used. I could get the full path but this would require tracking every import for every struct that implements ToSchema and this will result in a lot higher memory usage during compile times (which will get worse and worse with more and more schemas)

DenuxPlays commented 6 months ago

With the second approach we would keep compatibility with utoipa otherwise not

DenuxPlays commented 6 months ago

@ProbablyClem I need your help on a decision. With the current implementation the user gets force to either provide the full path in aliases or import it where the utoipauto macro gets used. I could get the full path but this would require tracking every import for every struct that implements ToSchema and this will result in a lot higher memory usage during compile times (which will get worse and worse with more and more schemas)

This will also add more compile overhead.

I would add a feature flag do enable this and clearly state this in the documentation!

ProbablyClem commented 6 months ago

How would the first approach looks like ? if i do this #[aliases(GenricModelSchema = GenericSchema<crate::models::ModelSchema>)] it doesn't work.

Otherwise, I'd rather avoid the memory consomption overhead but I we had no choice a feature flag is a good idea

DenuxPlays commented 6 months ago

How would the first approach looks like ?

if i do this #[aliases(GenricModelSchema = GenericSchema<crate::models::ModelSchema>)] it doesn't work.

Otherwise, I'd rather avoid the memory consomption overhead but I we had no choice a feature flag is a good idea

At the moment the user can decide if the wants to apply the full module path in the aliases macro or if he wants to import it in their main file (or where ever utoipauto is)

I am working on a feature flag with minimal (but some) overhead. Scanning the imports and processing them in a way we can use them but really really want to keep this behind a feature flag

DenuxPlays commented 6 months ago

How would the first approach looks like ? if i do this #[aliases(GenricModelSchema = GenericSchema<crate::models::ModelSchema>)] it doesn't work.

Ok I see. In this case we have to import it via the main file because we cannot depend on ourself

DenuxPlays commented 6 months ago

Okay nvm The test setup seems to be scuffed. I can only access other module from the test.rs file

DenuxPlays commented 6 months ago

@ProbablyClem Hey, I am currently struggeling with the whole project. I have my test struct defined like this:

#[utoipauto(paths = "./tests/generic_full_path")]
#[derive(OpenApi)]
#[openapi(info(title = "Percentage API", version = "1.0.0"))]
pub struct CrateApiDocs;

But he scans every file (even src/lib.rs) but not the these in the generic_full_path folder? image

Do you know why this would happen?

I have not changed anything about what files get loaded etc.

DenuxPlays commented 6 months ago

Command I execute:

cargo test generic_full_path --all-features
DenuxPlays commented 6 months ago

Ah fixed it

DenuxPlays commented 6 months ago

I updated the description and they are only two things left to do. I also tested this feature on my project (where I actually need it) to make sure it works in production.

DenuxPlays commented 6 months ago

The last commit already added a check if we specified the full path

DenuxPlays commented 6 months ago

@ProbablyClem I am done.

Added tests and tested it on my production project. Please take a look at it and review my code. :)

DenuxPlays commented 6 months ago

Here is my production example: https://github.com/financrr/backend/tree/feature/general-enhancements

DenuxPlays commented 6 months ago

Thank you for your work.

It's a nice feature, and it's well done.

If you could take the time to add few unit tests it would be great.

It just made little comments but otherwise it good

I will add some unit tests.

I also left a comment about the current sub dependency issue. I think this is fixable

DenuxPlays commented 6 months ago

@ProbablyClem Hey sorry for the late response but I've implemented a few basic unit tests. The should be covered by the first tests.

Also we could make this project into a workspace to simplify ci but this is just something I noticed.

ProbablyClem commented 6 months ago

Thanks for your work, I'll make a new release