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

[Rust Server] Improve XML support #2504

Merged
merged 1 commit into from
Apr 22, 2019
Merged

[Rust Server] Improve XML support #2504

merged 1 commit into from
Apr 22, 2019

Conversation

richardwhiuk
Copy link
Contributor

@richardwhiuk richardwhiuk commented Mar 25, 2019

CC: Rust technical committee - @frol @farcaller @bjgill

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This change significantly improves rust-server's support for generating APIs with XML bodies.

  • Restore XML namespace support which was lost in OpenAPI upgrade
  • 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

As part of this it makes a couple of core changes to Rust Server's code generator

  • Run all tests, not just those for OpenAPI 2.0
  • Force wrapping (aliasing) for rust-server - this is required because Rust doesn't allow implementing traits on types which the user doesn't own. Note, with wrapping disabled, the current generator creates code which doesn't compile.

The majority of this code was originally written by @bjgil. Additional fixes were contributed by myself, and others at @Metaswitch.

- 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);
Copy link
Member

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.

Copy link
Contributor Author

@richardwhiuk richardwhiuk Apr 21, 2019

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).

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #2714

Copy link
Contributor Author

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?

Copy link
Member

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.

@wing328
Copy link
Member

wing328 commented Apr 22, 2019

The CircleCI failure is related to outdated samples. I'll update those after merging this PR into master.

@wing328 wing328 merged commit f14bac8 into OpenAPITools:master Apr 22, 2019
@richardwhiuk richardwhiuk deleted the rust-server-xml branch April 22, 2019 09:12
jimschubert added a commit that referenced this pull request Apr 25, 2019
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants