Clean up TODO and implement an echo test case

Author: bspeice

Cargo.toml

@@ -68,5 +68,6 @@ features = [
 
 [dev-dependencies]
 async-std = { version = "1.6.0", features = ["unstable", "attributes"] }
-tide = { version = "0.9.0" }
 portpicker = "0.1.0"
+tide = { version = "0.9.0" }
+tokio = { version = "0.2.21", features = ["macros"] }

src/hyper.rs

@@ -1,14 +1,25 @@
 //! http-client implementation for reqwest
+
 use super::{Error, HttpClient, Request, Response};
-use http_types::StatusCode;
+use http_types::headers::{HeaderName, HeaderValue};
+use http_types::{Method, StatusCode, Version};
 use hyper::body::HttpBody;
 use hyper_tls::HttpsConnector;
 use std::convert::{TryFrom, TryInto};
 use std::str::FromStr;
 
 /// Hyper-based HTTP Client.
 #[derive(Debug)]
-struct HyperClient {}
+pub struct HyperClient {}
+
+impl HyperClient {
+    /// Create a new client.
+    ///
+    /// There is no specific benefit to reusing instances of this client.
+    pub fn new() -> Self {
+        HyperClient {}
+    }
+}
 
 impl HttpClient for HyperClient {
     fn send(&self, req: Request) -> futures::future::BoxFuture<'static, Result<Response, Error>> {
@@ -41,23 +52,32 @@ struct HyperHttpRequest {
 }
 
 impl HyperHttpRequest {
-    async fn try_from(mut value: http_types::Request) -> Result<Self, Error> {
-        // Note: Much of this code was taken from the `http-types` compat implementation. Trying to
-        // figure out the feature flags to conditionally compile with compat support was rather
-        // difficult, so copying code was deemed a reasonable intermediate solution.
-        // Also, because converting the `http_types` body to bytes is async, we can't implement `TryFrom`
-
-        // TODO: Do this without a `String` allocation
-        let method = hyper::Method::from_str(&value.method().to_string()).unwrap();
+    async fn try_from(mut value: Request) -> Result<Self, Error> {
+        let method = match value.method() {
+            Method::Get => hyper::Method::GET,
+            Method::Head => hyper::Method::HEAD,
+            Method::Post => hyper::Method::POST,
+            Method::Put => hyper::Method::PUT,
+            Method::Patch => hyper::Method::PATCH,
+            Method::Options => hyper::Method::OPTIONS,
+            Method::Trace => hyper::Method::TRACE,
+            Method::Connect => hyper::Method::CONNECT,
+            _ => {
+                return Err(Error::from_str(
+                    StatusCode::BadRequest,
+                    "unrecognized HTTP method",
+                ))
+            }
+        };
 
         let version = value
             .version()
             .map(|v| match v {
-                http_types::Version::Http0_9 => Ok(hyper::Version::HTTP_09),
-                http_types::Version::Http1_0 => Ok(hyper::Version::HTTP_10),
-                http_types::Version::Http1_1 => Ok(hyper::Version::HTTP_11),
-                http_types::Version::Http2_0 => Ok(hyper::Version::HTTP_2),
-                http_types::Version::Http3_0 => Ok(hyper::Version::HTTP_3),
+                Version::Http0_9 => Ok(hyper::Version::HTTP_09),
+                Version::Http1_0 => Ok(hyper::Version::HTTP_10),
+                Version::Http1_1 => Ok(hyper::Version::HTTP_11),
+                Version::Http2_0 => Ok(hyper::Version::HTTP_2),
+                Version::Http3_0 => Ok(hyper::Version::HTTP_3),
                 _ => Err(Error::from_str(
                     StatusCode::BadRequest,
                     "unrecognized HTTP version",
@@ -109,14 +129,11 @@ impl HyperHttpRequest {
 }
 
 struct HttpTypesResponse {
-    inner: http_types::Response,
+    inner: Response,
 }
 
 impl HttpTypesResponse {
     async fn try_from(value: hyper::Response<hyper::Body>) -> Result<Self, Error> {
-        // Note: Much of this code was taken from the `http-types` compat implementation. Trying to
-        // figure out the feature flags to conditionally compile with compat support was rather
-        // difficult, so copying code was deemed a reasonable intermediate solution.
         let (parts, mut body) = value.into_parts();
 
         // UNWRAP: http and http-types implement the same status codes
@@ -128,9 +145,8 @@ impl HttpTypesResponse {
             hyper::Version::HTTP_11 => Ok(http_types::Version::Http1_1),
             hyper::Version::HTTP_2 => Ok(http_types::Version::Http2_0),
             hyper::Version::HTTP_3 => Ok(http_types::Version::Http3_0),
-            // TODO: Is this realistically reachable, and should it be marked BadRequest?
             _ => Err(Error::from_str(
-                StatusCode::BadRequest,
+                StatusCode::BadGateway,
                 "unrecognized HTTP response version",
             )),
         }?;
@@ -140,26 +156,24 @@ impl HttpTypesResponse {
             Some(Ok(b)) => Some(b),
             Some(Err(_)) => {
                 return Err(Error::from_str(
-                    StatusCode::BadRequest,
+                    StatusCode::BadGateway,
                     "unable to read HTTP response body",
                 ))
             }
         }
         .map(|b| http_types::Body::from_bytes(b.to_vec()))
-        // TODO: How does `http-types` handle responses without bodies?
-        .unwrap_or(http_types::Body::from_bytes(Vec::new()));
+        .unwrap_or(http_types::Body::empty());
 
         let mut res = Response::new(status);
         res.set_version(Some(version));
 
         for (name, value) in parts.headers {
-            // TODO: http_types uses an `unsafe` block here, should it be allowed for `hyper` as well?
             let value = value.as_bytes().to_owned();
-            let value = http_types::headers::HeaderValue::from_bytes(value)?;
+            let value = HeaderValue::from_bytes(value)?;
 
             if let Some(name) = name {
                 let name = name.as_str();
-                let name = http_types::headers::HeaderName::from_str(name)?;
+                let name = HeaderName::from_str(name)?;
                 res.insert_header(name, value);
             }
         }
@@ -168,7 +182,55 @@ impl HttpTypesResponse {
         Ok(HttpTypesResponse { inner: res })
     }
 
-    fn into_inner(self) -> http_types::Response {
+    fn into_inner(self) -> Response {
         self.inner
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::{Error, HttpClient};
+    use http_types::{Method, Request, Url};
+    use hyper::service::{make_service_fn, service_fn};
+    use std::time::Duration;
+    use tokio::sync::oneshot::channel;
+
+    use super::HyperClient;
+
+    async fn echo(
+        req: hyper::Request<hyper::Body>,
+    ) -> Result<hyper::Response<hyper::Body>, hyper::Error> {
+        Ok(hyper::Response::new(req.into_body()))
+    }
+
+    #[tokio::test]
+    async fn basic_functionality() {
+        let (send, recv) = channel::<()>();
+
+        let recv = async move { recv.await.unwrap_or(()) };
+
+        let addr = ([127, 0, 0, 1], portpicker::pick_unused_port().unwrap()).into();
+        let service = make_service_fn(|_| async { Ok::<_, hyper::Error>(service_fn(echo)) });
+        let server = hyper::Server::bind(&addr)
+            .serve(service)
+            .with_graceful_shutdown(recv);
+
+        let client = HyperClient::new();
+        let url = Url::parse(&format!("http://localhost:{}", addr.port())).unwrap();
+        let mut req = Request::new(Method::Get, url);
+        req.set_body("hello");
+
+        let client = async move {
+            tokio::time::delay_for(Duration::from_millis(100)).await;
+            let mut resp = client.send(req).await?;
+            send.send(()).unwrap();
+            assert_eq!(resp.body_string().await?, "hello");
+
+            Result::<(), Error>::Ok(())
+        };
+
+        let (client_res, server_res) = tokio::join!(client, server);
+        assert!(client_res.is_ok());
+        assert!(server_res.is_ok());
+    }
+}