From e4228fb77c644d4d24f36a3af1748440840c8571 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 28 May 2024 22:13:51 -0400 Subject: [PATCH] Fix preferred encoding computation (#1355) * Decides which encoding to use depending on both `Accept-Encode` and the preferred encoding (chooses the preferred one in case brotli and gzip have the same q value) * Uses `gzip` by default This will fix #1315 --- martin/src/srv/tiles.rs | 69 ++++++++++++++++++++++++++-------- martin/tests/mb_server_test.rs | 10 ++--- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/martin/src/srv/tiles.rs b/martin/src/srv/tiles.rs index d79cdc46..fe592180 100755 --- a/martin/src/srv/tiles.rs +++ b/martin/src/srv/tiles.rs @@ -1,5 +1,6 @@ +use actix_http::header::Quality; use actix_http::ContentEncoding; -use actix_web::error::{ErrorBadRequest, ErrorNotFound}; +use actix_web::error::{ErrorBadRequest, ErrorNotAcceptable, ErrorNotFound}; use actix_web::http::header::{ AcceptEncoding, Encoding as HeaderEnc, Preference, CONTENT_ENCODING, }; @@ -21,13 +22,7 @@ use crate::utils::{ }; use crate::{Tile, TileCoord, TileData}; -static PREFER_BROTLI_ENC: &[HeaderEnc] = &[ - HeaderEnc::brotli(), - HeaderEnc::gzip(), - HeaderEnc::identity(), -]; - -static PREFER_GZIP_ENC: &[HeaderEnc] = &[ +static SUPPORTED_ENC: &[HeaderEnc] = &[ HeaderEnc::gzip(), HeaderEnc::brotli(), HeaderEnc::identity(), @@ -180,6 +175,49 @@ impl<'a> DynTileSource<'a> { self.recompress(data) } + /// Decide which encoding to use for the uncompressed tile data, based on the client's Accept-Encoding header + fn decide_encoding(&self, accept_enc: &AcceptEncoding) -> ActixResult> { + let mut q_gzip = None; + let mut q_brotli = None; + for enc in accept_enc.iter() { + if let Preference::Specific(HeaderEnc::Known(e)) = enc.item { + match e { + ContentEncoding::Gzip => q_gzip = Some(enc.quality), + ContentEncoding::Brotli => q_brotli = Some(enc.quality), + _ => {} + } + } else if let Preference::Any = enc.item { + q_gzip.get_or_insert(enc.quality); + q_brotli.get_or_insert(enc.quality); + } + } + Ok(match (q_gzip, q_brotli) { + (Some(q_gzip), Some(q_brotli)) if q_gzip == q_brotli => { + if q_gzip > Quality::ZERO { + Some(self.get_preferred_enc()) + } else { + None + } + } + (Some(q_gzip), Some(q_brotli)) if q_brotli > q_gzip => Some(ContentEncoding::Brotli), + (Some(_), Some(_)) => Some(ContentEncoding::Gzip), + _ => { + if let Some(HeaderEnc::Known(enc)) = accept_enc.negotiate(SUPPORTED_ENC.iter()) { + Some(enc) + } else { + return Err(ErrorNotAcceptable("No supported encoding found")); + } + } + }) + } + + fn get_preferred_enc(&self) -> ContentEncoding { + match self.preferred_enc { + None | Some(PreferredEncoding::Gzip) => ContentEncoding::Gzip, + Some(PreferredEncoding::Brotli) => ContentEncoding::Brotli, + } + } + fn recompress(&self, tile: TileData) -> ActixResult { let mut tile = Tile::new(tile, self.info); if let Some(accept_enc) = &self.accept_enc { @@ -198,18 +236,12 @@ impl<'a> DynTileSource<'a> { } if tile.info.encoding == Encoding::Uncompressed { - let ordered_encodings = match self.preferred_enc { - Some(PreferredEncoding::Gzip) | None => PREFER_GZIP_ENC, - Some(PreferredEncoding::Brotli) => PREFER_BROTLI_ENC, - }; - - // only apply compression if the content supports it - if let Some(HeaderEnc::Known(enc)) = accept_enc.negotiate(ordered_encodings.iter()) - { + if let Some(enc) = self.decide_encoding(accept_enc)? { // (re-)compress the tile into the preferred encoding tile = encode(tile, enc)?; } } + Ok(tile) } else { // no accepted-encoding header, decode the tile if compressed @@ -270,6 +302,11 @@ mod tests { use super::*; use crate::srv::server::tests::TestSource; + #[actix_rt::test] + async fn test_deleteme() { + test_enc_preference(&["gzip", "deflate", "br", "zstd"], None, Encoding::Gzip).await; + } + #[rstest] #[trace] #[case(&["gzip", "deflate", "br", "zstd"], None, Encoding::Gzip)] diff --git a/martin/tests/mb_server_test.rs b/martin/tests/mb_server_test.rs index 9af72bc9..95bb7074 100644 --- a/martin/tests/mb_server_test.rs +++ b/martin/tests/mb_server_test.rs @@ -3,8 +3,8 @@ use actix_web::test::{call_service, read_body, read_body_json, TestRequest}; use ctor::ctor; use indoc::indoc; use insta::assert_yaml_snapshot; -use martin::decode_gzip; use martin::srv::SrvConfig; +use martin::{decode_brotli, decode_gzip}; use tilejson::TileJSON; pub mod utils; @@ -211,7 +211,7 @@ async fn mbt_get_mvt_brotli() { assert_eq!(response.headers().get(CONTENT_ENCODING).unwrap(), "br"); let body = read_body(response).await; assert_eq!(body.len(), 871); // this number could change if compression gets more optimized - let body = martin::decode_brotli(&body).unwrap(); + let body = decode_brotli(&body).unwrap(); assert_eq!(body.len(), 1828); } @@ -267,10 +267,10 @@ async fn mbt_get_raw_mvt_gzip_br() { response.headers().get(CONTENT_TYPE).unwrap(), "application/x-protobuf" ); - assert_eq!(response.headers().get(CONTENT_ENCODING).unwrap(), "br"); + assert_eq!(response.headers().get(CONTENT_ENCODING).unwrap(), "gzip"); let body = read_body(response).await; - assert_eq!(body.len(), 871); // this number could change if compression gets more optimized - let body = martin::decode_brotli(&body).unwrap(); + assert_eq!(body.len(), 1107); // this number could change if compression gets more optimized + let body = decode_gzip(&body).unwrap(); assert_eq!(body.len(), 1828); }