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

Backwards compatible retention of extension parsing for async reqwest #928

Merged
merged 5 commits into from
Apr 28, 2024

Conversation

bengsparks
Copy link
Contributor

@bengsparks bengsparks commented Apr 27, 2024

Why are we making this change?

Partially addresses the needs of #927.

What effects does this change have?

Changes the ReqwestExt trait to return DelayedResponse.

Calling DelayedResponse::retain_extensions::<T> and awaiting will deserialise the extensions field using the shape of T.

Directly awaiting DelayedResponse (i.e. the previous behaviour, thereby maintaining backwards compatibility) will produce an ImmediateResponse, which will ignore the contents of the "extensions" field.

Adds futures-lite to make implementing of std::future::Future simpler.
If you think the implementation could be improved to avoid needing futures::FutureExt, please make any changes you deem necessary.

Adds tokio = { version = "1", features = ["macros"] } and mockito = "1.4.0" to dev dependencies to provide an asynchronous mock server for testing purposes.

cynic/tests/http.rs provides a comparison of their usages.

Examples:

For a GraphQL server that responds with

{
  "errors": [
    {
      "message": "Unauthorized",
      "locations": null,
      "path": [ "helloWorld" ],
      "extensions": { "code": 401 }
    }
  ]
}
Including extensions with DelayedResponse::retain_extensions
#[derive(Debug, serde::Deserialize)]
struct Extensions {
    code: u16,
}

let output = client
    .get(format!("http://{}/graphql", graphql.host_with_port()))
    .run_graphql(FieldWithString::build(FieldWithStringVariables {
        input: "InputGoesHere",
    }))
    .retain_extensions::<Extensions>()
    .await;
dbg(!&output);

Will produce a response with extensions: Some(Extensions { code: 401 } ).

Backwards compatible
let client = reqwest::Client::new();
let output = client
    .get(format!("http://{}/graphql", graphql.host_with_port()))
    .run_graphql(FieldWithString::build(FieldWithStringVariables {
        input: "InputGoesHere",
    }))
    .await;
dbg(!&output);

Will produce a response with extensions: Some(IgnoredAny).

Copy link

netlify bot commented Apr 27, 2024

Deploy Preview for cynic-querygen-web canceled.

Name Link
🔨 Latest commit af3dcd8
🔍 Latest deploy log https://app.netlify.com/sites/cynic-querygen-web/deploys/662e70a314e7df0008eb0bed

@bengsparks
Copy link
Contributor Author

@obmarg The CI fails to due to some version incompatibility between clap and rustc. Please let me know if I need to make any changes to this PR myself to fix this.

@obmarg
Copy link
Owner

obmarg commented Apr 28, 2024

Nice work @bengsparks - thanks for taking the time to do this.

I'm going to push up a couple of tweaks:

  1. rolling back some of the Cargo.toml changes (I do need to upgrade a lot of the packages, but I'd rather not bundle those updates into this PR)
  2. I've removed the need for futures-lite - nothing wrong with it, but I'd rather avoid an extra dependency if I can.
  3. Also renamed DelayedResponse to CynicReqwestBuilder. DelayedResponse makes sense in the context of this PR, but this builder seems like it'd be a good place to put any other features that get added to the integration, so I'd like to go for the more generic name.

@obmarg obmarg enabled auto-merge (squash) April 28, 2024 15:55
@obmarg obmarg merged commit 05dc9f3 into obmarg:main Apr 28, 2024
6 checks passed
@cynic-releaser cynic-releaser bot mentioned this pull request Apr 28, 2024
@obmarg
Copy link
Owner

obmarg commented Apr 28, 2024

Released in v3.7.0

@bengsparks bengsparks deleted the feat/reqwest-error-extension branch April 28, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants