Changes to IntoHeaderName and IntoHeaderValue

.circleci/config.yml

@@ -13,7 +13,7 @@ executors:
   rust_linux: &rust_linux_executor
     docker:
       - image: cimg/base:stable
-    resource_class: large
+    resource_class: xlarge
   rust_macos: &rust_macos_executor
     macos:
       xcode: 11.7

NEXT_CHANGELOG.md

@@ -53,13 +53,25 @@ Some methods of `apollo_router::TestHarness` were renamed:
 
 By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollographql/router/pull/1579
 
-### Request and Response types from apollo_router::http_ext are private ([Issue #1589](https://github.com/apollographql/router/issues/1589))
+### `Request` and `Response` types from `apollo_router::http_ext` are private ([Issue #1589](https://github.com/apollographql/router/issues/1589))
 
 These types were wrappers around the `Request` and `Response` types from the `http` crate.
 Now the latter are used directly instead.
 
 By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollographql/router/pull/1589
 
+### Changes to `IntoHeaderName` and `IntoHeaderValue` ([PR #1607](https://github.com/apollographql/router/pull/1607))
+
+Note: these types are typically not use directly, so we expect most user code to require no change.
+
+* Move from `apollo_router::http_ext` to `apollo_router::services`
+* Rename to `TryIntoHeaderName` and `TryIntoHeaderValue`
+* Make contents opaque
+* Replace generic `From<T: Display>` conversion with multiple specific conversions
+  that are implemented by `http::headers::Header{Name,Value}`.
+
+By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollographql/router/pull/1607
+
 ### QueryPlan::usage_reporting and QueryPlannerContent are private ([Issue #1556](https://github.com/apollographql/router/issues/1556))
 
 These items have been removed from the public API of `apollo_router::services::execution`.

apollo-router/src/http_ext.rs

@@ -12,119 +12,207 @@ use std::ops::DerefMut;
 use axum::body::boxed;
 use axum::response::IntoResponse;
 use bytes::Bytes;
-use http::header::HeaderName;
-use http::header::{self};
+use http::header;
 use http::HeaderValue;
 use multimap::MultiMap;
 
 use crate::graphql;
 
