Clean up handling of Accept header.

src/api.rs

@@ -593,8 +593,16 @@ impl<State, Error> Api<State, Error> {
         } else {
             // If there is no healthcheck handler registered, just return [HealthStatus::Available]
             // by default; after all, if this handler is getting hit at all, the service must be up.
-            let mut accept = Accept::from_headers(req.headers()).ok().flatten();
-            route::health_check_response(&mut accept, HealthStatus::Available)
+            route::health_check_response(
+                &req.accept().unwrap_or_else(|_| {
+                    // The healthcheck endpoint is not allowed to fail, so just use the default content
+                    // type if we can't parse the Accept header.
+                    let mut accept = Accept::new();
+                    accept.set_wildcard(true);
+                    accept
+                }),
+                HealthStatus::Available,
+            )
         }
     }
 
@@ -653,8 +661,11 @@ where
         req: RequestParams,
         state: &State,
     ) -> Result<tide::Response, RouteError<Error>> {
-        let accept = accept_header(&req)?;
-        response_from_result(accept, state.read(|state| (self.handler)(req, state)).await)
+        let accept = req.accept()?;
+        response_from_result(
+            &accept,
+            state.read(|state| (self.handler)(req, state)).await,
+        )
     }
 }
 
@@ -679,9 +690,9 @@ where
         req: RequestParams,
         state: &State,
     ) -> Result<tide::Response, RouteError<Error>> {
-        let accept = accept_header(&req)?;
+        let accept = req.accept()?;
         response_from_result(
-            accept,
+            &accept,
             state.write(|state| (self.handler)(req, state)).await,
         )
     }

src/app.rs

@@ -27,8 +27,7 @@ use std::collections::hash_map::{Entry, HashMap};
 use std::convert::Infallible;
 use std::io;
 use tide::{
-    http::headers::HeaderValue,
-    http::{content::Accept, mime},
+    http::{headers::HeaderValue, mime},
     security::{CorsMiddleware, Origin},
     StatusCode,
 };
@@ -235,9 +234,10 @@ impl<State: Send + Sync + 'static, Error: 'static + crate::Error> App<State, Err
                         let prefix = prefix.clone();
                         async move {
                             let api = &req.state().apis[&prefix];
-                            respond_with(&mut Accept::from_headers(&req)?, api.version()).map_err(
-                                |err| Error::from_route_error::<Infallible>(err).into_tide_error(),
-                            )
+                            let accept = RequestParams::accept_from_headers(&req)?;
+                            respond_with(&accept, api.version()).map_err(|err| {
+                                Error::from_route_error::<Infallible>(err).into_tide_error()
+                            })
                         }
                     });
             }
@@ -250,14 +250,15 @@ impl<State: Send + Sync + 'static, Error: 'static + crate::Error> App<State, Err
                 let state = req.state().clone();
                 let app_state = &*state.state;
                 let req = request_params(req, &[]).await?;
-                let mut accept = Accept::from_headers(req.headers())?;
+                let accept = req.accept()?;
                 let res = state.health(req, app_state).await;
-                Ok(health_check_response(&mut accept, res))
+                Ok(health_check_response(&accept, res))
             });
         server
             .at("version")
             .get(|req: tide::Request<Arc<Self>>| async move {
-                respond_with(&mut Accept::from_headers(&req)?, req.state().version())
+                let accept = RequestParams::accept_from_headers(&req)?;
+                respond_with(&accept, req.state().version())
                     .map_err(|err| Error::from_route_error::<Infallible>(err).into_tide_error())
             });
 
