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

Document more complex parameters in actix plugin #242

Closed
wants to merge 7 commits into from

Conversation

platy
Copy link
Collaborator

@platy platy commented Oct 6, 2020

Attempt at fixing #239 and #265

For usage of serde_qs::QsQuery<T>, this will descend the tree of T and document each primitive or array as a separate parameter with the name given by it's location in the tree in the qs format, eg. user[phone_number][mobile]. The main limitaion is that an array cannot contain an object, only primitives - this is because an array of primitives can be identified without array indicies which would require a parameter in the name - and openapi doesn't support that.

For other parameters this change also adds the .items parameter of the ParameterObject and is generated recursively.

data_type: v.data_type,
format: v.format,
enum_: v.enum_,
description: v.description,
items: v.items.as_ref().map(|schema| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nested parameters can go beyond one level in schema, so it's better if we probably leave it at items: v.items.clone()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here v.items is Option<DefaultSchemaRaw> rather than Option<Items> so it needs to be mapped.
I thought that this part of the code was only being used to produce items for query / path parameters in which case nested items would only work with different values for collection_format so that will need to be specified somewhere.
I think we need collection_format before this can be merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now that I have read the documentation of serde_qs a bit better I see what you mean, nesting in queries is not something I have used.
The openapi v2 spec doesn't allow for objects in query parameters so the definitions generated for serde_qs will need to create a separate parameter for each nested field. I will have a try at this serde_qs-specific generation

@platy platy changed the title Produce items field on parameters Document more complex parameters in actix plugin Dec 8, 2020
@platy platy marked this pull request as ready for review December 8, 2020 09:10
@omid
Copy link
Contributor

omid commented Dec 8, 2020

One issue which seems not related to your changes... If I don't have any dependency on serde_qs in my own package, it will trigger an error like:

  --> /usr/local/cargo/git/checkouts/paperclip-a2bfe9987995ca28/f053346/core/src/v2/actix.rs:22:15
   |
22 | use serde_qs::actix::QsQuery;
   |               ^^^^^ could not find `actix` in `serde_qs`

I think it's because we don't have the actix feature enabled in the serde_qs dependency definition.
Currently in serde_qs we have actix and actix2 features. I think the equivalent feature should be applied here.

Cargo.toml Outdated
@@ -53,6 +53,7 @@ reqwest = { version = "0.10", features = ["blocking", "json"] }
log = { version = "0.4", features = ["kv_unstable"] }
insta = "1.0"
env_logger = "0.8"
serde_qs_test = { package = "serde_qs", version = "0.8.1", features = ["actix"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

The version of serde_qs in the core is 0. It means at some point in time, tests may pass, but the code can be broken.

@platy
Copy link
Collaborator Author

platy commented Dec 8, 2020

@omid thanks for the feedback, I'll get around to trying to fix these things tomorrow. I'm a bit confused about what to do about the dependencies. I guess that maybe we should reexport QsQuery like with the actix::web stuff.

@omid
Copy link
Contributor

omid commented Dec 8, 2020

@omid thanks for the feedback, I'll get around to trying to fix these things tomorrow. I'm a bit confused about what to do about the dependencies. I guess that maybe we should reexport QsQuery like with the actix::web stuff.

We can fix this dependency problem in another PR @platy.
I have the environment prepared for this, maybe I can do that :)

@platy
Copy link
Collaborator Author

platy commented Dec 9, 2020

So afaics we cant specify the actix / actix2 features on serde_qs without splitting the serde_qs feature into 2, ie. serde_qs+actix2, serde_qs+actix3. Which doesn't seem worth it.

@platy
Copy link
Collaborator Author

platy commented Dec 10, 2020

I'm now not convinced that the QS array serialisation can actually be expressed in OpenAPI. I tried out a JS client generator using the spec generated by this branch and it url-encodes the brackets, which serde_qs doesn't accept by default.

So I'm making this a draft again.

@platy
Copy link
Collaborator Author

platy commented Dec 17, 2020

I've raised another PR #278 which just has the items and not the QS stuff which I don't think will work

@platy platy closed this Dec 17, 2020
@platy platy deleted the parameter-items branch December 22, 2020 07:49
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.

3 participants