-/// Temporary holder of header name while for use while building requests and responses. Required
-/// because header name creation is fallible.
-#[derive(Eq)]
-pub enum IntoHeaderName {
-    String(String),
-    HeaderName(HeaderName),
+/// Delayed-fallibility wrapper for conversion to [`http::header::HeaderName`].
+///
+/// `buildstructor` builders allow doing implict conversions for convenience,
+/// but only infallible ones.
+/// `HeaderName` can be converted from various types but the conversions is often fallible,
+/// with `TryFrom` or `TryInto` instead of `From` or `Into`.
+/// This types splits conversion in two steps:
+/// it implements infallible conversion from various types like `&str` (that builders can rely on)
+/// and fallible conversion to `HeaderName` (called later where we can handle errors).
+///
+/// See for example [`supergraph::Request::builder`][crate::services::supergraph::Request::builder]
+/// which can be used like this:
+///
+/// ```
+/// # fn main() -> Result<(), tower::BoxError> {
+/// use apollo_router::services::supergraph;
+/// let request = supergraph::Request::fake_builder()
+///     .header("accept-encoding", "gzip")
+///     // Other parameters
+///     .build()?;
+/// # Ok(()) }
+/// ```
+pub struct TryIntoHeaderName {
+    /// The fallible conversion result
+    result: Result<header::HeaderName, header::InvalidHeaderName>,
+}
+
+/// Delayed-fallibility wrapper for conversion to [`http::header::HeaderValue`].
+///
+/// `buildstructor` builders allow doing implict conversions for convenience,
+/// but only infallible ones.
+/// `HeaderValue` can be converted from various types but the conversions is often fallible,
+/// with `TryFrom` or `TryInto` instead of `From` or `Into`.
+/// This types splits conversion in two steps:
+/// it implements infallible conversion from various types like `&str` (that builders can rely on)
+/// and fallible conversion to `HeaderValue` (called later where we can handle errors).
+///
+/// See for example [`supergraph::Request::builder`][crate::services::supergraph::Request::builder]
+/// which can be used like this:
+///
+/// ```
+/// # fn main() -> Result<(), tower::BoxError> {
+/// use apollo_router::services::supergraph;
+/// let request = supergraph::Request::fake_builder()
+///     .header("accept-encoding", "gzip")
+///     // Other parameters
+///     .build()?;
+/// # Ok(()) }
+/// ```
+pub struct TryIntoHeaderValue {
+    /// The fallible conversion result
+    result: Result<header::HeaderValue, header::InvalidHeaderValue>,
+}
+
+impl TryFrom<TryIntoHeaderName> for header::HeaderName {
+    type Error = header::InvalidHeaderName;
+
+    fn try_from(value: TryIntoHeaderName) -> Result<Self, Self::Error> {
+        value.result
+    }
 }
 
-/// Temporary holder of header value while for use while building requests and responses. Required
-/// because header value creation is fallible.
-#[derive(Eq)]
-pub enum IntoHeaderValue {
-    String(String),
-    HeaderValue(HeaderValue),
+impl TryFrom<TryIntoHeaderValue> for header::HeaderValue {
+    type Error = header::InvalidHeaderValue;
+
+    fn try_from(value: TryIntoHeaderValue) -> Result<Self, Self::Error> {
+        value.result
+    }
 }
 
-impl PartialEq for IntoHeaderName {
-    fn eq(&self, other: &Self) -> bool {
-        self.as_bytes() == other.as_bytes()
+impl From<header::HeaderName> for TryIntoHeaderName {
+    fn from(value: header::HeaderName) -> Self {
+        Self { result: Ok(value) }
     }
 }
 
-impl PartialEq for IntoHeaderValue {
-    fn eq(&self, other: &Self) -> bool {
-        self.as_bytes() == other.as_bytes()
+impl From<&'_ header::HeaderName> for TryIntoHeaderName {
+    fn from(value: &'_ header::HeaderName) -> Self {
+        Self {
+            result: Ok(value.clone()),
+        }
     }
 }
 
-impl Hash for IntoHeaderName {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        self.as_bytes().hash(state);
+impl From<&'_ [u8]> for TryIntoHeaderName {
+    fn from(value: &'_ [u8]) -> Self {
+        Self {
+            result: value.try_into(),
+        }
     }
 }
 
-impl Hash for IntoHeaderValue {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        self.as_bytes().hash(state);
+impl From<&'_ str> for TryIntoHeaderName {
+    fn from(value: &'_ str) -> Self {
+        Self {
+            result: value.try_into(),
+        }
     }
 }
 
-impl IntoHeaderName {
-    fn as_bytes(&self) -> &[u8] {
-        match self {
-            IntoHeaderName::String(s) => s.as_bytes(),
-            IntoHeaderName::HeaderName(h) => h.as_str().as_bytes(),
+impl From<Vec<u8>> for TryIntoHeaderName {
+    fn from(value: Vec<u8>) -> Self {
+        Self {
+            result: value.try_into(),
         }
     }
 }
 
-impl IntoHeaderValue {
-    fn as_bytes(&self) -> &[u8] {
-        match self {
-            IntoHeaderValue::String(s) => s.as_bytes(),
-            IntoHeaderValue::HeaderValue(v) => v.as_bytes(),
+impl From<String> for TryIntoHeaderName {
+    fn from(value: String) -> Self {
+        Self {
+            result: value.try_into(),
         }
     }
 }
 
-impl<T> From<T> for IntoHeaderName
-where
-    T: std::fmt::Display,
-{
-    fn from(name: T) -> Self {
-        IntoHeaderName::String(name.to_string())
+impl From<header::HeaderValue> for TryIntoHeaderValue {
+    fn from(value: header::HeaderValue) -> Self {
+        Self { result: Ok(value) }
     }
 }
 
-impl<T> From<T> for IntoHeaderValue
-where
-    T: std::fmt::Display,
-{
-    fn from(name: T) -> Self {
-        IntoHeaderValue::String(name.to_string())
+impl From<&'_ header::HeaderValue> for TryIntoHeaderValue {
+    fn from(value: &'_ header::HeaderValue) -> Self {
+        Self {
+            result: Ok(value.clone()),
+        }
     }
 }
 
-impl TryFrom<IntoHeaderName> for HeaderName {
-    type Error = http::Error;
+impl From<&'_ [u8]> for TryIntoHeaderValue {
+    fn from(value: &'_ [u8]) -> Self {
+        Self {
+            result: value.try_into(),
+        }
+    }
+}
 
-    fn try_from(value: IntoHeaderName) -> Result<Self, Self::Error> {
-        Ok(match value {
-            IntoHeaderName::String(name) => HeaderName::try_from(name)?,
-            IntoHeaderName::HeaderName(name) => name,
-        })
+impl From<&'_ str> for TryIntoHeaderValue {
+    fn from(value: &'_ str) -> Self {
+        Self {
+            result: value.try_into(),
+        }
+    }
+}
+
+impl From<Vec<u8>> for TryIntoHeaderValue {
+    fn from(value: Vec<u8>) -> Self {
+        Self {
+            result: value.try_into(),
+        }
+    }
+}
+
+impl From<String> for TryIntoHeaderValue {
+    fn from(value: String) -> Self {
+        Self {
+            result: value.try_into(),
+        }
     }
 }
 
-impl TryFrom<IntoHeaderValue> for HeaderValue {
-    type Error = http::Error;
+impl Eq for TryIntoHeaderName {}
+
+impl PartialEq for TryIntoHeaderName {
+    fn eq(&self, other: &Self) -> bool {
+        match (&self.result, &other.result) {
+            (Ok(a), Ok(b)) => a == b,
+            (Err(_), Err(_)) => true,
+            _ => false,
+        }
+    }
+}
 
-    fn try_from(value: IntoHeaderValue) -> Result<Self, Self::Error> {
-        Ok(match value {
-            IntoHeaderValue::String(value) => HeaderValue::try_from(value)?,
-            IntoHeaderValue::HeaderValue(value) => value,
-        })
+impl Hash for TryIntoHeaderName {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        match &self.result {
+            Ok(value) => value.hash(state),
+            Err(_) => {}
+        }
     }
 }
 
 pub(crate) fn header_map(
-    from: MultiMap<IntoHeaderName, IntoHeaderValue>,
+    from: MultiMap<TryIntoHeaderName, TryIntoHeaderValue>,
 ) -> Result<http::HeaderMap<http::HeaderValue>, http::Error> {
     let mut http = http::HeaderMap::new();
     for (key, values) in from {
-        let name = http::header::HeaderName::try_from(key)?;
-        for value in values {
-            http.append(name.clone(), value.try_into()?);
+        let name = key.result?;
+        let mut values = values.into_iter();
+        if let Some(last) = values.next_back() {
+            for value in values {
+                http.append(name.clone(), value.result?);
+            }
+            http.append(name, last.result?);
         }
     }
     Ok(http)
@@ -167,7 +255,7 @@ impl<T> Request<T> {
     /// Required parameters are required in non-testing code to create a Request.
     #[builder]
     pub(crate) fn new(
-        headers: MultiMap<IntoHeaderName, IntoHeaderValue>,
+        headers: MultiMap<TryIntoHeaderName, TryIntoHeaderValue>,
         uri: http::Uri,
         method: http::Method,
         body: T,
@@ -193,7 +281,7 @@ impl<T> Request<T> {
     /// In addition, fake requests are expected to be valid, and will panic if given invalid values.
     #[builder]
     pub(crate) fn fake_new(
-        headers: MultiMap<IntoHeaderName, IntoHeaderValue>,
+        headers: MultiMap<TryIntoHeaderName, TryIntoHeaderValue>,
         uri: Option<http::Uri>,
         method: Option<http::Method>,
         body: T,

apollo-router/src/lib.rs

@@ -60,7 +60,7 @@ pub mod error;
 mod executable;
 mod files;
 pub mod graphql;
-pub mod http_ext;
+mod http_ext;
 mod http_server_factory;
 mod introspection;
 pub mod layers;

apollo-router/src/services/execution.rs

@@ -16,8 +16,8 @@ use tower::BoxError;
 
 use crate::error::Error;
 use crate::graphql;
-use crate::http_ext::IntoHeaderName;
-use crate::http_ext::IntoHeaderValue;
+use crate::http_ext::TryIntoHeaderName;
+use crate::http_ext::TryIntoHeaderValue;
 use crate::json_ext::Object;
 use crate::json_ext::Path;
 use crate::Context;
@@ -153,7 +153,7 @@ impl Response {
     fn error_new(
         errors: Vec<Error>,
         status_code: Option<StatusCode>,
-        headers: MultiMap<IntoHeaderName, IntoHeaderValue>,
+        headers: MultiMap<TryIntoHeaderName, TryIntoHeaderValue>,
         context: Context,
     ) -> Result<Self, BoxError> {
         Ok(Response::new(

apollo-router/src/services/mod.rs

@@ -8,6 +8,8 @@ pub(crate) use self::subgraph_service::*;
 pub(crate) use self::supergraph_service::*;
 use crate::graphql::Request;
 use crate::http_ext;
+pub use crate::http_ext::TryIntoHeaderName;
+pub use crate::http_ext::TryIntoHeaderValue;
 pub(crate) use crate::services::execution::Request as ExecutionRequest;
 pub(crate) use crate::services::execution::Response as ExecutionResponse;
 pub(crate) use crate::services::query_planner::Request as QueryPlannerRequest;