@@ -344,15 +345,15 @@ fn add_error_body<T: Clone + Send + Sync + 'static, E: crate::Error>(
     next: tide::Next<T>,
 ) -> BoxFuture<tide::Result> {
     Box::pin(async {
-        let mut accept = Accept::from_headers(&req)?;
+        let accept = RequestParams::accept_from_headers(&req)?;
         let mut res = next.run(req).await;
         if let Some(error) = res.take_error() {
             let error = E::from_server_error(error);
             tracing::warn!("responding with error: {}", error);
             // Try to add the error to the response body using a format accepted by the client. If
             // we cannot do that (for example, if the client requested a format that is incompatible
             // with a serialized error) just add the error as a string using plaintext.
-            let (body, content_type) = route::response_body::<_, E>(&mut accept, &error)
+            let (body, content_type) = route::response_body::<_, E>(&accept, &error)
                 .unwrap_or_else(|_| (error.to_string().into(), mime::PLAIN));
             res.set_body(body);
             res.set_content_type(content_type);

src/request.rs

@@ -3,7 +3,7 @@ use std::collections::HashMap;
 use std::fmt::Display;
 use strum_macros::EnumString;
 use tagged_base64::TaggedBase64;
-use tide::http::Headers;
+use tide::http::{content::Accept, Headers};
 
 #[derive(Clone, Debug, Snafu)]
 pub enum RequestError {
@@ -37,6 +37,9 @@ pub enum RequestError {
 
     #[snafu(display("Body type not specified or type not supported"))]
     UnsupportedBody,
+
+    #[snafu(display("HTTP protocol error: {}", reason))]
+    Http { reason: String },
 }
 
 /// Parameters passed to a route handler.
@@ -73,6 +76,37 @@ impl RequestParams {
         &self.headers
     }
 
+    /// The [Accept] header of this request.
+    ///
+    /// The media type proposals in the resulting header are sorted in order of decreasing weight.
+    ///
+    /// If no [Accept] header was explicitly set, defaults to the wildcard `Accept: *`.
+    ///
+    /// # Error
+    ///
+    /// Returns [RequestError::Http] if the [Accept] header is malformed.
+    pub fn accept(&self) -> Result<Accept, RequestError> {
+        Self::accept_from_headers(&self.headers)
+    }
+
+    pub(crate) fn accept_from_headers(
+        headers: impl AsRef<Headers>,
+    ) -> Result<Accept, RequestError> {
+        match Accept::from_headers(headers).map_err(|err| RequestError::Http {
+            reason: err.to_string(),
+        })? {
+            Some(mut accept) => {
+                accept.sort();
+                Ok(accept)
+            }
+            None => {
+                let mut accept = Accept::new();
+                accept.set_wildcard(true);
+                Ok(accept)
+            }
+        }
+    }
+
     /// Get the value of a named parameter.
     ///
     /// The name of the parameter can be given by any type that implements [Display]. Of course, the

src/route.rs

@@ -1,6 +1,6 @@
 use crate::{
     healthcheck::HealthCheck,
-    request::{RequestParam, RequestParamType, RequestParams},
+    request::{RequestError, RequestParam, RequestParamType, RequestParams},
 };
 use async_trait::async_trait;
 use derive_more::From;
@@ -29,6 +29,7 @@ use tide::{
 /// result of the user-installed handler into an HTTP response.
 pub enum RouteError<E> {
     AppSpecific(E),
+    Request(RequestError),
     UnsupportedContentType,
     Bincode(bincode::Error),
     Json(serde_json::Error),
@@ -39,6 +40,7 @@ impl<E: Display> Display for RouteError<E> {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self {
             Self::AppSpecific(err) => write!(f, "{}", err),
+            Self::Request(err) => write!(f, "{}", err),
             Self::UnsupportedContentType => write!(f, "requested content type is not supported"),
             Self::Bincode(err) => write!(f, "error creating byte stream: {}", err),
             Self::Json(err) => write!(f, "error creating JSON response: {}", err),
@@ -50,14 +52,15 @@ impl<E: Display> Display for RouteError<E> {
 impl<E> RouteError<E> {
     pub fn status(&self) -> StatusCode {
         match self {
-            Self::UnsupportedContentType => StatusCode::BadRequest,
+            Self::Request(_) | Self::UnsupportedContentType => StatusCode::BadRequest,
             _ => StatusCode::InternalServerError,
         }
     }
 
     pub fn map_app_specific<E2>(self, f: impl Fn(E) -> E2) -> RouteError<E2> {
         match self {
             RouteError::AppSpecific(e) => RouteError::AppSpecific(f(e)),
+            RouteError::Request(e) => RouteError::Request(e),
             RouteError::UnsupportedContentType => RouteError::UnsupportedContentType,
             RouteError::Bincode(err) => RouteError::Bincode(err),
             RouteError::Json(err) => RouteError::Json(err),
@@ -66,6 +69,12 @@ impl<E> RouteError<E> {
     }
 }
 
+impl<E> From<RequestError> for RouteError<E> {
+    fn from(err: RequestError) -> Self {
+        Self::Request(err)
+    }
+}
+
 /// A route handler.
 ///
 /// The [Handler] trait defines the interface required of route handlers -- they must be able to
@@ -115,23 +124,17 @@ where
         req: RequestParams,
         state: &State,
     ) -> Result<tide::Response, RouteError<Error>> {
-        let accept = accept_header(&req)?;
-        response_from_result(accept, (self.0)(req, state).await)
+        let accept = req.accept()?;
+        response_from_result(&accept, (self.0)(req, state).await)
     }
 }
 
-pub(crate) fn accept_header<Error>(
-    req: &RequestParams,
-) -> Result<Option<Accept>, RouteError<Error>> {
-    Accept::from_headers(req.headers()).map_err(RouteError::Tide)
-}
-
 pub(crate) fn response_from_result<T: Serialize, Error>(
-    mut accept: Option<Accept>,
+    accept: &Accept,
     res: Result<T, Error>,
 ) -> Result<tide::Response, RouteError<Error>> {
     res.map_err(RouteError::AppSpecific)
-        .and_then(|res| respond_with(&mut accept, &res))
+        .and_then(|res| respond_with(accept, &res))
 }
 
 #[async_trait]
@@ -399,10 +402,7 @@ where
 pub(crate) type HealthCheckHandler<State> =
     Box<dyn 'static + Send + Sync + Fn(RequestParams, &State) -> BoxFuture<'_, tide::Response>>;
 
-pub(crate) fn health_check_response<H: HealthCheck>(
-    accept: &mut Option<Accept>,
-    health: H,
-) -> tide::Response {
+pub(crate) fn health_check_response<H: HealthCheck>(accept: &Accept, health: H) -> tide::Response {
     let status = health.status();
     let (body, content_type) =
         response_body::<H, Infallible>(accept, health).unwrap_or_else(|err| {
@@ -431,67 +431,59 @@ where
     F: 'static + Send + Future<Output = H>,
 {
     Box::new(move |req, state| {
-        let mut accept = Accept::from_headers(req.headers()).ok().flatten();
+        let accept = req.accept().unwrap_or_else(|_| {
+            // The healthcheck endpoint is not allowed to fail, so just use the default content
+            // type if we can't parse the Accept header.
+            let mut accept = Accept::new();
+            accept.set_wildcard(true);
+            accept
+        });
         let future = handler(state);
         async move {
             let health = future.await;
-            health_check_response(&mut accept, health)
+            health_check_response(&accept, health)
         }
         .boxed()
     })
 }
 
-fn best_response_type<E>(
-    accept: &mut Option<Accept>,
-    available: &[Mime],
-) -> Result<Mime, RouteError<E>> {
-    match accept {
-        Some(accept) => {
-            // The Accept type has a `negotiate` method, but it doesn't properly handle
-            // wildcards. It handles * but not */* and basetype/*, because for content type
-            // proposals like */* and basetype/*, it looks for a literal match in `available`,
-            // it does not perform pattern matching. So, we implement negotiation ourselves.
-            //
-            // First sort by the weight parameter, which the Accept type does do correctly.
-            accept.sort();
-            // Go through each proposed content type, in the order specified by the client, and
-            // match them against our available types, respecting wildcards.
-            for proposed in accept.iter() {
-                if proposed.basetype() == "*" {
-                    // The only acceptable Accept value with a basetype of * is */*, therefore
-                    // this will match any available type.
-                    return Ok(available[0].clone());
-                } else if proposed.subtype() == "*" {
-                    // If the subtype is * but the basetype is not, look for a proposed type
-                    // with a matching basetype and any subtype.
-                    for mime in available {
-                        if mime.basetype() == proposed.basetype() {
-                            return Ok(mime.clone());
-                        }
-                    }
-                } else if available.contains(proposed) {
-                    // If neither part of the proposal is a wildcard, look for a literal match.
-                    return Ok((**proposed).clone());
+fn best_response_type<E>(accept: &Accept, available: &[Mime]) -> Result<Mime, RouteError<E>> {
+    // The Accept type has a `negotiate` method, but it doesn't properly handle wildcards. It
+    // handles * but not */* and basetype/*, because for content type proposals like */* and
+    // basetype/*, it looks for a literal match in `available`, it does not perform pattern
+    // matching. So, we implement negotiation ourselves. Go through each proposed content type, in
+    // the order specified by the client, and match them against our available types, respecting
+    // wildcards.
+    for proposed in accept.iter() {
+        if proposed.basetype() == "*" {
+            // The only acceptable Accept value with a basetype of * is */*, therefore this will
+            // match any available type.
+            return Ok(available[0].clone());
+        } else if proposed.subtype() == "*" {
+            // If the subtype is * but the basetype is not, look for a proposed type with a matching
+            // basetype and any subtype.
+            for mime in available {
+                if mime.basetype() == proposed.basetype() {
+                    return Ok(mime.clone());
                 }
             }
-
-            if accept.wildcard() {
-                // If no proposals are available but a wildcard flag * was given, return any
-                // available content type.
-                Ok(available[0].clone())
-            } else {
-                Err(RouteError::UnsupportedContentType)
-            }
-        }
-        None => {
-            // If no content type is explicitly requested, default to the first available type.
-            Ok(available[0].clone())
+        } else if available.contains(proposed) {
+            // If neither part of the proposal is a wildcard, look for a literal match.
+            return Ok((**proposed).clone());
         }
     }
+
+    if accept.wildcard() {
+        // If no proposals are available but a wildcard flag * was given, return any available
+        // content type.
+        Ok(available[0].clone())
+    } else {
+        Err(RouteError::UnsupportedContentType)
+    }
 }
 
 pub(crate) fn response_body<T: Serialize, E>(
-    accept: &mut Option<Accept>,
+    accept: &Accept,
     body: T,
 ) -> Result<(Body, Mime), RouteError<E>> {
     let ty = best_response_type(accept, &[mime::JSON, mime::BYTE_STREAM])?;
@@ -507,7 +499,7 @@ pub(crate) fn response_body<T: Serialize, E>(
 }
 
 pub(crate) fn respond_with<T: Serialize, E>(
-    accept: &mut Option<Accept>,
+    accept: &Accept,
     body: T,
 ) -> Result<tide::Response, RouteError<E>> {
     let (body, content_type) = response_body(accept, body)?;