-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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] Async await #4210
[Rust] Async await #4210
Conversation
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
LGTM! Exciting work. I like the use of async-trait to keep the API as a trait and still async. 👍 One small nit. It would be nice if the generated API functions were generated as follows: async fn add_pet(&self, body: crate::models::Pet) -> Result<(), serde_json::Value>; ... instead of the lengthier but equivalent: fn add_pet(&self, body: crate::models::Pet) -> Box<Future<Item = (), Error = Error<serde_json::Value>>>; |
@JohnDoneth They will. Haven't run the bash scripts to update the examples yet. Example from generated code #[async_trait]
pub trait PetApi {
async fn add_pet(&self, body: crate::models::Pet) -> Result<(), Error<serde_json::Value>>;
async fn delete_pet(&self, pet_id: i64, api_key: Option<&str>) -> Result<(), Error<serde_json::Value>>;
async fn find_pets_by_status(&self, status: Vec<String>) -> Result<Vec<crate::models::Pet>, Error<serde_json::Value>>;
async fn find_pets_by_tags(&self, tags: Vec<String>) -> Result<Vec<crate::models::Pet>, Error<serde_json::Value>>;
async fn get_pet_by_id(&self, pet_id: i64) -> Result<crate::models::Pet, Error<serde_json::Value>>;
async fn update_pet(&self, body: crate::models::Pet) -> Result<(), Error<serde_json::Value>>;
async fn update_pet_with_form(&self, pet_id: i64, name: Option<&str>, status: Option<&str>) -> Result<(), Error<serde_json::Value>>;
async fn upload_file(&self, pet_id: i64, additional_metadata: Option<&str>, file: Option<std::path::PathBuf>) -> Result<crate::models::ApiResponse, Error<serde_json::Value>>;
} |
As per Metaswitch/swagger-rs#83, we'll want to wait for Hyper 0.13 to come out of beta before merging this. |
@dbcfd, I'm trying out this branch and with
currently: let mut req_builder = client.PUT(uri_str.as_str()); should be: let mut req_builder = client.put(uri_str.as_str()); It is named |
} else if (REQWEST_LIBRARY.equals(getLibrary())) { | ||
operation.httpMethod = operation.httpMethod.toLowerCase(Locale.ROOT); |
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.
May be leave this.
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.
There's places that require the method (e.g. .method(PUT)
). Thought I had got this working, but have been working on server, so will revisit.
@ctaggart Yeah, it's still very much wip, since both hyper and reqwest are changing. Even more so, swagger-rs would take significant changes that I can't test easily, so currently just using the context portion of it to make progress. Will push up my latest work so that if people want to pitch in they can. It's a pretty signficant effort. |
For our needs, these are the changes I made for the reqwest client templates ctaggart#1
|
@ctaggart can you explain the async_trait removal? Changing client from a trait to just an implementation, so not needed? |
It is still early days for https://github.com/dtolnay/async-trait. I hit some lifetime issues that I didn't want to troubleshoot. Our cli app doesn't need the traits or that additional dependency. I suspect that most probably don't. |
It may also be a good idea to implement |
What can we do to speed up this effort? Anything we can help with? |
@kanekv One would be to update swagger-rs to work with latest hyper. I've temporarily forked just the changes needed for this library in https://github.com/dbcfd/openapi-context. Updating swagger-rs to latest hyper is a much larger effort, but one that will need to be done. |
@dbcfd Is it a blocker or we can just continue with your approach? |
@kanekv it will definitely be a blocker for final merge, unless we want to roll the context stuff into this crate. If you're looking specifically for work on this repo, the client generator needs a revisit, pulling in some of @ctaggart changes. I've also pushed up my work on the server generator code. I likely won't have time over the next week to work on that, so feel free to work on that as well. |
@dbcfd Not necessarily for this repo, just looking how we can coordinate the work and get it moving. Who is working on swagger-rs transition? Is there any way we can parallelize it? |
I was looking at the swagger-rs transition, but realized it was fairly large on its own right, and I didn't have a great way to test the changes I would have to make. That's where the openapi-context fork came from. The client generator and the server generator are mostly distinct from each other, so one person could finish up the client portion and split it out as its own MR. |
Would it be beneficial to assume we can roll context stuff in (temporarily) and make client/server based on that assumption and also get it merged until we figure out what to do with swagger-rs, at that point we can switch it back? |
Yeah, we could publish openapi-context as a crate, then switch to swagger-rs when it is ready. Client/server could then be worked as publishable using openapi-context crate. |
Cool, any update? |
Almost have the example generated code working. Hoping to get it finished up today. At that point it should be good for people to experiment with. |
@ctaggart reqwest client should work now, in addition to hyper client and the server module. |
@JohnDoneth samples have been updated with the result of this work now. |
ping~ |
Great job @dbcfd! But I think replacing the current "reqwest" generator with an async version is not a good idea: So, the two versions (sync and async) should coexist (with a generator parameter, or with a new library "reqwest-async" for example). |
@bcourtine The reqwest client is by default async now, and blocking is under a different api. Leaving the default reqwest client as blocking will require a PR as well. There's a couple of ways to move this PR forward:
Release as is Release with async flagged Create new generators Thoughts? |
Hi @dbcfd, I personally like a lot async/await, and the language/crates moving forward is a good thing ;-) But Rust 1.39 was only released 3 months ago. Even if Rust is moving fast, considering a 4 months old Rust version deprecated and not supported anymore by openapi-generator seems harsh. @wing328 What is your opinion? |
@bcourtine I meant the versions of the crates tokio, hyper, reqwest, etc. being deprecated. The current versions of those crates all require 1.39 or higher. |
I agree, but theses crates still have support for a pre-1.39 version ;) |
@bcourtine Although these crates have pre 1.39 versions, they are not supported. Attempting to use the currently supported crates (e.g. hyper 0.13) on a 1.38 compiler will fail. info: default toolchain set to '1.38.0-x86_64-apple-darwin'
1.38.0-x86_64-apple-darwin installed - rustc 1.38.0 (625451e37 2019-09-23)
$ cd code/rust/
$ cargo new --lib hyper-test
Created library `hyper-test` package
$ cd hyper-test/
$ vi Cargo.toml
$ cargo build
Compiling tower-service v0.3.0
error: `core::slice::<impl [T]>::len` is not yet stable as a const fn
--> /Users/dannybrowning/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/bytes-0.5.4/src/bytes.rs:130:18
|
130 | len: bytes.len(),
| ^^^^^^^^^^^
error: aborting due to previous error |
Incorporate hyper and reqwest with async/await support. Bring in async-trait to simplify async trait interaction.
@dbcfd I've added an option to support async operations for I wonder if you can pull the latest master to give it a try. |
Closing as there's no update. |
Addresses #3865
Upgrade to use async/await by bringing in appropriate alpha releases for tokio, hyper, and reqwest. Also bring in async-trait to make api usage a little cleaner.
Requires beta compiler for Rust compiler. Expected to not be merged until after 1.39 is released.
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.1.x
,5.0.x
. Default:master
./cc @frol @farcaller @bjgill @richardwhiuk