Pluggable HTTP client for CosmosDB #79

MindFlavor · 2020-11-12T11:18:28Z

Intro

This is a proposal for a pluggable HTTP client implementation.
I've started with CosmosDB because it's the most feature-complete create (and thus most complicated).

The first commit has:

The HttpClient definition (very simple).
An implementation of HttpClient for both Reqwest and Hyper.
Support for the Get Database function.
An example of Get Database usage with the new pluggable interface.

To support other HTTP clients all we need is to implement HttpClient for the new client. Basically it's one single function that translates http::request::Request in the implementation-specific calls and maps back the result into http::response::Response. The idea, as you can see, is to abstract away the HTTP call:

  #[async_trait]
  pub trait HttpClient: Send + Sync + std::fmt::Debug {
      async fn execute_request(
          &self,
          request: Request<&[u8]>,
      ) -> Result<Response<Vec<u8>>, Box<dyn std::error::Error + Sync + Send>>;

// ...<cut>...

As of now, the client instantiation goes through a builder that ensures a valid HTTP client has been specified:

let client = azure_cosmos::client_builder::new(&account, authorization_token)?
  .with_http_client(http_client)   // mandatory call
  .build();                        // without HTTP client this call would fail at compile time

The HTTP client must implement HttpClient interface (right now both reqwest and hyper are supported). The HttpClient trait is Send + Sync so it's thread safe.

The actual clients take the trait wrapped in a Arc heap allocated object:

Box is for ergonomics (so the trait object is owned).
Arc is to make sure the trait object is cloneable (plain trait objects are not cloneable). This may not be necessary, can be removed if needed at later time. This also allows to reuse the same underlying HTTP client in many Azure clients (good because the initialization is relatively expensive).

This is an basic, working example:

Using reqwest

    // use reqwest
    let http_client: Box<dyn HttpClient> = Box::new(reqwest::Client::new());
    let http_client = Arc::new(http_client);

    let client = azure_cosmos::client_builder::new(&account, authorization_token)?
        .with_http_client(http_client)
        .build();

Using hyper

    // use hyper  
    let http_client: Box<dyn HttpClient> =
        Box::new(hyper::Client::builder().build(HttpsConnector::new()));
    let http_client = Arc::new(http_client);

    let client = azure_cosmos::client_builder::new(&account, authorization_token)?
        .with_http_client(http_client)
        .build();

From now on the code is the behaves the same, as show from the execution result of get_database example:

from reqwest == GetDatabaseResponse { database: Database { id: "pollo", rid: "cJJTAA==", ts: 1586886691, _self: "dbs/cJJTAA==/", etag: "\"0000fe06-0000-0c00-0000-5e95f8230000\"", colls: "colls/", users: "users/" }, charge: 2.0, activity_id: 6c8b331e-1ba8-458a-9f6f-dc6485217ef0, session_token: "0:-1#8058", etag: "\"0000fe06-0000-0c00-0000-5e95f8230000\"", last_state_change: 2020-11-11T20:08:21.262Z, resource_quota: [Databases(1000)], resource_usage: [Databases(2)], schema_version: "1.10", service_version: "version=2.11.0.0", gateway_version: "version=2.11.0" }
from hyper == GetDatabaseResponse { database: Database { id: "pollo", rid: "cJJTAA==", ts: 1586886691, _self: "dbs/cJJTAA==/", etag: "\"0000fe06-0000-0c00-0000-5e95f8230000\"", colls: "colls/", users: "users/" }, charge: 2.0, activity_id: 7046192b-696a-4575-9f4f-7b7c2f79506c, session_token: "0:-1#8058", etag: "\"0000fe06-0000-0c00-0000-5e95f8230000\"", last_state_change: 2020-11-11T18:43:04.173Z, resource_quota: [Databases(1000)], resource_usage: [Databases(2)], schema_version: "1.10", service_version: "version=2.11.0.0", gateway_version: "version=2.11.0" }

ToDo

Things to do before this PR can be merged:

Correct and re-enable all the CosmosDB functions.
Remove Hyper from the crate's dependencies (and move it to dev-dependencies as it will be used in the examples).
Remap the responses TryFrom implementations to accept a simpler http::response::Response.
Correct all the CosmosDB examples.
Correct all the CosmosDB E2E tests.

Optional:

~~- [ ] Implement HttpClient for other HTTP libraries (surf maybe?) to check if the abstraction holds.~~

rylev

Great to see this work kicked off! ❤

Cargo.toml

rylev · 2020-11-12T13:03:11Z

sdk/core/src/http_client.rs

@@ -0,0 +1,126 @@
+use crate::errors::{AzureError, UnexpectedHTTPResult};
+use async_trait::async_trait;
+use http::{Request, Response, StatusCode};


Using these types somewhat binds us to the Hyper ecosystem. I believe Surf has a way to convert to and from these types though. Then again we probably don't want to implement our own versions of these types...

sdk/core/src/http_client.rs

rylev · 2020-11-12T13:10:50Z

sdk/core/src/http_client.rs

+        let body = String::from_utf8(request.body().to_vec())?;
+        let hyper_request = hyper_request.body(Body::from(body))?;
+
+        let hyper_response = self.request(hyper_request).await?;


Doesn't hyper already return an http response?

I've tried but it seems to return an http::Response<hyper::Body> . The trait expect an http::Response<Vec<u8>> instead. In my understanding are basically the same thing (ie Body implements From<Vec<u8>>) so the move should be cheap:

error[E0308]: mismatched types --> sdk/core/src/http_client.rs:83:12 | 83 | Ok(hyper_response) | ^^^^^^^^^^^^^^ expected struct `Vec`, found struct `hyper::Body` | = note: expected struct `http::Response<Vec<u8>>` found struct `http::Response<hyper::Body>`

I think in this case you can just use .map to change the body type instead of constructing a whole new response object https://docs.rs/hyper/0.13.9/hyper/struct.Response.html#method.map

That's a great idea!
I've tried it but unfortunately Hyper's Body is a stream so in order to obtain the Vec we must await on body::to_bytes().
Something like (this is as far as I went, it's not working):

Ok(hyper_response.map(async move |b| body::to_bytes(b).await?.to_vec()))

But:

And async closures are unstable 🙅‍♂️ .

We cannot use ? in map.

Map creates a new type anyway so the effect is the same (albeit more verbose).

sdk/core/src/http_client.rs

rylev · 2020-11-12T13:55:04Z

sdk/cosmos/src/client_builder.rs

+use std::marker::PhantomData;
+
+pub fn new<'a, IntoCowStr>(
+    account: IntoCowStr,


Again, I think we should take String instead of Cow<'a, str>

sdk/cosmos/src/client_builder.rs

rylev · 2020-11-12T13:57:24Z

sdk/cosmos/src/clients/cosmos_struct.rs

-            cosmos_uri_builder,
-        })
-    }
+pub struct ClientBuilder<'a, CUB, HTTPClientToAssign>


