Namespacing same types #187

kellpossible · 2022-06-25T05:13:06Z

This work was sponsored by Arctoris.

…ve the same name.

kellpossible · 2022-06-25T06:57:49Z

Right now there is a failing test for:

#[derive(Component)]
struct Bar {
     args: Vec<std::vec::Vec<std::string::String>>
}

I'm wondering whether it's reasonable to even resolve primitive types when they have a path?

kellpossible · 2022-06-25T07:23:55Z

It seems like I may have also broken some of the optional features format resolution with this.

juhaku

I refactored the value_type functionality for Component and added it to IntoParams as well. This PR needs rebase from latest master.

There are couple of things need a bit of rethinking. One being the namespacing. At least we cannot have it namespaced with the Rust path which will result invalid OpenAPI spec. Maybe better way is to allow users to use as MyAnotherType but this might not be feasible because the question relies who to reference such type which uses as MyType syntax.

Another thing is that I have been planning on refactoring the component() function to return a name of the component as well along side with the component. This would allow users to use serde's rename on container level. But with this as well the hot question is how to reference such component which has been renamed.

Lastly is it too cumbersome to support sort of namespacing? It would be great to have some solution to this but after all if one was to write a OpenAPI spec manually he/she would need to write distinct type names for each type. So there would not be a clash in such scenario. Point being use uniques type names for Components as well. 😂

tests/component_derive_test.rs

tests/request_body_derive_test.rs

juhaku · 2022-07-02T22:27:15Z

utoipa-gen/src/lib.rs

+                        );
+
+                        let argument_path: &TypePath = match argument {
+                            GenericArgument::Type(syn::Type::Path(path)) => path,


We probably could get rid of this whole custom type parsing and use the ComponentPart. This would allow users to use the regular Rust types as arguments.

Unfortunately it fails to parse things like [Foo] and Option<[Foo]>

Perhaps we could do a breaking change, and make it so people need to use Vec<Foo> and Option<Vec<Foo>> instead?

Yeah. That kind of was my assumption as well. With allowing users to use Vec<Foo> and Option<Vec<Foo>> using ComponentPart we could possibly get rid of this custom type thing which is not necessary a bad thing. It can be addressed in another PR and it surely could be a breaking change.

utoipa-gen/src/schema.rs

juhaku · 2022-07-02T22:36:22Z

utoipa-gen/src/schema.rs

+        let path = TypePath {
+            qself: None,
+            path: syn::Path::from(segment.clone()),
+        };
+
+        let mut generic_component_type = ComponentPart::convert(Cow::Owned(path));


Hmm.. interesting approach 🤔

Do you think there is a better way to do it?

Actually not sure whether there is any better approach. 😄

utoipa-gen/src/schema/into_params.rs

kellpossible · 2022-07-03T01:30:32Z

At least we cannot have it namespaced with the Rust path which will result invalid OpenAPI spec.

Is this true? It seems to work fine in swagger, and I can't see anything in the spec which says that component references can't contain :: in them.

Lastly is it too cumbersome to support sort of namespacing? It would be great to have some solution to this but after all if one was to write a OpenAPI spec manually he/she would need to write distinct type names for each type. So there would not be a clash in such scenario. Point being use uniques type names for Components as well.

Let's say you have module alpha and inside you have Query, and inside module beta you have Query. You could rename them to AlphaQuery and BetaQuery but the Rust community errs against this, https://rust-lang.github.io/rust-clippy/master/#module_name_repetitions . You could rename them with a use alpha::Query as AlphaQuery but this makes mental mapping the objects back to the code more cumbersome.

juhaku · 2022-07-03T14:29:14Z

At least we cannot have it namespaced with the Rust path which will result invalid OpenAPI spec.

Is this true? It seems to work fine in swagger, and I can't see anything in the spec which says that component references can't contain :: in them.

It actually works but not without caveats. If you try out the spec with https://editor.swagger.io/ and try to use :: referenced names here it will work but same time it will complain you about invalid names.

While this may not be a deal breaker for all but certainly most of people want to have spec validated and linted correctly.

If we want to have namespaces implemented there. I guess we need to do manual mangling of the type name according to the rules of Swagger.

Let's say you have module alpha and inside you have Query, and inside module beta you have Query. You could rename them to AlphaQuery and BetaQuery but the Rust community errs against this, https://rust-lang.github.io/rust-clippy/master/#module_name_repetitions . You could rename them with a use alpha::Query as AlphaQuery but this makes mental mapping the objects back to the code more cumbersome.

Yeah I agree it's not optimal solution not necessary fan of implementing such thing too though 😄

kellpossible · 2022-07-04T09:11:47Z

@juhaku

While this may not be a deal breaker for all but certainly most of people want to have spec validated and linted correctly.

If we want to have namespaces implemented there. I guess we need to do manual mangling of the type name according to the rules of Swagger

That sounds reasonable. How about this proposal, we replace :: with .? And if people don't like it, nobody is forcing them to reference components using paths, but at least there is the option, because the alternative effectively requires a bunch of renaming or awkward imports in a pre-existing codebase.

juhaku · 2022-07-04T16:12:11Z

@juhaku

While this may not be a deal breaker for all but certainly most of people want to have spec validated and linted correctly.
If we want to have namespaces implemented there. I guess we need to do manual mangling of the type name according to the rules of Swagger

That sounds reasonable. How about this proposal, we replace :: with .? And if people don't like it, nobody is forcing them to reference components using paths, but at least there is the option, because the alternative effectively requires a bunch of renaming or awkward imports in a pre-existing codebase.

I reckon we could go with the dot . instead of the colon:. Though it kind of sticks out from Rust syntax but I guess that is best we can go with so far. Underscore _ would look too out of the place I wonder? Yeah we can offer a way to use references but its not necessary to them to use it.

Also another thing I am wondering could we use sort of special syntax like components(path(foo::bar::MyType)) if we wanted to have the full path seen in the OpenAPI doc? This might be also too much and just add to the complexity which would be unnecessary for the users point of view though?

kellpossible · 2022-07-06T03:58:35Z

I reckon we could go with the dot . instead of the colon:. Though it kind of sticks out from Rust syntax but I guess that is best we can go with so far. Underscore _ would look too out of the place I wonder? Yeah we can offer a way to use references but its not necessary to them to use it.

Yeah I think . is definitely better than an underscore, it's more commonly used in programming to delineate context.

Also another thing I am wondering could we use sort of special syntax like components(path(foo::bar::MyType)) if we wanted to have the full path seen in the OpenAPI doc? This might be also too much and just add to the complexity which would be unnecessary for the users point of view though?

I think I agree that this is unnecessary at this stage if we have the . it should be good enough and simple to implement.

juhaku · 2022-07-06T10:41:42Z

Yeah good

juhaku · 2022-07-07T15:56:58Z

We probably need to strip the generics out of the path. The error in build is because the path has generic params
"#/components/schemas/Foo < 'bar >"

Failing test: derive_component_with_complex_enum_lifetimes

kellpossible · 2022-07-07T19:19:03Z

We probably need to strip the generics out of the path. The error in build is because the path has generic params "#/components/schemas/Foo < 'bar >"

Failing test: derive_component_with_complex_enum_lifetimes

Thanks! Yes I didn't have time to fix that one yet, I'll fix it next week and the other comments, and hopefully ready to merge soon so it doesn't require another large merge conflict resolution.

juhaku · 2022-07-07T22:13:49Z

hopefully ready to merge soon so it doesn't require another large merge conflict resolution.

Yeah sorry about that previously 🙇 . Hopefully not, I'm working on the extension part which is getting some refinment there and the impact should not be too big.

kellpossible · 2022-07-11T02:20:19Z

@juhaku I can't figure out why the utoipa-gen tests are failing on CI, it's not happening locally for me

juhaku · 2022-07-11T20:30:07Z

@juhaku I can't figure out why the utoipa-gen tests are failing on CI, it's not happening locally for me

Ahh.. I had broken the tests there previously. It was missing a feature flag from actix-web framework. I merged the encoding PR and fixed the test. If you take rebase from master I guess this issue will get sorted out. :)

