From 7cca4ce1cf58196863344789bd2433f06fed2fe5 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 30 May 2025 08:54:43 -0400 Subject: [PATCH 1/5] impl(wkt): helper type for `i32` deserialization ProtoJSON requires accepting both strings and integers for integers. This PR only adds support for signed 32-bit integers. Future PRs will deal with unsigned integers and with 64-bit integers. The changes to the generator and the generated code will also happen in separate PRs. --- src/wkt/src/internal.rs | 3 + src/wkt/src/internal/int32.rs | 125 ++++++++++++++ src/wkt/tests/message_with_i32.rs | 272 ++++++++++++++++++++++++++++++ 3 files changed, 400 insertions(+) create mode 100644 src/wkt/src/internal/int32.rs create mode 100644 src/wkt/tests/message_with_i32.rs diff --git a/src/wkt/src/internal.rs b/src/wkt/src/internal.rs index 5deaa64264..0b6a0a1bc9 100644 --- a/src/wkt/src/internal.rs +++ b/src/wkt/src/internal.rs @@ -17,6 +17,9 @@ //! These types are intended for developers of the Google Cloud client libraries //! for Rust. They are undocumented and may change at any time. +mod int32; +pub use int32::I32; + pub struct F32; pub struct F64; diff --git a/src/wkt/src/internal/int32.rs b/src/wkt/src/internal/int32.rs new file mode 100644 index 0000000000..48242fe1ef --- /dev/null +++ b/src/wkt/src/internal/int32.rs @@ -0,0 +1,125 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Implement custom serializers for `i32`. +//! +//! In ProtoJSON 32-bit integers can be serialized as either strings or numbers. + +use serde::de::Unexpected::Other; + +pub struct I32; + +impl<'de> serde_with::DeserializeAs<'de, i32> for I32 { + fn deserialize_as(deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + deserializer.deserialize_any(I32Visitor) + } +} + +const ERRMSG: &str = "a 32-bit signed integer"; + +struct I32Visitor; + +impl serde::de::Visitor<'_> for I32Visitor { + type Value = i32; + + fn visit_str(self, value: &str) -> std::result::Result + where + E: serde::de::Error, + { + value.parse::().map_err(E::custom) + } + + fn visit_i64(self, value: i64) -> std::result::Result + where + E: serde::de::Error, + { + match value { + _ if value < i32::MIN as i64 => { + Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) + } + _ if value > i32::MAX as i64 => { + Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) + } + _ => Ok(value as i32), + } + } + + fn visit_u64(self, value: u64) -> std::result::Result + where + E: serde::de::Error, + { + match value { + _ if value > i32::MAX as u64 => { + Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) + } + _ => Ok(value as i32), + } + } + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + formatter.write_str("a 32-bit integer in ProtoJSON format") + } +} + +impl serde_with::SerializeAs for I32 { + fn serialize_as(source: &i32, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_i32(*source) + } +} + +#[cfg(test)] +mod test { + use super::*; + use anyhow::Result; + use serde_json::{Value, json}; + use serde_with::{DeserializeAs, SerializeAs}; + use test_case::test_case; + + #[test_case(0, 0)] + #[test_case("0", 0; "zero string")] + #[test_case(-42, -42)] + #[test_case("-7", -7)] + #[test_case(84, 84)] + #[test_case("21", 21)] + #[test_case(i32::MAX, i32::MAX; "max")] + #[test_case(format!("{}", i32::MAX), i32::MAX; "max as string")] + #[test_case(i32::MIN, i32::MIN; "min")] + #[test_case(format!("{}", i32::MIN), i32::MIN; "min as string")] + // Not quite a roundtrip test because we always serialize as numbers. + fn deser_and_ser(input: T, want: i32) -> Result<()> { + let got = I32::deserialize_as(json!(input))?; + assert_eq!(got, want); + + let serialized = I32::serialize_as(&got, serde_json::value::Serializer)?; + assert_eq!(serialized, json!(got)); + Ok(()) + } + + #[test_case(json!(i64::MAX))] + #[test_case(json!(i64::MIN))] + #[test_case(json!(format!("{}", i64::MAX)))] + #[test_case(json!(format!("{}", i64::MIN)))] + #[test_case(json!("abc"))] + #[test_case(json!({}))] + fn deser_error(input: Value) { + let got = I32::deserialize_as(input).unwrap_err(); + assert!(got.is_data(), "{got:?}"); + } +} diff --git a/src/wkt/tests/message_with_i32.rs b/src/wkt/tests/message_with_i32.rs new file mode 100644 index 0000000000..23a1503cb0 --- /dev/null +++ b/src/wkt/tests/message_with_i32.rs @@ -0,0 +1,272 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#[cfg(test)] +mod test { + use serde_json::{Value, json}; + use std::collections::HashMap; + use test_case::test_case; + + type Result = anyhow::Result<()>; + + #[serde_with::serde_as] + #[derive(Clone, Debug, Default, PartialEq, serde::Deserialize, serde::Serialize)] + #[serde(default, rename_all = "camelCase")] + pub struct MessageWithI32 { + #[serde(skip_serializing_if = "google_cloud_wkt::internal::is_default")] + #[serde_as(as = "google_cloud_wkt::internal::I32")] + pub singular: i32, + + #[serde(skip_serializing_if = "std::option::Option::is_none")] + #[serde_as(as = "Option")] + pub optional: Option, + + #[serde(skip_serializing_if = "Vec::is_empty")] + #[serde_as(as = "Vec")] + pub repeated: Vec, + + #[serde(skip_serializing_if = "std::collections::HashMap::is_empty")] + #[serde_as(as = "std::collections::HashMap<_, google_cloud_wkt::internal::I32>")] + pub map_value: std::collections::HashMap, + + #[serde(skip_serializing_if = "std::collections::HashMap::is_empty")] + #[serde_as(as = "std::collections::HashMap")] + pub map_key: std::collections::HashMap, + + #[serde(skip_serializing_if = "std::collections::HashMap::is_empty")] + #[serde_as( + as = "std::collections::HashMap" + )] + pub map_key_value: std::collections::HashMap, + } + + #[test_case("123", 123)] + #[test_case(456, 456)] + #[test_case("-789", -789)] + fn test_singular(input: T, want: i32) -> Result + where + T: serde::ser::Serialize, + { + let value = json!({"singular": input}); + let got = serde_json::from_value::(value)?; + let output = json!({"singular": want}); + assert_eq!( + got, + MessageWithI32 { + singular: want, + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({"singular": 0}))] + #[test_case(json!({"singular": "0"}); "string zero")] + #[test_case(json!({}))] + fn test_singular_default(input: Value) -> Result { + let want = MessageWithI32 { + singular: 0, + ..Default::default() + }; + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + let output = serde_json::to_value(&got)?; + assert_eq!(output, json!({})); + Ok(()) + } + + #[test_case("123", 123)] + #[test_case(456, 456)] + #[test_case("-789", -789)] + #[test_case(0, 0)] + #[test_case("0", 0; "string zero")] + fn test_optional(input: T, want: i32) -> Result + where + T: serde::ser::Serialize, + { + let value = json!({"optional": input}); + let got = serde_json::from_value::(value)?; + let output = json!({"optional": want}); + if want == 0 { + return Ok(()); + } + assert_eq!( + got, + MessageWithI32 { + optional: Some(want), + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({}))] + fn test_optional_none(input: Value) -> Result { + let want = MessageWithI32 { + optional: None, + ..Default::default() + }; + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + Ok(()) + } + + #[test_case("123", 123)] + #[test_case(456, 456)] + #[test_case("-789", -789)] + fn test_repeated(input: T, want: i32) -> Result + where + T: serde::ser::Serialize, + { + let value = json!({"repeated": [input]}); + let got = serde_json::from_value::(value)?; + let output = json!({"repeated": [want]}); + if want == 0 { + return Ok(()); + } + assert_eq!( + got, + MessageWithI32 { + repeated: vec![want], + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({"repeated": []}))] + #[test_case(json!({}))] + fn test_repeated_default(input: Value) -> Result { + let want = MessageWithI32 { + repeated: vec![], + ..Default::default() + }; + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + let output = serde_json::to_value(&got)?; + assert_eq!(output, json!({})); + Ok(()) + } + + #[test_case(0, 0)] + #[test_case("0", 0; "zero string")] + #[test_case("123", 123)] + #[test_case(456, 456)] + #[test_case("-789", -789)] + fn test_map_value(input: T, want: i32) -> Result + where + T: serde::ser::Serialize, + { + let value = json!({"mapValue": {"test": input}}); + let got = serde_json::from_value::(value)?; + let output = json!({"mapValue": {"test": want}}); + assert_eq!( + got, + MessageWithI32 { + map_value: HashMap::from([("test".to_string(), want)]), + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({"mapValue": {}}))] + #[test_case(json!({}))] + fn test_map_value_default(input: Value) -> Result { + let want = MessageWithI32::default(); + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + let output = serde_json::to_value(&got)?; + assert_eq!(output, json!({})); + Ok(()) + } + + #[test_case("0", 0)] + #[test_case("123", 123)] + #[test_case("-789", -789)] + fn test_map_key(input: T, want: i32) -> Result + where + T: Into, + { + let value = json!({"mapKey": {input: "test"}}); + let got = serde_json::from_value::(value)?; + let output = json!({"mapKey": {want.to_string(): "test"}}); + assert_eq!( + got, + MessageWithI32 { + map_key: HashMap::from([(want, "test".to_string())]), + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({"mapKey": {}}))] + #[test_case(json!({}))] + fn test_map_key_default(input: Value) -> Result { + let want = MessageWithI32::default(); + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + let output = serde_json::to_value(&got)?; + assert_eq!(output, json!({})); + Ok(()) + } + + #[test_case("0", "0", 0, 0; "string zero")] + #[test_case("0", 0, 0, 0)] + #[test_case("123", 234, 123, 234)] + #[test_case("123", "345", 123, 345)] + #[test_case("-789", 456, -789, 456)] + #[test_case("-789", "567", -789, 567)] + fn test_map_key_value(key: K, value: V, want_key: i32, want_value: i32) -> Result + where + K: Into, + V: serde::Serialize, + { + let value = json!({"mapKeyValue": {key: value}}); + let got = serde_json::from_value::(value)?; + let output = json!({"mapKeyValue": {want_key.to_string(): want_value}}); + assert_eq!( + got, + MessageWithI32 { + map_key_value: HashMap::from([(want_key, want_value)]), + ..Default::default() + } + ); + let trip = serde_json::to_value(&got)?; + assert_eq!(trip, output); + Ok(()) + } + + #[test_case(json!({"mapKeyValue": {}}))] + #[test_case(json!({}))] + fn test_map_key_value_default(input: Value) -> Result { + let want = MessageWithI32::default(); + let got = serde_json::from_value::(input)?; + assert_eq!(got, want); + let output = serde_json::to_value(&got)?; + assert_eq!(output, json!({})); + Ok(()) + } +} From 14fe676e63254c409ab59be1bfad68873cf693f4 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 30 May 2025 13:43:57 -0400 Subject: [PATCH 2/5] More test cases --- src/wkt/src/internal/int32.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wkt/src/internal/int32.rs b/src/wkt/src/internal/int32.rs index 48242fe1ef..d6652c638d 100644 --- a/src/wkt/src/internal/int32.rs +++ b/src/wkt/src/internal/int32.rs @@ -114,6 +114,8 @@ mod test { #[test_case(json!(i64::MAX))] #[test_case(json!(i64::MIN))] + #[test_case(json!(i32::MAX as i64 + 2))] + #[test_case(json!(i32::MIN as i64 - 2))] #[test_case(json!(format!("{}", i64::MAX)))] #[test_case(json!(format!("{}", i64::MIN)))] #[test_case(json!("abc"))] From 6aecb1c7773a9a729b320194c7a4168dd873032d Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 30 May 2025 17:59:53 -0400 Subject: [PATCH 3/5] Support floating point numbers that are integers --- src/wkt/src/internal/int32.rs | 53 ++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/src/wkt/src/internal/int32.rs b/src/wkt/src/internal/int32.rs index d6652c638d..92ff570067 100644 --- a/src/wkt/src/internal/int32.rs +++ b/src/wkt/src/internal/int32.rs @@ -40,7 +40,11 @@ impl serde::de::Visitor<'_> for I32Visitor { where E: serde::de::Error, { - value.parse::().map_err(E::custom) + // ProtoJSON says that both strings and numbers are accepted. Parse the + // string as a f64 number (all JSON numbers are f64) and then try to + // parse that as a + let number = value.parse::().map_err(E::custom)?; + self.visit_f64(number) } fn visit_i64(self, value: i64) -> std::result::Result @@ -48,12 +52,8 @@ impl serde::de::Visitor<'_> for I32Visitor { E: serde::de::Error, { match value { - _ if value < i32::MIN as i64 => { - Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) - } - _ if value > i32::MAX as i64 => { - Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) - } + _ if value < i32::MIN as i64 => Err(self::value_error(value)), + _ if value > i32::MAX as i64 => Err(self::value_error(value)), _ => Ok(value as i32), } } @@ -63,9 +63,22 @@ impl serde::de::Visitor<'_> for I32Visitor { E: serde::de::Error, { match value { - _ if value > i32::MAX as u64 => { - Err(E::invalid_value(Other(&format!("{value}")), &ERRMSG)) - } + _ if value > i32::MAX as u64 => Err(self::value_error(value)), + _ => Ok(value as i32), + } + } + + fn visit_f64(self, value: f64) -> std::result::Result + where + E: serde::de::Error, + { + match value { + _ if value < i32::MIN as f64 => Err(self::value_error(value)), + _ if value > i32::MAX as f64 => Err(self::value_error(value)), + _ if value.fract().abs() > 0.0 => Err(self::value_error(value)), + // The number is "rounded towards zero". Because we are in range, + // and the fractional part is 0, this conversion should be safe. + // See https://doc.rust-lang.org/reference/expressions/operator-expr.html#r-expr.as.numeric.float-as-int _ => Ok(value as i32), } } @@ -75,6 +88,14 @@ impl serde::de::Visitor<'_> for I32Visitor { } } +fn value_error(value: T) -> E +where + T: std::fmt::Display, + E: serde::de::Error, +{ + E::invalid_value(Other(&format!("{value}")), &ERRMSG) +} + impl serde_with::SerializeAs for I32 { fn serialize_as(source: &i32, serializer: S) -> Result where @@ -94,14 +115,24 @@ mod test { #[test_case(0, 0)] #[test_case("0", 0; "zero string")] + #[test_case("2.0", 2)] + #[test_case(3e5, 300_000)] + #[test_case(-4e4, -40_000)] + #[test_case("5e4", 50_000)] + #[test_case("-6e5", -600_000)] #[test_case(-42, -42)] #[test_case("-7", -7)] #[test_case(84, 84)] + #[test_case(168.0, 168)] #[test_case("21", 21)] #[test_case(i32::MAX, i32::MAX; "max")] + #[test_case(i32::MAX as f64, i32::MAX; "max as f64")] #[test_case(format!("{}", i32::MAX), i32::MAX; "max as string")] + #[test_case(format!("{}.0", i32::MAX), i32::MAX; "max as f64 string")] #[test_case(i32::MIN, i32::MIN; "min")] + #[test_case(i32::MIN as f64, i32::MIN; "min as f64")] #[test_case(format!("{}", i32::MIN), i32::MIN; "min as string")] + #[test_case(format!("{}.0", i32::MIN), i32::MIN; "min as f64 string")] // Not quite a roundtrip test because we always serialize as numbers. fn deser_and_ser(input: T, want: i32) -> Result<()> { let got = I32::deserialize_as(json!(input))?; @@ -119,6 +150,8 @@ mod test { #[test_case(json!(format!("{}", i64::MAX)))] #[test_case(json!(format!("{}", i64::MIN)))] #[test_case(json!("abc"))] + #[test_case(json!(123.4))] + #[test_case(json!("234.5"))] #[test_case(json!({}))] fn deser_error(input: Value) { let got = I32::deserialize_as(input).unwrap_err(); From 16cd950027d8bc835622df3c7aa45dac1f1473ec Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 30 May 2025 18:41:07 -0400 Subject: [PATCH 4/5] Address review comments. --- src/wkt/src/internal/int32.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wkt/src/internal/int32.rs b/src/wkt/src/internal/int32.rs index 92ff570067..9e6a097395 100644 --- a/src/wkt/src/internal/int32.rs +++ b/src/wkt/src/internal/int32.rs @@ -41,8 +41,8 @@ impl serde::de::Visitor<'_> for I32Visitor { E: serde::de::Error, { // ProtoJSON says that both strings and numbers are accepted. Parse the - // string as a f64 number (all JSON numbers are f64) and then try to - // parse that as a + // string as a `f64` number (all JSON numbers are `f64`) and then try to + // parse that as an `i32`. let number = value.parse::().map_err(E::custom)?; self.visit_f64(number) } From 9068aadca927ad31e05d51be5199e53071ebadb4 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 30 May 2025 20:05:19 -0400 Subject: [PATCH 5/5] Addressed review comments. --- src/wkt/tests/message_with_i32.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/wkt/tests/message_with_i32.rs b/src/wkt/tests/message_with_i32.rs index 23a1503cb0..18597d508a 100644 --- a/src/wkt/tests/message_with_i32.rs +++ b/src/wkt/tests/message_with_i32.rs @@ -100,9 +100,6 @@ mod test { let value = json!({"optional": input}); let got = serde_json::from_value::(value)?; let output = json!({"optional": want}); - if want == 0 { - return Ok(()); - } assert_eq!( got, MessageWithI32 { @@ -126,6 +123,8 @@ mod test { Ok(()) } + #[test_case(0, 0)] + #[test_case("0", 0; "zero as string")] #[test_case("123", 123)] #[test_case(456, 456)] #[test_case("-789", -789)] @@ -136,9 +135,6 @@ mod test { let value = json!({"repeated": [input]}); let got = serde_json::from_value::(value)?; let output = json!({"repeated": [want]}); - if want == 0 { - return Ok(()); - } assert_eq!( got, MessageWithI32 {