diff --git a/.circleci/config.yml b/.circleci/config.yml index 81fa7244cd..319b2ccb1a 100644 --- a/.circleci/config.yml +++ b/.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 diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 2268b84ec2..812f50f8cf 100644 --- a/NEXT_CHANGELOG.md +++ b/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` 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`. diff --git a/apollo-router/src/http_ext.rs b/apollo-router/src/http_ext.rs index 8fd649b64a..06b7781d7e 100644 --- a/apollo-router/src/http_ext.rs +++ b/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, +} + +/// 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, +} + +impl TryFrom for header::HeaderName { + type Error = header::InvalidHeaderName; + + fn try_from(value: TryIntoHeaderName) -> Result { + 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 for header::HeaderValue { + type Error = header::InvalidHeaderValue; + + fn try_from(value: TryIntoHeaderValue) -> Result { + value.result + } } -impl PartialEq for IntoHeaderName { - fn eq(&self, other: &Self) -> bool { - self.as_bytes() == other.as_bytes() +impl From 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(&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(&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> for TryIntoHeaderName { + fn from(value: Vec) -> 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 for TryIntoHeaderName { + fn from(value: String) -> Self { + Self { + result: value.try_into(), } } } -impl From for IntoHeaderName -where - T: std::fmt::Display, -{ - fn from(name: T) -> Self { - IntoHeaderName::String(name.to_string()) +impl From for TryIntoHeaderValue { + fn from(value: header::HeaderValue) -> Self { + Self { result: Ok(value) } } } -impl From 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 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 { - 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> for TryIntoHeaderValue { + fn from(value: Vec) -> Self { + Self { + result: value.try_into(), + } + } +} + +impl From for TryIntoHeaderValue { + fn from(value: String) -> Self { + Self { + result: value.try_into(), + } } } -impl TryFrom 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 { - Ok(match value { - IntoHeaderValue::String(value) => HeaderValue::try_from(value)?, - IntoHeaderValue::HeaderValue(value) => value, - }) +impl Hash for TryIntoHeaderName { + fn hash(&self, state: &mut H) { + match &self.result { + Ok(value) => value.hash(state), + Err(_) => {} + } } } pub(crate) fn header_map( - from: MultiMap, + from: MultiMap, ) -> Result, 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 Request { /// Required parameters are required in non-testing code to create a Request. #[builder] pub(crate) fn new( - headers: MultiMap, + headers: MultiMap, uri: http::Uri, method: http::Method, body: T, @@ -193,7 +281,7 @@ impl Request { /// In addition, fake requests are expected to be valid, and will panic if given invalid values. #[builder] pub(crate) fn fake_new( - headers: MultiMap, + headers: MultiMap, uri: Option, method: Option, body: T, diff --git a/apollo-router/src/lib.rs b/apollo-router/src/lib.rs index 5d2ca4dd08..ab6e77399d 100644 --- a/apollo-router/src/lib.rs +++ b/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; diff --git a/apollo-router/src/services/execution.rs b/apollo-router/src/services/execution.rs index 821dfdeae8..f4c4081a7c 100644 --- a/apollo-router/src/services/execution.rs +++ b/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, status_code: Option, - headers: MultiMap, + headers: MultiMap, context: Context, ) -> Result { Ok(Response::new( diff --git a/apollo-router/src/services/mod.rs b/apollo-router/src/services/mod.rs index e77c98e4ca..4d872299ed 100644 --- a/apollo-router/src/services/mod.rs +++ b/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; diff --git a/apollo-router/src/services/supergraph.rs b/apollo-router/src/services/supergraph.rs index f1c3aa0cdb..b8f182f14b 100644 --- a/apollo-router/src/services/supergraph.rs +++ b/apollo-router/src/services/supergraph.rs @@ -19,8 +19,8 @@ use tower::BoxError; use crate::error::Error; use crate::graphql; use crate::http_ext::header_map; -use crate::http_ext::IntoHeaderName; -use crate::http_ext::IntoHeaderValue; +use crate::http_ext::TryIntoHeaderName; +use crate::http_ext::TryIntoHeaderValue; use crate::json_ext::Path; use crate::Context; @@ -63,7 +63,7 @@ impl Request { variables: JsonMap, extensions: JsonMap, context: Context, - headers: MultiMap, + headers: MultiMap, uri: Uri, method: Method, ) -> Result { @@ -99,15 +99,13 @@ impl Request { variables: JsonMap, extensions: JsonMap, context: Option, - mut headers: MultiMap, + mut headers: MultiMap, method: Option, ) -> Result { // Avoid testing requests getting blocked by the CSRF-prevention plugin headers - .entry(IntoHeaderName::HeaderName(http::header::CONTENT_TYPE)) - .or_insert(IntoHeaderValue::HeaderValue(HeaderValue::from_static( - "application/json", - ))); + .entry(http::header::CONTENT_TYPE.into()) + .or_insert(HeaderValue::from_static("application/json").into()); Request::new( query, operation_name, @@ -127,7 +125,7 @@ impl Request { // Skip the `Object` type alias in order to use buildstructor’s map special-casing extensions: JsonMap, context: Option, - headers: MultiMap, + headers: MultiMap, ) -> Result { let query = " query TopProducts($first: Int) { @@ -176,7 +174,7 @@ impl Response { // Skip the `Object` type alias in order to use buildstructor’s map special-casing extensions: JsonMap, status_code: Option, - headers: MultiMap, + headers: MultiMap, context: Context, ) -> Result { // Build a response @@ -220,7 +218,7 @@ impl Response { // Skip the `Object` type alias in order to use buildstructor’s map special-casing extensions: JsonMap, status_code: Option, - headers: MultiMap, + headers: MultiMap, context: Option, ) -> Result { Response::new( @@ -241,7 +239,7 @@ impl Response { fn error_new( errors: Vec, status_code: Option, - headers: MultiMap, + headers: MultiMap, context: Context, ) -> Result { Response::new( diff --git a/examples/jwt-auth/src/jwt.rs b/examples/jwt-auth/src/jwt.rs index 813f46c640..fa1d0dc4b8 100644 --- a/examples/jwt-auth/src/jwt.rs +++ b/examples/jwt-auth/src/jwt.rs @@ -615,7 +615,7 @@ mod tests { // Let's create a request with a properly formatted authorization header let request_with_appropriate_auth = supergraph::Request::fake_builder() - .header("authorization", &format!("Bearer {token}")) + .header("authorization", format!("Bearer {token}")) .build() .expect("expecting valid request");