From 483a690f35b66347b6806d76287e93ba55071f43 Mon Sep 17 00:00:00 2001 From: Nikolay Marchuk Date: Wed, 17 Sep 2025 16:38:21 +0700 Subject: [PATCH 1/4] Refactor http handlers, extract supporting logic, remove magic constants --- src/handlers_http.rs | 206 ++++++++++++++++++++++++------------------- 1 file changed, 117 insertions(+), 89 deletions(-) diff --git a/src/handlers_http.rs b/src/handlers_http.rs index 676fe423f9..c8fdbe235b 100644 --- a/src/handlers_http.rs +++ b/src/handlers_http.rs @@ -13,13 +13,15 @@ // limitations under the License. // -use anyhow::anyhow; use serde::Deserialize; use tracing::*; use actix_web::{ - Error, HttpRequest, HttpResponse, - web::{self}, + Error, HttpResponse, + http::header::{ + self, HeaderName, HeaderValue, IfMatch, IfNoneMatch, TryIntoHeaderValue, from_one_raw_str, + }, + web, }; use crate::{ @@ -51,6 +53,9 @@ pub struct PathParams { workspace: String, } +pub struct TtlSecsHeader(usize); +pub struct TtlExpiresAtHeader(u64); + /// list pub async fn list( path: web::Path, @@ -60,12 +65,8 @@ pub async fn list( let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "list request"); - async move || -> anyhow::Result { - let entries = db.list(&key).await?; - Ok(HttpResponse::Ok().json(entries)) - }() - .await - .map_err(map_handler_error) + let entries = db.list(&key).await.map_err(map_handler_error)?; + Ok(HttpResponse::Ok().json(entries)) } /// get @@ -77,114 +78,141 @@ pub async fn get( let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "get request"); - async move || -> anyhow::Result { - let entry_opt = db.read(&key).await?; - let resp = match entry_opt { - Some(entry) => HttpResponse::Ok() - .insert_header(("ETag", entry.etag.clone())) - .json(entry), - None => HttpResponse::NotFound().body("empty"), - }; - Ok(resp) - }() - .await - .map_err(map_handler_error) + let entry_opt = db.read(&key).await.map_err(map_handler_error)?; + let resp = match entry_opt { + Some(entry) => HttpResponse::Ok() + .insert_header((header::ETAG, entry.etag.clone())) + .json(entry), + None => HttpResponse::NotFound().body("empty"), + }; + Ok(resp) } /// put pub async fn put( - req: HttpRequest, path: web::Path, body: web::Bytes, db: web::Data, + (secs, expires_at): ( + Option>, + Option>, + ), + (if_match, if_none_match): ( + Option>, + Option>, + ), ) -> Result { let params = path.into_inner(); let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "put request"); - async move || -> anyhow::Result { - // TTL logic - let mut ttl = None; - if let Some(x) = req.headers().get("HULY-TTL") { - let s = x.to_str().map_err(|_| anyhow!("Invalid HULY-TTL header"))?; - let secs = s - .parse::() - .map_err(|_| anyhow!("Invalid TTL value in HULY-TTL header"))?; - ttl = Some(Ttl::Sec(secs)); - } else if let Some(x) = req.headers().get("HULY-EXPIRE-AT") { - let s = x - .to_str() - .map_err(|_| anyhow!("Invalid HULY-EXPIRE-AT header"))?; - let ts = s - .parse::() - .map_err(|_| anyhow!("Invalid EXPIRE-AT value in HULY-EXPIRE-AT header"))?; - ttl = Some(Ttl::At(ts)); + // TTL logic + let ttl = match ( + secs.map(web::Header::into_inner), + expires_at.map(web::Header::into_inner), + ) { + (None, None) => None, + (Some(TtlSecsHeader(secs)), None) => Some(Ttl::Sec(secs)), + (None, Some(TtlExpiresAtHeader(timestamp))) => Some(Ttl::At(timestamp)), + _ => { + return Err(actix_web::error::ErrorBadRequest("Multiple ttl specified")); } + }; - // MODE logic - let mut mode = Some(SaveMode::Upsert); - if let Some(h) = req.headers().get("If-Match") { - // `If-Match: *` - update only if the key exists - let s = h.to_str().map_err(|_| anyhow!("Invalid If-Match header"))?; - if s == "*" { - mode = Some(SaveMode::Update); - } - // `If-Match: *` — update only if exist - else { - mode = Some(SaveMode::Equal(s.to_string())); - } // `If-Match: ` — update only if current - } else if let Some(h) = req.headers().get("If-None-Match") { - // `If-None-Match: *` — insert only if does not exist - let s = h - .to_str() - .map_err(|_| anyhow!("Invalid If-None-Match header"))?; - if s == "*" { - mode = Some(SaveMode::Insert); - } else { - return Err(anyhow!("If-None-Match must be '*'")); - } + // MODE logic + let mode = match ( + if_match.map(web::Header::into_inner), + if_none_match.map(web::Header::into_inner), + ) { + (None, None) => SaveMode::Upsert, + (Some(IfMatch::Any), None) => SaveMode::Update, + (Some(IfMatch::Items(etags)), None) if etags.len() == 1 => { + SaveMode::Equal(etags[0].tag().to_string()) } + (None, Some(IfNoneMatch::Any)) => SaveMode::Insert, + _ => { + return Err(actix_web::error::ErrorBadRequest( + "Unsupported combination of If-Match and If-None-Match", + )); + } + }; - db.save(&key, &body[..], ttl, mode).await?; - Ok(HttpResponse::Ok().body("DONE")) - }() - .await - .map_err(map_handler_error) + db.save(&key, &body[..], ttl, Some(mode)) + .await + .map_err(map_handler_error)?; + Ok(HttpResponse::Ok().body("DONE")) } /// delete pub async fn delete( - req: HttpRequest, path: web::Path, db: web::Data, + if_match: Option>, ) -> Result { let params = path.into_inner(); let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "delete request"); - async move || -> anyhow::Result { - // MODE logic - let mut mode = Some(SaveMode::Upsert); - if let Some(h) = req.headers().get("If-Match") { - // `If-Match: *` - delete only if the key exists - let s = h.to_str().map_err(|_| anyhow!("Invalid If-Match header"))?; - if s == "*" { - mode = Some(SaveMode::Update); - } - // `If-Match: *` — return error if not exist - else { - mode = Some(SaveMode::Equal(s.to_string())); - } // `If-Match: ` — delete only if current + // MODE logic + let mode = match if_match.map(web::Header::into_inner) { + Some(IfMatch::Any) => SaveMode::Update, + Some(IfMatch::Items(etags)) if etags.len() == 1 => { + SaveMode::Equal(etags[0].tag().to_string()) } + None => SaveMode::Upsert, + _ => { + return Err(actix_web::error::ErrorBadRequest( + "Multiple If-Match are not supported", + )); + } + }; - let deleted = db.delete(&key, mode).await?; - let response = match deleted { - true => HttpResponse::NoContent().finish(), - false => HttpResponse::NotFound().body("not found"), - }; + let deleted = db + .delete(&key, Some(mode)) + .await + .map_err(map_handler_error)?; + let response = match deleted { + true => HttpResponse::NoContent().finish(), + false => HttpResponse::NotFound().body("not found"), + }; - Ok(response) - }() - .await - .map_err(map_handler_error) + Ok(response) +} + +impl TryIntoHeaderValue for TtlSecsHeader { + type Error = std::convert::Infallible; + + fn try_into_value(self) -> Result { + Ok(HeaderValue::from(self.0)) + } +} + +impl actix_web::http::header::Header for TtlSecsHeader { + fn name() -> HeaderName { + HeaderName::from_static("huly-ttl") + } + + fn parse(msg: &M) -> Result { + let val = from_one_raw_str(msg.headers().get(Self::name()))?; + Ok(Self(val)) + } +} + +impl TryIntoHeaderValue for TtlExpiresAtHeader { + type Error = std::convert::Infallible; + + fn try_into_value(self) -> Result { + Ok(HeaderValue::from(self.0)) + } +} + +impl actix_web::http::header::Header for TtlExpiresAtHeader { + fn name() -> HeaderName { + HeaderName::from_static("huly-expire-at") + } + + fn parse(msg: &M) -> Result { + let val = from_one_raw_str(msg.headers().get(Self::name()))?; + Ok(Self(val)) + } } From 9d996401e0db3e51bd00629fba3f83d3b932eee6 Mon Sep 17 00:00:00 2001 From: Nikolay Marchuk Date: Wed, 17 Sep 2025 16:57:56 +0700 Subject: [PATCH 2/4] Fix If-Match headers logic --- src/handlers_http.rs | 47 ++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/handlers_http.rs b/src/handlers_http.rs index c8fdbe235b..0c3b75c67f 100644 --- a/src/handlers_http.rs +++ b/src/handlers_http.rs @@ -98,8 +98,8 @@ pub async fn put( Option>, ), (if_match, if_none_match): ( - Option>, - Option>, + web::Header, + web::Header, ), ) -> Result { let params = path.into_inner(); @@ -120,16 +120,19 @@ pub async fn put( }; // MODE logic - let mode = match ( - if_match.map(web::Header::into_inner), - if_none_match.map(web::Header::into_inner), - ) { - (None, None) => SaveMode::Upsert, - (Some(IfMatch::Any), None) => SaveMode::Update, - (Some(IfMatch::Items(etags)), None) if etags.len() == 1 => { + let mode = match (if_match.into_inner(), if_none_match.into_inner()) { + (IfMatch::Items(items), IfNoneMatch::Items(nitems)) + if items.is_empty() && nitems.is_empty() => + { + SaveMode::Upsert + } + (IfMatch::Any, IfNoneMatch::Items(nitems)) if nitems.is_empty() => SaveMode::Update, + (IfMatch::Items(etags), IfNoneMatch::Items(nitems)) + if etags.len() == 1 && nitems.is_empty() => + { SaveMode::Equal(etags[0].tag().to_string()) } - (None, Some(IfNoneMatch::Any)) => SaveMode::Insert, + (IfMatch::Items(items), IfNoneMatch::Any) if items.is_empty() => SaveMode::Insert, _ => { return Err(actix_web::error::ErrorBadRequest( "Unsupported combination of If-Match and If-None-Match", @@ -147,23 +150,25 @@ pub async fn put( pub async fn delete( path: web::Path, db: web::Data, - if_match: Option>, + if_match: web::Header, ) -> Result { let params = path.into_inner(); let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "delete request"); // MODE logic - let mode = match if_match.map(web::Header::into_inner) { - Some(IfMatch::Any) => SaveMode::Update, - Some(IfMatch::Items(etags)) if etags.len() == 1 => { - SaveMode::Equal(etags[0].tag().to_string()) - } - None => SaveMode::Upsert, - _ => { - return Err(actix_web::error::ErrorBadRequest( - "Multiple If-Match are not supported", - )); + let mode = match if_match.into_inner() { + IfMatch::Any => SaveMode::Update, + IfMatch::Items(etags) => { + if etags.len() == 1 { + SaveMode::Equal(etags[0].tag().to_string()) + } else if etags.is_empty() { + SaveMode::Upsert + } else { + return Err(actix_web::error::ErrorBadRequest( + "Multiple If-Match are not supported", + )); + } } }; From c806b196ed834125380e6a36d8ac3af58dddcc3b Mon Sep 17 00:00:00 2001 From: Nikolay Marchuk Date: Wed, 17 Sep 2025 17:36:02 +0700 Subject: [PATCH 3/4] Fix error handling of parsing custom headers --- src/handlers_http.rs | 67 ++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/src/handlers_http.rs b/src/handlers_http.rs index 0c3b75c67f..1c75428028 100644 --- a/src/handlers_http.rs +++ b/src/handlers_http.rs @@ -14,13 +14,13 @@ // use serde::Deserialize; +use std::str::FromStr; use tracing::*; use actix_web::{ Error, HttpResponse, - http::header::{ - self, HeaderName, HeaderValue, IfMatch, IfNoneMatch, TryIntoHeaderValue, from_one_raw_str, - }, + error::ParseError, + http::header::{self, HeaderName, HeaderValue, IfMatch, IfNoneMatch, TryIntoHeaderValue}, web, }; @@ -53,8 +53,8 @@ pub struct PathParams { workspace: String, } -pub struct TtlSecsHeader(usize); -pub struct TtlExpiresAtHeader(u64); +pub struct TtlSecsHeader(Option); +pub struct TtlExpiresAtHeader(Option); /// list pub async fn list( @@ -94,8 +94,8 @@ pub async fn put( body: web::Bytes, db: web::Data, (secs, expires_at): ( - Option>, - Option>, + Result, ParseError>, + Result, ParseError>, ), (if_match, if_none_match): ( web::Header, @@ -107,13 +107,10 @@ pub async fn put( trace!(key, "put request"); // TTL logic - let ttl = match ( - secs.map(web::Header::into_inner), - expires_at.map(web::Header::into_inner), - ) { + let ttl = match (secs?.into_inner().0, expires_at?.into_inner().0) { (None, None) => None, - (Some(TtlSecsHeader(secs)), None) => Some(Ttl::Sec(secs)), - (None, Some(TtlExpiresAtHeader(timestamp))) => Some(Ttl::At(timestamp)), + (Some(secs), None) => Some(Ttl::Sec(secs)), + (None, Some(timestamp)) => Some(Ttl::At(timestamp)), _ => { return Err(actix_web::error::ErrorBadRequest("Multiple ttl specified")); } @@ -188,17 +185,31 @@ impl TryIntoHeaderValue for TtlSecsHeader { type Error = std::convert::Infallible; fn try_into_value(self) -> Result { - Ok(HeaderValue::from(self.0)) + Ok(self + .0 + .map(HeaderValue::from) + .unwrap_or(HeaderValue::from_static(""))) } } -impl actix_web::http::header::Header for TtlSecsHeader { +impl header::Header for TtlSecsHeader { fn name() -> HeaderName { HeaderName::from_static("huly-ttl") } - fn parse(msg: &M) -> Result { - let val = from_one_raw_str(msg.headers().get(Self::name()))?; + fn parse(msg: &M) -> Result { + let mut values = msg.headers().get_all(Self::name()); + let val = if let Some(value) = values.next() { + Some( + usize::from_str(value.to_str().map_err(|_| ParseError::Header)?.trim()) + .map_err(|_| ParseError::Header)?, + ) + } else { + None + }; + if values.next().is_some() { + return Err(ParseError::TooLarge); + } Ok(Self(val)) } } @@ -207,17 +218,31 @@ impl TryIntoHeaderValue for TtlExpiresAtHeader { type Error = std::convert::Infallible; fn try_into_value(self) -> Result { - Ok(HeaderValue::from(self.0)) + Ok(self + .0 + .map(HeaderValue::from) + .unwrap_or(HeaderValue::from_static(""))) } } -impl actix_web::http::header::Header for TtlExpiresAtHeader { +impl header::Header for TtlExpiresAtHeader { fn name() -> HeaderName { HeaderName::from_static("huly-expire-at") } - fn parse(msg: &M) -> Result { - let val = from_one_raw_str(msg.headers().get(Self::name()))?; + fn parse(msg: &M) -> Result { + let mut values = msg.headers().get_all(Self::name()); + let val = if let Some(value) = values.next() { + Some( + u64::from_str(value.to_str().map_err(|_| ParseError::Header)?.trim()) + .map_err(|_| ParseError::Header)?, + ) + } else { + None + }; + if values.next().is_some() { + return Err(ParseError::TooLarge); + } Ok(Self(val)) } } From 00357432b74b1eb6a83cefe6cc2435e56cfce8ab Mon Sep 17 00:00:00 2001 From: Nikolay Marchuk Date: Thu, 18 Sep 2025 10:20:30 +0700 Subject: [PATCH 4/4] Rename error mapper in http handlers --- src/handlers_http.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/handlers_http.rs b/src/handlers_http.rs index 1c75428028..b9ade997c7 100644 --- a/src/handlers_http.rs +++ b/src/handlers_http.rs @@ -29,7 +29,7 @@ use crate::{ redis::{SaveMode, Ttl}, }; -pub fn map_handler_error(err: impl std::fmt::Display) -> Error { +pub fn map_redis_error(err: impl std::fmt::Display) -> Error { let msg = err.to_string(); if let Some(detail) = msg.split(" - ExtensionError: ").nth(1) { @@ -65,7 +65,7 @@ pub async fn list( let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "list request"); - let entries = db.list(&key).await.map_err(map_handler_error)?; + let entries = db.list(&key).await.map_err(map_redis_error)?; Ok(HttpResponse::Ok().json(entries)) } @@ -78,7 +78,7 @@ pub async fn get( let key = format!("{}/{}", ¶ms.workspace, ¶ms.key); trace!(key, "get request"); - let entry_opt = db.read(&key).await.map_err(map_handler_error)?; + let entry_opt = db.read(&key).await.map_err(map_redis_error)?; let resp = match entry_opt { Some(entry) => HttpResponse::Ok() .insert_header((header::ETAG, entry.etag.clone())) @@ -139,7 +139,7 @@ pub async fn put( db.save(&key, &body[..], ttl, Some(mode)) .await - .map_err(map_handler_error)?; + .map_err(map_redis_error)?; Ok(HttpResponse::Ok().body("DONE")) } @@ -169,10 +169,7 @@ pub async fn delete( } }; - let deleted = db - .delete(&key, Some(mode)) - .await - .map_err(map_handler_error)?; + let deleted = db.delete(&key, Some(mode)).await.map_err(map_redis_error)?; let response = match deleted { true => HttpResponse::NoContent().finish(), false => HttpResponse::NotFound().body("not found"),