-
Notifications
You must be signed in to change notification settings - Fork 252
Add expect_number_as_string_or_null in aws-smithy-json to preserve arbitrary precision for BigInteger/BigDecimal #4444
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
Changes from 3 commits
9e0197f
adac99e
b7868ca
6bde3a3
c04c89d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --- | ||
| applies_to: | ||
| - server | ||
| - client | ||
| - aws-sdk-rust | ||
| authors: | ||
| - AmitKulkarni23 | ||
| references: | ||
| - smithy-rs#4418 | ||
| breaking: false | ||
| new_feature: true | ||
| bug_fix: false | ||
| --- | ||
| Add `expect_number_as_string_or_null` function to `aws-smithy-json` that extracts JSON numbers as strings without converting to u64/i64/f64. This preserves arbitrary precision for BigInteger and BigDecimal support, preventing precision loss for numbers larger than standard numeric types can represent. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -331,18 +331,39 @@ impl<'a> JsonTokenIterator<'a> { | |
| ) | ||
| } else if negative { | ||
| // If the negative value overflows, then stuff it into an f64 | ||
| let positive = u64::from_str(&number_str[1..]) | ||
| .map_err(|_| self.error_at(start, InvalidNumber))?; | ||
| let negative = positive.wrapping_neg() as i64; | ||
| if negative > 0 { | ||
| Number::Float(-(positive as f64)) | ||
| } else { | ||
| Number::NegInt(negative) | ||
| match u64::from_str(&number_str[1..]) { | ||
| Ok(positive) => { | ||
| // Check if the positive value fits in i64's negative range | ||
| if positive <= i64::MAX as u64 { | ||
| Number::NegInt(-(positive as i64)) | ||
| } else if positive == (i64::MAX as u64) + 1 { | ||
| // Special case: i64::MIN | ||
| Number::NegInt(i64::MIN) | ||
| } else { | ||
| // Too large for i64, use f64 | ||
| Number::Float(-(positive as f64)) | ||
| } | ||
| } | ||
| Err(_) => { | ||
| // Number too large for u64, parse as f64 (may be infinity) | ||
| Number::Float( | ||
| f64::from_str(number_str) | ||
| .map_err(|_| self.error_at(start, InvalidNumber))?, | ||
| ) | ||
| } | ||
|
Comment on lines
+334
to
+353
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows overflow to f64, enabling expect_number_as_string_or_null() to extract the original string for BigInteger/BigDecimal. Not a breaking change: more permissive behavior, public API unchanged, existing code paths unaffected |
||
| } | ||
| } else { | ||
| Number::PosInt( | ||
| u64::from_str(number_str).map_err(|_| self.error_at(start, InvalidNumber))?, | ||
| ) | ||
| // Try to parse as u64, fall back to f64 if too large | ||
| match u64::from_str(number_str) { | ||
| Ok(n) => Number::PosInt(n), | ||
| Err(_) => { | ||
| // Number too large for u64, parse as f64 (may be infinity) | ||
| Number::Float( | ||
| f64::from_str(number_str) | ||
| .map_err(|_| self.error_at(start, InvalidNumber))?, | ||
| ) | ||
| } | ||
| } | ||
| }, | ||
| }) | ||
| } | ||
|
|
@@ -820,4 +841,212 @@ mod tests { | |
| assert_eq!("foo\\nbar", escaped.as_escaped_str()); | ||
| assert_eq!("foo\nbar", escaped.to_unescaped().unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_integer_overflow_to_float() { | ||
| // Positive integer larger than u64::MAX should parse as Float | ||
| let input = b"18450000000000000000"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::Float(f), | ||
| .. | ||
| })) => { | ||
| assert!(f.is_finite()); | ||
| assert!(f > 0.0); | ||
| } | ||
| other => panic!("Expected Float token, got {:?}", other), | ||
| } | ||
|
|
||
| // Negative integer smaller than i64::MIN should parse as Float | ||
| let input = b"-9223372036854775809"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::Float(f), | ||
| .. | ||
| })) => { | ||
| assert!(f.is_finite()); | ||
| assert!(f < 0.0); | ||
| } | ||
| other => panic!("Expected Float token, got {:?}", other), | ||
| } | ||
|
|
||
| // Extremely large number should parse as infinity | ||
| let large_num = b"100000000000000000000000000000000000000000000000000000000000000\ | ||
| 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\ | ||
| 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\ | ||
| 00000000000000000000000000000000000000000000000000000000000000000000000"; | ||
| let mut iter = json_token_iter(large_num); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::Float(f), | ||
| .. | ||
| })) => { | ||
| assert_eq!(f, f64::INFINITY); | ||
| } | ||
| other => panic!("Expected Float(infinity) token, got {:?}", other), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_integer_within_range() { | ||
| // Numbers that fit in u64/i64 should still parse as PosInt/NegInt | ||
| let input = b"9007199254740993"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::PosInt(n), | ||
| .. | ||
| })) => { | ||
| assert_eq!(n, 9007199254740993); | ||
| } | ||
| other => panic!("Expected PosInt token, got {:?}", other), | ||
| } | ||
|
|
||
| let input = b"-9223372036854775808"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::NegInt(n), | ||
| .. | ||
| })) => { | ||
| assert_eq!(n, i64::MIN); | ||
| } | ||
| other => panic!("Expected NegInt token, got {:?}", other), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_integer_boundaries() { | ||
| // Zero | ||
| let input = b"0"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::PosInt(0), | ||
| .. | ||
| })) => {} | ||
| other => panic!("Expected PosInt(0), got {:?}", other), | ||
| } | ||
|
|
||
| // Regular negative number | ||
| let input = b"-123"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::NegInt(-123), | ||
| .. | ||
| })) => {} | ||
| other => panic!("Expected NegInt(-123), got {:?}", other), | ||
| } | ||
|
|
||
| // i64::MAX (largest positive i64) | ||
| let input = b"9223372036854775807"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::PosInt(n), | ||
| .. | ||
| })) => { | ||
| assert_eq!(n, i64::MAX as u64); | ||
| } | ||
| other => panic!("Expected PosInt(i64::MAX), got {:?}", other), | ||
| } | ||
|
|
||
| // i64::MIN + 1 (edge case for negative range check) | ||
| let input = b"-9223372036854775807"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::NegInt(n), | ||
| .. | ||
| })) => { | ||
| assert_eq!(n, i64::MIN + 1); | ||
| } | ||
| other => panic!("Expected NegInt(i64::MIN + 1), got {:?}", other), | ||
| } | ||
|
|
||
| // u64::MAX (fits in u64, should be PosInt) | ||
| let input = b"18446744073709551615"; | ||
| let mut iter = json_token_iter(input); | ||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { | ||
| value: Number::PosInt(n), | ||
| .. | ||
| })) => { | ||
| assert_eq!(n, u64::MAX); | ||
| } | ||
| other => panic!("Expected PosInt(u64::MAX), got {:?}", other), | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod proptest_tests { | ||
| use super::*; | ||
|
|
||
| proptest! { | ||
| #[test] | ||
| fn positive_integers_within_u64_parse_as_posint(n in 0u64..=u64::MAX) { | ||
| let input = n.to_string(); | ||
| let input_bytes = input.as_bytes(); | ||
| let mut iter = json_token_iter(input_bytes); | ||
|
|
||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { value: Number::PosInt(parsed), .. })) => { | ||
| prop_assert_eq!(parsed, n); | ||
| } | ||
| other => { | ||
| return Err(proptest::test_runner::TestCaseError::fail( | ||
| format!("Expected PosInt({}), got {:?}", n, other) | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn negative_integers_within_i64_parse_as_negint(n in i64::MIN..=i64::MAX) { | ||
| if n >= 0 { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let input = n.to_string(); | ||
| let input_bytes = input.as_bytes(); | ||
| let mut iter = json_token_iter(input_bytes); | ||
|
|
||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { value: Number::NegInt(parsed), .. })) => { | ||
| prop_assert_eq!(parsed, n); | ||
| } | ||
| other => { | ||
| return Err(proptest::test_runner::TestCaseError::fail( | ||
| format!("Expected NegInt({}), got {:?}", n, other) | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn large_integers_overflow_to_float( | ||
| // u64::MAX = 18_446_744_073_709_551_615 (20 digits) | ||
| // Generate numbers with 21+ digits to guarantee overflow | ||
| num_str in "1[0-9]{20,49}" | ||
| ) { | ||
| let input_bytes = num_str.as_bytes(); | ||
| let mut iter = json_token_iter(input_bytes); | ||
|
|
||
| match iter.next() { | ||
| Some(Ok(Token::ValueNumber { value: Number::Float(f), .. })) => { | ||
| prop_assert!(f.is_finite()); | ||
| prop_assert!(f > 0.0); | ||
| } | ||
| other => { | ||
| return Err(proptest::test_runner::TestCaseError::fail( | ||
| format!("Expected Float for large number, got {:?}", other) | ||
| )); | ||
| } | ||
| } | ||
|
Comment on lines
+1031
to
+1049
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test should also validate that the large int is actually correctly extracted by your |
||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for reviewers for my understanding:
aws-smithy-jsonautomatically trigger version bumps in crates that depend on it (like generated SDK crates), or is that handled separately?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a manual action is required by the SDK team but its basically automated. you can use this code in this repository (e.g. we can work on / merge your other PR)
we don't have to wait for it to be merged