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

Update schemars for preserve_order #31

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Conversation

ralpha
Copy link
Collaborator

@ralpha ralpha commented Jun 8, 2020

After some weird git conflicts 🤐 Here it is.
This is the followup from GREsau/schemars#36

This uses the preserve_order flag from schemars.
I tested and it is not needed to set the flag inside okapi or rocket_okapi as long it is set in your code (example in this case).

I found there is an other edge case, that was already in old code.
When you have 2 routes with the same method and path: get("/user") in this case. Rocket_okapi will not handle this right now (as the structure does not allow it). I set an warning message where it happens. The other suggestions in GREsau/schemars#36 (comment) would not solve this either I think?
This warning should be handled I thing. Any ideas how? How would the spec handle this?
Otherwise I think we should add test if the route exists and keep the first route not the last.

There is also a use std::collections::BTreeMap as Map; I added a TODO to this as I'm not sure if we have to change this one too?

Also some changes from upstream in Schemars: schema_for_any() deprecation. (was nice to see how rust and cargo handles depredations :D ) Same with PartialEq. @GREsau could you take a look at impl OpenApiResponder<'_> for JsonValue as I am not sure about this. Looks a bit odd to me.

Let me know what should be changed.

Copy link
Owner

@GREsau GREsau 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, just a couple of niggles to fix and then I think we're good to merge this 🙂

When you have 2 routes with the same method and path: get("/user") in this case. Rocket_okapi will not handle this right now

Logging a warning is fine for now. In the future I'd like to either:

  1. use the route with the lowest rank (i.e. the one Rocket tries first)
  2. try to combine all conflicting routes into a single operation - I'm not sure how easy this would be though...

There is also a use std::collections::BTreeMap as Map; I added a TODO to this as I'm not sure if we have to change this one too?

I don't think that one needs changing since it's never iterated over, so order is irrelevant.

Also some changes from upstream in Schemars: schema_for_any() deprecation...Same with PartialEq

👍

could you take a look at impl OpenApiResponder<'_> for JsonValue as I am not sure about this

Looks fine to me. For some other places that don't actually use that argument, I just call the parameter _ rather then _gen, but it really doesn't matter

@ralpha ralpha marked this pull request as ready for review June 11, 2020 21:57
@ralpha ralpha requested a review from GREsau June 11, 2020 21:57
@ralpha
Copy link
Collaborator Author

ralpha commented Jun 11, 2020

I have not seen many warnings in the rocket_okapi code,
but with the println!("Warning: Operation replaced for {}:{}", op.method, e.key()); I want to suggest changing this to use log at some point. This way it integrates with the rest of rust's logging. Like rocket and others.

@GREsau GREsau merged commit 1c40f4d into GREsau:master Jun 12, 2020
@GREsau
Copy link
Owner

GREsau commented Jun 12, 2020

Brilliant, thanks!

I agree with using the log crate too

@ralpha
Copy link
Collaborator Author

ralpha commented Jun 13, 2020

Successfully updated to the new version of both okapi and schemars from crates.io
Will soon open some issues for my last outstanding changes in rocket_okapi and codegen.
These are currently only:

  • Allowing to create openapi.json without starting rocket server. (already have most of the code nicely done, so will see if you like it too)
  • Set more specific header information in the openapi spec. (a bit of a hack on my side currently, will probably go for a more general approach)

Once these are resolved can change rocket_okapi and codegen over to the create.io version too. 😄

mautamu pushed a commit to mautamu/okapi that referenced this pull request Apr 23, 2021
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