diff --git a/Cargo.lock b/Cargo.lock index d8ad69b..44260b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -280,7 +280,6 @@ dependencies = [ "serde", "serde-ndim", "serde_json", - "serde_plain", "serde_repr", "serdebug", "socket2", @@ -4053,15 +4052,6 @@ dependencies = [ "serde_core", ] -[[package]] -name = "serde_plain" -version = "1.0.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ce1fc6db65a611022b23a0dec6975d63fb80a302cb3388835ff02c097258d50" -dependencies = [ - "serde", -] - [[package]] name = "serde_repr" version = "0.1.20" diff --git a/Cargo.toml b/Cargo.toml index 9245701..919298e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,7 +66,6 @@ reqwest = { version = "0.13.1", optional = true, default-features = false, featu serde = { version = "1.0.228", features = ["derive", "rc"] } serde-ndim = { version = "2.2.0", optional = true, features = ["ndarray"] } serde_json = { version = "1.0.148" } -serde_plain = { version = "1.0.2", optional = true } serde_repr = "0.1.20" socket2 = "0.6.1" thiserror = "2.0.17" @@ -147,7 +146,6 @@ server = [ "dep:axum", "dep:http", "dep:indexmap", - "dep:serde_plain", ] [package.metadata.docs.rs] diff --git a/src/server/error.rs b/src/server/error.rs index 24282ce..9a68daf 100644 --- a/src/server/error.rs +++ b/src/server/error.rs @@ -1,3 +1,4 @@ +use super::params::AlpacaParseError; use crate::ASCOMError; use crate::api::DeviceType; use axum::http::StatusCode; @@ -22,7 +23,7 @@ pub(crate) enum Error { BadParameter { name: &'static str, #[source] - err: serde_plain::Error, + err: AlpacaParseError, }, #[error(transparent)] Ascom(#[from] ASCOMError), diff --git a/src/server/params.rs b/src/server/params.rs index 380e695..b58dc49 100644 --- a/src/server/params.rs +++ b/src/server/params.rs @@ -6,11 +6,234 @@ use axum::response::{IntoResponse, Response}; use http::{Method, StatusCode}; use indexmap::IndexMap; use serde::Deserialize; -use serde::de::{DeserializeOwned, IntoDeserializer}; -use std::any::TypeId; -use std::fmt::Debug; +use serde::de::{DeserializeOwned, Deserializer, Visitor}; +use std::fmt::{self, Debug}; use std::hash::Hash; +/// Error type for Alpaca parameter parsing that distinguishes parse errors from range errors. +#[derive(Debug, thiserror::Error)] +pub(crate) enum AlpacaParseError { + /// Invalid format (not a valid integer/bool/etc) -> HTTP 400 + #[error("{0}")] + BadFormat(String), + /// Valid integer but out of range for target type -> ASCOM `INVALID_VALUE` + #[error("value {value} is out of range for {target_type}")] + OutOfRange { value: i64, target_type: &'static str }, +} + +impl serde::de::Error for AlpacaParseError { + fn custom(msg: T) -> Self { + Self::BadFormat(msg.to_string()) + } +} + +/// Custom serde Deserializer for Alpaca parameters. +/// +/// Integers are parsed as i64 first, then converted to the target type. +/// This allows distinguishing parse errors (`BadParameter`) from range errors +/// (`INVALID_VALUE`), and supports both i32 device parameters and uint32 +/// transaction/identity parameters (`ClientID`, `ClientTransactionID`). +struct AlpacaDeserializer { + value: String, +} + +impl AlpacaDeserializer { + /// Parse an integer: parse as i64 first, then convert to the target type. + fn parse_integer>(&self) -> Result { + let i64_value: i64 = self.value.trim().parse().map_err(|_parse_err| { + AlpacaParseError::BadFormat(format!("invalid integer: {}", self.value)) + })?; + + T::try_from(i64_value).map_err(|_range_err| AlpacaParseError::OutOfRange { + value: i64_value, + target_type: std::any::type_name::(), + }) + } +} + +impl<'de> Deserializer<'de> for AlpacaDeserializer { + type Error = AlpacaParseError; + + fn deserialize_any>(self, visitor: V) -> Result { + visitor.visit_string(self.value) + } + + fn deserialize_bool>(self, visitor: V) -> Result { + match self.value.trim().to_ascii_lowercase().as_str() { + "true" => visitor.visit_bool(true), + "false" => visitor.visit_bool(false), + _ => Err(AlpacaParseError::BadFormat(format!( + "invalid boolean: {}", + self.value + ))), + } + } + + fn deserialize_i8>(self, visitor: V) -> Result { + visitor.visit_i8(self.parse_integer()?) + } + + fn deserialize_i16>(self, visitor: V) -> Result { + visitor.visit_i16(self.parse_integer()?) + } + + fn deserialize_i32>(self, visitor: V) -> Result { + visitor.visit_i32(self.parse_integer()?) + } + + fn deserialize_i64>(self, visitor: V) -> Result { + visitor.visit_i64(self.parse_integer()?) + } + + fn deserialize_u8>(self, visitor: V) -> Result { + visitor.visit_u8(self.parse_integer()?) + } + + fn deserialize_u16>(self, visitor: V) -> Result { + visitor.visit_u16(self.parse_integer()?) + } + + fn deserialize_u32>(self, visitor: V) -> Result { + visitor.visit_u32(self.parse_integer()?) + } + + fn deserialize_u64>(self, visitor: V) -> Result { + visitor.visit_u64(self.parse_integer()?) + } + + fn deserialize_f32>(self, visitor: V) -> Result { + let value: f32 = self.value.trim().parse().map_err(|_parse_err| { + AlpacaParseError::BadFormat(format!("invalid float: {}", self.value)) + })?; + visitor.visit_f32(value) + } + + fn deserialize_f64>(self, visitor: V) -> Result { + let value: f64 = self.value.trim().parse().map_err(|_parse_err| { + AlpacaParseError::BadFormat(format!("invalid float: {}", self.value)) + })?; + visitor.visit_f64(value) + } + + fn deserialize_char>(self, visitor: V) -> Result { + let mut chars = self.value.chars(); + match (chars.next(), chars.next()) { + (Some(c), None) => visitor.visit_char(c), + _ => Err(AlpacaParseError::BadFormat(format!( + "expected single character: {}", + self.value + ))), + } + } + + fn deserialize_bytes>(self, visitor: V) -> Result { + visitor.visit_bytes(self.value.as_bytes()) + } + + fn deserialize_byte_buf>(self, visitor: V) -> Result { + visitor.visit_byte_buf(self.value.into_bytes()) + } + + fn deserialize_str>(self, visitor: V) -> Result { + visitor.visit_string(self.value) + } + + fn deserialize_string>(self, visitor: V) -> Result { + visitor.visit_string(self.value) + } + + fn deserialize_newtype_struct>( + self, + _name: &'static str, + visitor: V, + ) -> Result { + visitor.visit_newtype_struct(self) + } + + fn deserialize_enum>( + self, + _name: &'static str, + _variants: &'static [&'static str], + visitor: V, + ) -> Result { + visitor.visit_enum(serde::de::value::StringDeserializer::new(self.value)) + } + + fn deserialize_identifier>(self, visitor: V) -> Result { + visitor.visit_string(self.value) + } + + fn deserialize_ignored_any>(self, visitor: V) -> Result { + visitor.visit_string(self.value) + } + + fn deserialize_option>(self, _visitor: V) -> Result { + Err(AlpacaParseError::BadFormat( + "Option types are not supported as Alpaca parameters".to_owned(), + )) + } + + fn deserialize_unit>(self, _visitor: V) -> Result { + Err(AlpacaParseError::BadFormat( + "unit type is not supported as Alpaca parameter".to_owned(), + )) + } + + fn deserialize_unit_struct>( + self, + _name: &'static str, + _visitor: V, + ) -> Result { + Err(AlpacaParseError::BadFormat( + "unit struct is not supported as Alpaca parameter".to_owned(), + )) + } + + fn deserialize_seq>(self, _visitor: V) -> Result { + Err(AlpacaParseError::BadFormat( + "sequences are not supported as Alpaca parameters".to_owned(), + )) + } + + fn deserialize_tuple>( + self, + _len: usize, + _visitor: V, + ) -> Result { + Err(AlpacaParseError::BadFormat( + "tuples are not supported as Alpaca parameters".to_owned(), + )) + } + + fn deserialize_tuple_struct>( + self, + _name: &'static str, + _len: usize, + _visitor: V, + ) -> Result { + Err(AlpacaParseError::BadFormat( + "tuple structs are not supported as Alpaca parameters".to_owned(), + )) + } + + fn deserialize_map>(self, _visitor: V) -> Result { + Err(AlpacaParseError::BadFormat( + "maps are not supported as Alpaca parameters".to_owned(), + )) + } + + fn deserialize_struct>( + self, + _name: &'static str, + _fields: &'static [&'static str], + _visitor: V, + ) -> Result { + Err(AlpacaParseError::BadFormat( + "structs are not supported as Alpaca parameters".to_owned(), + )) + } +} + #[derive(Deserialize, derive_more::Debug)] #[debug("{_0:?}")] #[serde(transparent)] @@ -27,28 +250,20 @@ impl OpaqueParams where str: AsRef, { - pub(crate) fn maybe_extract( + pub(crate) fn maybe_extract( &mut self, name: &'static str, ) -> super::Result> { self.0 .swap_remove(name.as_ref()) - .map(|mut value| { - // Specialization: optimized path to avoid cloning the string. - if TypeId::of::() == TypeId::of::() { - return T::deserialize(value.into_deserializer()); - } - // Specialization: booleans in ASCOM must be case-insensitive. - if TypeId::of::() == TypeId::of::() { - value.make_ascii_lowercase(); - } - serde_plain::from_str(&value) + .map(|value| { + T::deserialize(AlpacaDeserializer { value }) + .map_err(|err| Error::BadParameter { name, err }) }) .transpose() - .map_err(|err| Error::BadParameter { name, err }) } - pub(crate) fn extract( + pub(crate) fn extract( &mut self, name: &'static str, ) -> super::Result { @@ -93,3 +308,111 @@ impl FromRequest for ActionParams { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(value: &str) -> super::super::Result { + let deser = AlpacaDeserializer { + value: value.to_owned(), + }; + T::deserialize(deser).map_err(|err| Error::BadParameter { name: "test", err }) + } + + #[test] + fn u32_above_i32_max() { + let val: u32 = parse("3000000000").expect("should parse u32 above i32::MAX"); + assert_eq!(val, 3_000_000_000_u32); + } + + #[test] + fn u32_max_value() { + let val: u32 = parse("4294967295").expect("should parse u32::MAX"); + assert_eq!(val, u32::MAX); + } + + #[test] + fn i32_positive() { + let val: i32 = parse("42").expect("should parse positive i32"); + assert_eq!(val, 42_i32); + } + + #[test] + fn i32_negative() { + let val: i32 = parse("-1").expect("should parse negative i32"); + assert_eq!(val, -1_i32); + } + + #[test] + fn i32_max_boundary() { + let val: i32 = parse("2147483647").expect("should parse i32::MAX"); + assert_eq!(val, i32::MAX); + } + + #[test] + fn i32_out_of_range() { + let err = parse::("3000000000").expect_err("should fail for i32 out of range"); + assert!( + matches!( + err, + Error::BadParameter { + err: AlpacaParseError::OutOfRange { .. }, + .. + } + ), + "expected OutOfRange, got: {err:?}" + ); + } + + #[test] + fn u32_negative_out_of_range() { + let err = parse::("-1").expect_err("should fail for negative u32"); + assert!( + matches!( + err, + Error::BadParameter { + err: AlpacaParseError::OutOfRange { .. }, + .. + } + ), + "expected OutOfRange, got: {err:?}" + ); + } + + #[test] + fn u8_out_of_range() { + let err = parse::("256").expect_err("should fail for u8 out of range"); + assert!( + matches!( + err, + Error::BadParameter { + err: AlpacaParseError::OutOfRange { .. }, + .. + } + ), + "expected OutOfRange, got: {err:?}" + ); + } + + #[test] + fn bad_format_non_numeric() { + let err = parse::("abc").expect_err("should fail for non-numeric input"); + assert!( + matches!( + err, + Error::BadParameter { + err: AlpacaParseError::BadFormat(_), + .. + } + ), + "expected BadFormat, got: {err:?}" + ); + } + + #[test] + fn whitespace_trimming() { + let val: u32 = parse(" 42 ").expect("should parse with surrounding whitespace"); + assert_eq!(val, 42_u32); + } +} diff --git a/src/server/response.rs b/src/server/response.rs index 227848c..e4439ae 100644 --- a/src/server/response.rs +++ b/src/server/response.rs @@ -1,3 +1,4 @@ +use super::params::AlpacaParseError; use super::{Error, ResponseWithTransaction}; use crate::response::ValueResponse; use crate::{ASCOMError, ASCOMErrorCode, ASCOMResult}; @@ -52,6 +53,12 @@ where match self.response { Ok(response) => Ok(Ok(response)), Err(Error::Ascom(err)) => Ok(Err(err)), + Err(Error::BadParameter { + name, + err: AlpacaParseError::OutOfRange { value, target_type }, + }) => Ok(Err(ASCOMError::invalid_value(format!( + "Parameter {name:?} value {value} is out of range for {target_type}" + )))), Err(err @ (Error::MissingParameter { .. } | Error::BadParameter { .. })) => { Err((StatusCode::BAD_REQUEST, err.to_string())) }