juhaku · 2022-07-12T06:44:11Z

There seems to be broken rocket related tests. You can run all tests with command ./tests/test.sh utoipa and see what goes wrong. Seems like rocket integration breaks for some reason. 🤔

Test: resolve_get_with_multiple_args
Test: resolve_get_with_optinal_query_args
Error: Query items parameter type: [0].schema.items.type expected to be: "string" but was: null', tests/path_derive_rocket.rs:65:5

kellpossible · 2022-07-12T06:50:39Z

@juhaku I had a look into it today, it seems that the parsing for rocket arguments types (and perhaps actix too?) is pretty limited, ideally we should share code with the component derive macro?

kellpossible · 2022-07-12T06:51:48Z

I'm thinking maybe to quickly fix this bug with a work around, and lodge a new issue to propose a refactor in this regard

kellpossible · 2022-07-12T06:53:43Z

I'm also thinking maybe we should merge this with the [Type] and Option<[Type]> syntax parsing route, we've got almost the same logic in about 3 different places.

Edit: mentioned in issue #217

juhaku · 2022-07-12T21:33:29Z

@juhaku I had a look into it today, it seems that the parsing for rocket arguments types (and perhaps actix too?) is pretty limited, ideally we should share code with the component derive macro?

Yeah those use cases are quite specific. And there is surely room to improve and refactor the implementation. The axum integration will party address this issue. But will not fully "fix" it. My plan is to address the steps in #217 regarding this matter.

I'm thinking maybe to quickly fix this bug with a work around, and lodge a new issue to propose a refactor in this regard

Yeah that is okay for now.

I'm also thinking maybe we should merge this with the [Type] and Option<[Type]> syntax parsing route, we've got almost the same logic in about 3 different places.

I also commented there about this issue.

juhaku

Super. 🚀 Few comments only and soon this can be merged :)

juhaku · 2022-07-12T21:44:29Z

utoipa-gen/src/lib.rs

+                        );
+
+                        let argument_path: &TypePath = match argument {
+                            GenericArgument::Type(syn::Type::Path(path)) => path,


Yeah. That kind of was my assumption as well. With allowing users to use Vec<Foo> and Option<Vec<Foo>> using ComponentPart we could possibly get rid of this custom type thing which is not necessary a bad thing. It can be addressed in another PR and it surely could be a breaking change.

utoipa-gen/src/schema.rs

juhaku · 2022-07-12T21:58:07Z

utoipa-gen/src/schema.rs

+        let path = TypePath {
+            qself: None,
+            path: syn::Path::from(segment.clone()),
+        };
+
+        let mut generic_component_type = ComponentPart::convert(Cow::Owned(path));


Actually not sure whether there is any better approach. 😄

juhaku

👍

kellpossible · 2022-07-13T22:59:48Z

@juhaku thanks for reviewing this PR, it was a bit of a mammoth in the end!

kellpossible added 2 commits June 25, 2022 15:33

Work in progress support for juhaku#186 namespacing for types that ha…

03929ff

…ve the same name.

Work in progress support for path as component reference

4358be5

kellpossible force-pushed the namespacing-same-types branch from 0e7e463 to 4358be5 Compare June 25, 2022 05:38

kellpossible added 2 commits June 25, 2022 15:39

Fix compile error after rebase

c9c49b1

Support path in value_type field for Component derive

fdecc6c

kellpossible marked this pull request as ready for review June 25, 2022 06:55

Fix DateTime and String known type parsing tests failing

e01fd51

kellpossible mentioned this pull request Jun 26, 2022

Component value_type attribute doesn't support Vec, Option, Any, etc #191

Closed

kellpossible added 4 commits June 28, 2022 09:14

Implement PartialEq for ComponentPart manually

deb9a29

Merge branch 'master' into namespacing-same-types

32489d6

Fix compile errors with actix extras

f4a7da8

Fix rocket_extras compile errors

43db836

juhaku requested changes Jul 2, 2022

View reviewed changes

Merge branch 'master' into namespacing-same-types

6901f9f

kellpossible added 3 commits July 10, 2022 21:54

Strip generics from path used in references

cc6b5e4

Replace :: with . for paths

cb8f2e6

Remove redunant documentation

d8168c6

Merge branch 'master' into namespacing-same-types

00399ec

kellpossible added 2 commits July 12, 2022 19:31

Fix failing path_derive_rocket unit test

3068ee6

Fix compile error with actix_extras feature

f5d4f2b

juhaku reviewed Jul 12, 2022

View reviewed changes

juhaku approved these changes Jul 13, 2022

View reviewed changes

Clean up inline component logic

d21b03a

juhaku approved these changes Jul 13, 2022

View reviewed changes

juhaku merged commit a2b319f into juhaku:master Jul 13, 2022

kellpossible deleted the namespacing-same-types branch July 13, 2022 22:59

RemiKalbe mentioned this pull request Jul 25, 2022

(Master) the name _AssertComponentSomeComponent is defined multiple times #231

Closed

juhaku mentioned this pull request Nov 18, 2022

Unexpected error with schemas #352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespacing same types #187

Namespacing same types #187

kellpossible commented Jun 25, 2022 •

edited

Loading

kellpossible commented Jun 25, 2022

kellpossible commented Jun 25, 2022

juhaku left a comment

juhaku Jul 2, 2022

kellpossible Jul 11, 2022

kellpossible Jul 11, 2022

juhaku Jul 12, 2022

juhaku Jul 2, 2022

kellpossible Jul 12, 2022

juhaku Jul 12, 2022

kellpossible commented Jul 3, 2022 •

edited

Loading

juhaku commented Jul 3, 2022

kellpossible commented Jul 4, 2022 •

edited

Loading

juhaku commented Jul 4, 2022

kellpossible commented Jul 6, 2022

juhaku commented Jul 6, 2022

juhaku commented Jul 7, 2022

kellpossible commented Jul 7, 2022

juhaku commented Jul 7, 2022

kellpossible commented Jul 11, 2022

juhaku commented Jul 11, 2022

juhaku commented Jul 12, 2022

kellpossible commented Jul 12, 2022

kellpossible commented Jul 12, 2022

kellpossible commented Jul 12, 2022 •

edited

Loading

juhaku commented Jul 12, 2022

juhaku left a comment

juhaku Jul 12, 2022

juhaku Jul 12, 2022

juhaku left a comment

kellpossible commented Jul 13, 2022

Namespacing same types #187

Namespacing same types #187

Conversation

kellpossible commented Jun 25, 2022 • edited Loading

kellpossible commented Jun 25, 2022

kellpossible commented Jun 25, 2022

juhaku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellpossible commented Jul 3, 2022 • edited Loading

juhaku commented Jul 3, 2022

kellpossible commented Jul 4, 2022 • edited Loading

juhaku commented Jul 4, 2022

kellpossible commented Jul 6, 2022

juhaku commented Jul 6, 2022

juhaku commented Jul 7, 2022

kellpossible commented Jul 7, 2022

juhaku commented Jul 7, 2022

kellpossible commented Jul 11, 2022

juhaku commented Jul 11, 2022

juhaku commented Jul 12, 2022

kellpossible commented Jul 12, 2022

kellpossible commented Jul 12, 2022

kellpossible commented Jul 12, 2022 • edited Loading

juhaku commented Jul 12, 2022

juhaku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juhaku left a comment

Choose a reason for hiding this comment

kellpossible commented Jul 13, 2022

kellpossible commented Jun 25, 2022 •

edited

Loading

kellpossible commented Jul 3, 2022 •

edited

Loading

kellpossible commented Jul 4, 2022 •

edited

Loading

kellpossible commented Jul 12, 2022 •

edited

Loading