I really don't think the HttpClientToAssign trick is worth it. Just make the http client required to construct the ClientBuilder

rylev · 2020-11-12T13:58:55Z

sdk/cosmos/src/clients/cosmos_struct.rs

+pub struct ClientBuilder<'a, CUB, HTTPClientToAssign>
+where
+    CUB: CosmosUriBuilder,
+{


Can we create a ClientBuilder::new function?

rylev · 2020-11-12T14:01:40Z

sdk/cosmos/src/clients/permission_struct.rs

-        &self,
-    ) -> &hyper::Client<hyper_rustls::HttpsConnector<hyper::client::HttpConnector>> {
-        self.user_client.hyper_client()
+    fn http_client(&self) -> &dyn HttpClient {


I wonder if returning an Arc<Box<dyn HttpClient>> is a more ergonimic thing to do.

rylev

I'm going to merge this since we have a lot of changes coming for Cosmos. We can revisit this in a future PR to clean up.

rylev · 2020-11-20T14:45:34Z

sdk/core/Cargo.toml

 failure = "0.1"
 async-trait = "0.1.36"
 oauth2 = { version = "4.0.0-alpha.2" }
+reqwest = "*"


Let's use a concrete version not "*"

First function (get_database)

938d069

MindFlavor added feature-request This issue requires a new behavior in the product in order be resolved. Cosmos The azure_cosmos crate labels Nov 12, 2020

fixed create database

5156487

rylev reviewed Nov 12, 2020

View reviewed changes

ctaggart mentioned this pull request Nov 12, 2020

add HttpClientArc & TokenCredentialArc #80

Closed

MindFlavor added 6 commits November 12, 2020 21:06

fixed responses

7b5b853

removed hyper dependency (requests still to correct)

bd0102a

fixed requests

f01edb0

clippy

1deb9e6

fixed examples and e2e tests

77cbce4

fixed e2e tests

0af6b02

MindFlavor marked this pull request as ready for review November 14, 2020 20:06

reenabled services/mgmt/*

a7a3262

MindFlavor mentioned this pull request Nov 19, 2020

Simplify ConsistencyLevel and CosmosStruct #88

Merged

rylev approved these changes Nov 20, 2020

View reviewed changes

rylev merged commit ee7ec92 into master Nov 20, 2020

rylev deleted the pluggable_http_client_cosmos branch November 20, 2020 16:28

Pluggable HTTP client for CosmosDB #79

Pluggable HTTP client for CosmosDB #79

Uh oh!

Conversation

MindFlavor commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intro

Using reqwest

Using hyper

ToDo

Uh oh!

rylev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MindFlavor commented Nov 12, 2020 •

edited

Loading