-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Opt-in, feature-gated from_str_exact(...)
for the serde::Deserialize
impls exported by the rust_decimal::serde
module?
#569
Comments
Proof of concept differentialdiff --git i/Cargo.toml w/Cargo.toml
index a5133b4..1caa21f 100644
--- i/Cargo.toml
+++ w/Cargo.toml
@@ -73,6 +73,7 @@ rust-fuzz = ["arbitrary"]
serde-arbitrary-precision = ["serde-with-arbitrary-precision"]
serde-bincode = ["serde-str"] # Backwards compatability
serde-float = ["serde-with-float"]
+serde-from-str-exact = ["serde"]
serde-str = ["serde-with-str"]
serde-with-arbitrary-precision = ["serde", "serde_json/arbitrary_precision", "serde_json/std"]
serde-with-float = ["serde"]
diff --git i/Makefile.toml w/Makefile.toml
index 3e6c89d..9fa5d91 100644
--- i/Makefile.toml
+++ w/Makefile.toml
@@ -137,6 +137,7 @@ dependencies = [
"test-serde-with-arbitrary-precision",
"test-serde-with-float",
"test-serde-with-str",
+ "test-serde-from-str-exact",
]
[tasks.test-macros]
@@ -266,6 +267,10 @@ args = ["test", "--workspace", "--tests", "--features=serde-with-float", "serde"
command = "cargo"
args = ["test", "--workspace", "--tests", "--features=serde-with-str", "serde", "--", "--skip", "generated"]
+[tasks.test-serde-from-str-exact]
+command = "cargo"
+args = ["test", "--workspace", "--tests", "--features=serde-from-str-exact", "serde", "--", "--skip", "generated"]
+
[tasks.test-borsh]
command = "cargo"
args = ["test", "--workspace", "--features=borsh", "--", "--skip", "generated"]
diff --git i/src/serde.rs w/src/serde.rs
index e768a4a..a209564 100644
--- i/src/serde.rs
+++ w/src/serde.rs
@@ -339,6 +339,7 @@ impl<'de> serde::de::Visitor<'de> for DecimalVisitor {
}
}
+ #[cfg(not(feature = "serde-from-str-exact"))]
fn visit_str<E>(self, value: &str) -> Result<Decimal, E>
where
E: serde::de::Error,
@@ -355,6 +365,16 @@ impl<'de> serde::de::Visitor<'de> for DecimalVisitor {
.map_err(|_| E::invalid_value(Unexpected::Str(value), &self))
}
+ #[cfg(feature = "serde-from-str-exact")]
+ fn visit_str<E>(self, value: &str) -> Result<Decimal, E>
+ where
+ E: serde::de::Error,
+ {
+ Decimal::from_str_exact(value)
+ .or_else(|_| Decimal::from_scientific(value))
+ .map_err(|_| E::invalid_value(Unexpected::Str(value), &self))
+ }
+
#[cfg(feature = "serde-with-arbitrary-precision")]
fn visit_map<A>(self, map: A) -> Result<Decimal, A::Error>
where
@@ -488,6 +508,7 @@ impl<'de> serde::de::Deserialize<'de> for DecimalFromString {
formatter.write_str("string containing a decimal")
}
+ #[cfg(not(feature = "serde-from-str-exact"))]
fn visit_str<E>(self, value: &str) -> Result<DecimalFromString, E>
where
E: serde::de::Error,
@@ -497,6 +518,17 @@ impl<'de> serde::de::Deserialize<'de> for DecimalFromString {
.map_err(serde::de::Error::custom)?;
Ok(DecimalFromString { value: d })
}
+
+ #[cfg(feature = "serde-from-str-exact")]
+ fn visit_str<E>(self, value: &str) -> Result<DecimalFromString, E>
+ where
+ E: serde::de::Error,
+ {
+ let d = Decimal::from_str_exact(value)
+ .or_else(|_| Decimal::from_scientific(value))
+ .map_err(serde::de::Error::custom)?;
+ Ok(DecimalFromString { value: d })
+ }
}
deserializer.deserialize_str(Visitor)
@@ -899,4 +931,35 @@ mod test {
assert_eq!(deserialized.value, original.value);
assert!(deserialized.value.is_none());
}
+
+ #[test]
+ #[cfg(not(feature = "serde-from-str-exact"))]
+ fn fractional_underflow_is_ok() {
+ #[derive(Serialize, Deserialize)]
+ pub struct StringExample {
+ value: Decimal,
+ }
+
+ let deserialized: StringExample =
+ serde_json::from_str(r#"{"value":"123.4567890123456789012345678901234567890"}"#).unwrap();
+
+ assert_eq!(
+ deserialized.value,
+ Decimal::from_str("123.45678901234567890123456789").unwrap()
+ );
+ }
+
+ #[test]
+ #[cfg(feature = "serde-from-str-exact")]
+ fn fractional_underflow_is_err() {
+ #[derive(Serialize, Deserialize)]
+ pub struct StringExample {
+ value: Decimal,
+ }
+
+ let deserialized: Result<StringExample, serde_json::Error> =
+ serde_json::from_str(r#"{"value":"123.4567890123456789012345678901234567890"}"#);
+
+ assert!(deserialized.is_err());
+ }
} |
from_str_exact(...)
for the Deserialize
impls exported by the rust_decimal::serde
module?from_str_exact(...)
for the serde::Deserialize
impls exported by the rust_decimal::serde
module?
It's certainly a change that can be made, I guess at the expense of the usage becoming more brittle. Just thinking out loud, if you're wanting to use The only reason I'm thinking like this is because the All that said, if you put together a pull request I'd be more than happy to take a look to see how it fits together. Whilst the diff that you've provided looks good, it won't be until you run all the |
One possible idea: have a "strict" mode feature on Decimal. This could force all behavior to be explicit - including how and where to round during division (i.e. so there is no underflow rounding). |
Current behavior
Currently, a library user relying on the
serde
deserialization exported by therust_decimal::serde
module , who'd like to avoid the potential silent loss of fractional precision due toDecimal::from_str
accepting values causing fractional underflows, has no trivial option on the calling side.A pass-through newtype wrapping a
Decimal
member with a customserde::de::Visitor
implementation is a potential solution, I suppose.Desired behavior
It would be nice, if, by means of an additional feature-gate, it was possible to opt into alternative version(s) of the visitor(s) calling
Decimal::from_str_exact
instead.Proposed change
I might be mistaken, but it reads as if annotating the existing two parent functions (
DecimalVisitor::visit_str
andDecimalFromString::Visitor::visit_str
) with#[cfg(not(feature = "serde-from-str-exact"))]
(e.g.) and adding two analogous versions callingfrom_str_exact(...)
would already suffice to make this possible.I'd be happy to submit a PR, if interest in such a feature existed.
Thank you for your consideration - and the library as a whole.
The text was updated successfully, but these errors were encountered: