-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Rust Server] Improve XML support #2504
Conversation
- Restore XML namespace support - Remove non snake case rust warning for xml wrap_in methods - Add XML rust-server tests - Fix wrapping XML arrays when a property of another object - Run all tests, not just those for OpenAPI 2.0 - Force wrapping for rust-server
@@ -64,6 +67,9 @@ | |||
public RustServerCodegen() { | |||
super(); | |||
|
|||
// Force generating models for wrapping types | |||
ModelUtils.setGenerateAliasAsModel(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardwhiuk thanks for the PR. Do you mind explaining why we must enforce this option to true?
Some users simply want to reuse the same array definition through out the spec with aliases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this should only apply to the Rust Generator - I need to figure out where to move it to so it's only invoked when the Rust generator is in use.
Currently, the generator assumes that the model will exist when generating the API bindings.
This means that it currently generates compile errors like:
error[E0412]: cannot find type `AnotherXmlArray` in module `models`
--> output/openapi-v3/src/lib.rs:94:52
|
94 | fn xml_other_put(&self, string: Option<models::AnotherXmlArray>, context: &C) -> Box<Future<Item=XmlOtherPutResponse, Error=ApiError>>;
| ^^^^^^^^^^^^^^^ did you mean `AnotherXmlInner`?
(for non aliasing, that should be Option<Vec<....>>
.)
Worse, this is a backwards compatibility break - previously the generator did generate aliasing - #1729 changes this so the default is not to generate aliasing, meaning that the 4.x branch generates Rust code that doesn't compile.
Longer term we can look at enhancing the Rust Server code generator to support generating code which compiles when aliasing is disabled (though unit structs in the manner that the Rust generator generates are fairly idiomatic, and allow for additional methods to be declared on them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this should only apply to the Rust Generator - I need to figure out where to move it to so it's only invoked when the Rust generator is in use.
Currently, it's already the case, which means only the Rust server generator will have this option enabled.
Thanks for the detailed explanation. We can certainly support generating array/map as model later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it's already the case, which means only the Rust server generator will have this option enabled.
I know what you mean now. I'll need to uncomment this line and show a warning message for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #2714
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the constructor must get run at startup, which means the other generators pick this up.
I'm slightly surprised we've defaulted generateAliasAsModel
to false - this seems like the reverse of the default in 3.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.x is a branch with breaking changes (with or without fallback) and the main reason is that most generators do not support alias (array/map) as model so that's we set generateAliasAsModel
to false as the default.
The CircleCI failure is related to outdated samples. I'll update those after merging this PR into master. |
* master: (40 commits) Remove quotation marks around {{paramName}} for header params in api-body.mustache (#2727) Add FiNC Technologies (#2728) fix missing parenthesis for http bearer auth (#2723) Add missing closing parenthesis (#2720) update perl test with correct body parameter (#2717) [Java][Spring] Fix template for reactive implementation with `interfaceOnly` parameter (#2437) Bugfix(Perl): Support nested primitive types in ARRARY or HASH for basic object (#2713) Remove `-XX:MaxPermSize` (#2712) Remove setting generateAliasAsModel in rust server generator (#2714) update rust server samples Revert "update rust samples" update rust samples update samples [Rust Server] Improve XML support (#2504) Improve CONTRIBUTING.md (#2699) [PHP][Lumen] Rename template folder (#2707) [aspnetcore] Support async tasks and some code cleanups (#2629) [C++][Pistache] Fixed #2643 (#2653) update petstore samples (#2697) [JAVA][Webclient]fix select body for url encoded media type. (#2686) ...
CC: Rust technical committee - @frol @farcaller @bjgill
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,. Default:3.4.x
,4.0.x
master
.Description of the PR
This change significantly improves
rust-server
's support for generating APIs with XML bodies.As part of this it makes a couple of core changes to Rust Server's code generator
The majority of this code was originally written by @bjgil. Additional fixes were contributed by myself, and others at @Metaswitch.