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] Add support for reqwest-middleware when using reqwest #13946

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

whirm
Copy link
Contributor

@whirm whirm commented Nov 7, 2022

Add support for reqwest-middleware (only when async support is enabled).

With this, one can easily add middleware which add features like tracing or retries to the client requests:

use petstore_async::apis::configuration::Configuration;
use reqwest::Identity;
use reqwest_middleware::ClientBuilder;
use reqwest_tracing::TracingMiddleware;

let config = Configuration {
        client: ClientBuilder::new(reqwest::ClientBuilder::new().build()?)
            .with(TracingMiddleware::default())
            .build(),
        ..Configuration::default()      
    }

Not sure if this would be considered a breaking change or not as it changes the Configuration struct signature. Let me know if I need to retarget the PR.

@frol, @farcaller, @richardwhiuk, @paladinzh, @jacob-pro: Pinging you as requested on the PR template :)

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@whirm whirm force-pushed the feature/rust-reqwest-middleware branch from 60107f9 to c8b1f9d Compare November 7, 2022 16:47
@jacob-pro
Copy link
Contributor

I'm a little bit concerned about making this a mandatory extra dependency - especially a crate which appears relatively new / small in popularity...

What do you think about putting this behind an optional feature flag instead?

@whirm
Copy link
Contributor Author

whirm commented Nov 8, 2022

Fair enough. I'll give it a shot.

@whirm whirm force-pushed the feature/rust-reqwest-middleware branch 2 times, most recently from 20476ab to 35dd16c Compare November 8, 2022 09:50
@whirm whirm changed the title [Rust] Add support for reqwest-middleware when using reqwest Draft: [Rust] Add support for reqwest-middleware when using reqwest Nov 8, 2022
@whirm whirm force-pushed the feature/rust-reqwest-middleware branch from 35dd16c to 92799da Compare November 8, 2022 10:29
@whirm whirm changed the title Draft: [Rust] Add support for reqwest-middleware when using reqwest [Rust] Add support for reqwest-middleware when using reqwest Nov 8, 2022
@whirm
Copy link
Contributor Author

whirm commented Nov 8, 2022

@jacob-pro I'd say it's ready now.

@whirm whirm force-pushed the feature/rust-reqwest-middleware branch from 92799da to 089b921 Compare November 8, 2022 10:41
@jacob-pro
Copy link
Contributor

Looks good to me 👍 - @wing328 should be able to review ?

@wing328
Copy link
Member

wing328 commented Nov 11, 2022

The build is successful locally:

   Compiling hyper-tls v0.5.0
   Compiling reqwest v0.11.11
   Compiling reqwest-middleware v0.1.6
   Compiling petstore-reqwest-async-middleware v1.0.0 (/Users/williamcheng/Code/openapi-generator4/samples/client/petstore/rust/reqwest/petstore-async-middleware)
warning: unused variable: `file`
   --> reqwest/petstore-async-middleware/src/apis/pet_api.rs:507:9
    |
507 |     let file = params.file;
    |         ^^^^ help: if this is intentional, prefix it with an underscore: `_file`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: `petstore-reqwest-async-middleware` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 5m 53s
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  05:56 min
[INFO] Finished at: 2022-11-11T18:16:30+08:00
[INFO] ------------------------------------------------------------------------

Will try to add tests to the CI in a separate PR.

Thanks again for the enhancement.

@wing328 wing328 merged commit 1670e95 into OpenAPITools:master Nov 11, 2022
@wing328
Copy link
Member

wing328 commented Nov 11, 2022

FYI. I've filed #13990 to add tests in CI.

@whirm whirm deleted the feature/rust-reqwest-middleware branch November 13, 2022 11:44
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.

3 participants