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] enhance support for middlewares #552

Merged
merged 8 commits into from
Jul 19, 2018
Merged

[rust-server] enhance support for middlewares #552

merged 8 commits into from
Jul 19, 2018

Conversation

bjgill
Copy link
Contributor

@bjgill bjgill commented Jul 12, 2018

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 and ./bin/security/{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.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language. @frol @farcaller

Description of the PR

This enhances codegen to generate the new RequestParser trait, so middlewares can get the operation ID from a hyper Request. This PR also tweaks the autogenerated client to be generic over the future type that the underlying hyper Client returns. This is needed to allow passing wrapped clients - the FutureResponse type used originally is private to hyper and can't be constructed. This change allows us to wrap the hyper client below the autogen layer but above hyper.

I've not done any manual testing of this PR - we've been using this in production for the past month, though, so low risk.

Thanks to @mthebridge, who originally contributed these changes.

Part of the fix for #550

@wing328
Copy link
Member

wing328 commented Jul 17, 2018

@bjgill thanks for the PR. Is it correct to say that this PR is a breaking change for the Rust client?

If yes, we should include it in 3.2.x branch instead.

@bjgill
Copy link
Contributor Author

bjgill commented Jul 18, 2018

@wing328 - this will only be a breaking change if the user has put the Client's type signature in their code. Otherwise, no changes are needed - e.g. for the example client integration. Any migration should be straightforward - they'll just need to update their type signature to match the new one.

Given that rust-server is in the list of generators not yet fully migrated, do we actually need to abide by the stability guarantees implied by the versioning of openapi-generator as a whole? There is going to be at least one more breaking PR before I'm ready to do any serious testing of rust-server.

However, if it's not acceptable to keep rust-server unstable for the moment, we probably do need to move active development onto the 3.2.x branch. Do you know what the expected release date for that is?

(Also, I've noticed that we weren't previously checking that the example integration compiled, so have just fixed that in the latest commit.)

@wing328
Copy link
Member

wing328 commented Jul 19, 2018

Given that rust-server is in the list of generators not yet fully migrated, do we actually need to abide by the stability guarantees implied by the versioning of openapi-generator as a whole? There is going to be at least one more breaking PR before I'm ready to do any serious testing of rust-server.

In the migration guide, I've updated rust-server as "migrated". Only the apex generator is pending migration and testing at the moment.

@wing328
Copy link
Member

wing328 commented Jul 19, 2018

this will only be a breaking change if the user has put the Client's type signature in their code. Otherwise, no changes are needed - e.g. for the example client integration. Any migration should be straightforward - they'll just need to update their type signature to match the new one.

Thanks for the explanation.

If this PR is ready to merge, please kindly let me know.

@wing328 wing328 added this to the 3.1.2 milestone Jul 19, 2018
@bjgill
Copy link
Contributor Author

bjgill commented Jul 19, 2018

Thanks. This is ready to merge.

@wing328 wing328 merged commit c5e1709 into OpenAPITools:master Jul 19, 2018
@bjgill bjgill deleted the client-hyper-type branch July 20, 2018 14:32
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Generate RequestParser trait to allow retrieving operation ID in middlewares

* Update function name

* Fix incomplete comment

* Add comment poitning out auotgenerated duplication

* Final generation of sample scripts

* MMORCH-913 - Allow passing wrapped hyper clients to codegen

* Deprecate old API for back-compatibility rather than removing it

* Actually test Rust-server example integrations
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.

None yet

3 participants