Refactor converters to numeric types for `aws_smithy_types::Number` #1274

david-perez · 2022-03-23T15:44:49Z

Currently, conversions from aws_smithy_types::Number into numeric Rust
types ({i,u}{8, 16, 32, 64} and f{32, 64}) are always lossy, because
they use the as Rust keyword to cast into the target type. This means
that clients and servers are accepting lossy data: for example, if an
operation is modeled to take in a 32-bit integer as input, and a client
incorrectly sends an integer number that does not fit in 32 bits, the
server will silently accept the truncated input. There are malformed
request protocol tests that verify that servers must reject these
requests.

This commit removes the lossy to_* methods on Number and instead
implements TryFrom<Number> for $typ for the target numeric type
$typ. These converters will attempt their best to perform the
conversion safely, and fail if it is lossy.

The code-generated JSON parsers will now fail with
aws_smithy_json::deserialize::ErrorReason::InvalidNumber if the number
in the JSON document cannot be converted into the modeled integer type
without losing precision. For floating point target types, lossy
conversions are still performed, via Number::to_f32_lossy and
Number::to_f64_lossy.

Motivation and Context

Description

Testing

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Currently, conversions from `aws_smithy_types::Number` into numeric Rust types (`{i,u}{8, 16, 32, 64}` and `f{32, 64}`) are always lossy, because they use the `as` Rust keyword to cast into the target type. This means that clients and servers are accepting lossy data: for example, if an operation is modeled to take in a 32-bit integer as input, and a client incorrectly sends an integer number that does not fit in 32 bits, the server will silently accept the truncated input. There are malformed request protocol tests that verify that servers must reject these requests. This commit removes the lossy `to_*` methods on `Number` and instead implements `TryFrom<$typ> for Number` for the target numeric type `$typ`. These converters will attempt their best to perform the conversion safely, and fail if it is lossy. The code-generated JSON parsers will now fail with `aws_smithy_json::deserialize::ErrorReason::InvalidNumber` if the number in the JSON document cannot be converted into the modeled integer type without losing precision. For floating point target types, lossy conversions are still performed, via `Number::to_f32_lossy` and `Number::to_f64_lossy`.

github-actions · 2022-03-23T19:42:38Z

A new doc preview is ready to view.

github-actions · 2022-03-23T19:43:35Z

A new generated diff is ready to view.

github-actions · 2022-03-23T19:52:47Z

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	0.86%	66142.6	65578.75
Total requests	0.86%	5958657	5908013
Total errors	NaN%	0	0
Total successes	0.86%	5958657	5908013
Average latency ms	15.38%	1.2	1.04
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-6.01%	23.92	25.45
Stdev latency ms	13.87%	1.97	1.73
Transfer Mb	0.86%	619.41	614.14
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-03-23T21:45:06Z

A new generated diff is ready to view.

github-actions · 2022-03-23T21:45:31Z

A new doc preview is ready to view.

github-actions · 2022-03-23T21:54:12Z

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-0.74%	76390.22	76962.44
Total requests	-0.82%	6877380	6934018
Total errors	NaN%	0	0
Total successes	-0.82%	6877380	6934018
Average latency ms	2.54%	1.21	1.18
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-15.13%	25.86	30.47
Stdev latency ms	1.43%	2.13	2.1
Transfer Mb	-0.82%	714.91	720.79
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

rust-runtime/aws-smithy-types/src/lib.rs

crisidev

I left a couple of simple comments and suggestions. LGTM overall. Nice test suite!

rcoh

Overall LGTM. It seems like it may be possible to refactor the converters to share macros but probably not worth it.

I may consider adding a proptest of some invariants as well.

rust-runtime/aws-smithy-types/src/lib.rs

rcoh · 2022-03-23T22:21:48Z

CHANGELOG.next.toml

@@ -34,3 +34,9 @@ message = "Update all SDK and runtime crates to [edition 2021](https://blog.rust
 references = ["aws-sdk-rust#490"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "Velfi"
+
+[[smithy-rs]]
+message = "Refactor converters to numeric types for `aws_smithy_types::Number`"


can you indicate what the break is and how users should update their code?

I doubt any are performing these conversions themselves, but here's the updated changelog: c21572d

rust-runtime/aws-smithy-types/src/lib.rs

rcoh · 2022-03-23T22:31:44Z

rust-runtime/aws-smithy-json/src/deserialize/error.rs

+    fn from(_: aws_smithy_types::TryFromNumberError) -> Self {
+        Error {
+            reason: ErrorReason::InvalidNumber,
+            offset: None,


if we throw away this offset is it going to make it really hard to debug? Or do we have another mechanism to track the exact field where this was a problem?

We don't have the offset here. We only have the offset when working with the token directly in the code-generated deserializer, or when calling the expect_ functions which auto-fill it from the token in case of errors. When unescaping strings, we're also bubbling up errors without offsets. Making both cases bubble up offsets would require some refactoring in the parser generation.

rcoh · 2022-03-23T22:32:57Z

rust-runtime/aws-smithy-types/src/lib.rs

 pub enum Number {
-    /// Unsigned 64-bit integer value
+    /// Unsigned 64-bit integer value.
    PosInt(u64),
-    /// Signed 64-bit integer value
+    /// Signed 64-bit integer value. The wrapped value is _always_ negative.
    NegInt(i64),
-    /// 64-bit floating-point value
+    /// 64-bit floating-point value.
    Float(f64),


could consider making these variants #[non_exhaustive] which would prevent them from being directly instantiated so that we could insure that the invariants were held

What invariants? I don't understand the suggestion. These variants are being directly instantiated here:

https://github.com/awslabs/smithy-rs/blob/6130fcb9bd14e385dcd9be7cc6832771804c487a/rust-runtime/aws-smithy-json/src/deserialize.rs#L313-L349

by invariants I mean: /// Signed 64-bit integer value. The wrapped value is _always_ negative.
We would add constructor functions, ::pos_int, ::neg_int, ::float.

We would need to refactor code to use those. neg_int could panic if you passed in a positive int (or return a result, eg.)

not a blocker, just a possibility

…verters-to-numeric-types

david-perez · 2022-04-11T17:15:48Z

Should I add proptesting? In my tests I tested one or two values within the target type's range, as well as the values in the edges of the range (plus NaN and +-Infinity). If I add proptesting it would look like:

assert_eq!($typ::try_from(Number::PosInt(v)).unwrap(), v);

with v a random value within the range of $typ. Same with random values outside the range.

github-actions · 2022-04-11T17:29:08Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	12.81%	71424.88	63315.99
Total requests	12.87%	6434509	5701038
Total errors	NaN%	0	0
Total successes	12.87%	6434509	5701038
Average latency ms	34.94%	1.12	0.83
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	28.15%	23.72	18.51
Stdev latency ms	47.20%	1.84	1.25
Transfer Mb	12.86%	668.87	592.63
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

82marbag · 2022-07-11T12:21:19Z

Added needs-sdk-review, can we move this forward? @rcoh

CHANGELOG.next.toml

github-actions · 2022-07-19T16:56:31Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti

Overall, this looks good to me. Just a couple minor things.

rust-runtime/aws-smithy-types/src/lib.rs

...ain/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt

…nverters-to-numeric-types

github-actions · 2022-08-12T13:30:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

…nverters-to-numeric-types

github-actions · 2022-08-16T15:37:34Z

A new generated diff is ready to view.

A new doc preview is ready to view.

david-perez · 2022-08-16T15:57:23Z

@jdisanti @rcoh I'll merge this tomorrow if there are no further comments.

…nverters-to-numeric-types

github-actions · 2022-08-19T11:20:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

david-perez requested review from a team as code owners March 23, 2022 15:44

david-perez added 2 commits March 23, 2022 16:49

Update changelog

7cace8a

Kick off CI

ab6f306

fix needless_arbitrary_self_type

6130fcb

crisidev reviewed Mar 24, 2022

View reviewed changes

rust-runtime/aws-smithy-types/src/lib.rs Outdated Show resolved Hide resolved

crisidev reviewed Mar 24, 2022

View reviewed changes

rust-runtime/aws-smithy-types/src/lib.rs Outdated Show resolved Hide resolved

crisidev reviewed Mar 24, 2022

View reviewed changes

rust-runtime/aws-smithy-types/src/lib.rs Show resolved Hide resolved

crisidev approved these changes Mar 24, 2022

View reviewed changes

rcoh approved these changes Mar 24, 2022

View reviewed changes

david-perez added 3 commits April 11, 2022 18:50

Suggestions from PR

fc23fad

Merge remote-tracking branch 'awslabs/main' into davidpz-refactor-con…

ae129ec

…verters-to-numeric-types

Expand on breaking change in changelog

c21572d

82marbag added the needs-sdk-review label Jul 11, 2022

jdisanti reviewed Jul 19, 2022

View reviewed changes

CHANGELOG.next.toml Show resolved Hide resolved

Merge branch 'main' into davidpz-refactor-converters-to-numeric-types

8aba8cd

jdisanti approved these changes Jul 19, 2022

View reviewed changes

rust-runtime/aws-smithy-types/src/lib.rs Outdated Show resolved Hide resolved

...ain/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt Outdated Show resolved Hide resolved

jdisanti removed the needs-sdk-review label Jul 19, 2022

Rename some TryFromNumberError variants

e17da69

david-perez mentioned this pull request Aug 12, 2022

Should clients reject responses with sets with duplicate elements? smithy-lang/smithy#1266

Closed

david-perez added 3 commits August 12, 2022 15:16

Merge remote-tracking branch 'upstream/main' into davidpz-refactor-co…

1f383c7

…nverters-to-numeric-types

Unnecessary TryFrom import in 2021 edition

afffb9c

Merge remote-tracking branch 'upstream/main' into davidpz-refactor-co…

a67cb97

…nverters-to-numeric-types

david-perez added 3 commits August 16, 2022 14:35

appease ktlint

329a929

refactor aws-config CredentialProcess JSON parsing

c0400f6

Merge remote-tracking branch 'upstream/main' into davidpz-refactor-co…

9b7fb96

…nverters-to-numeric-types

Merge remote-tracking branch 'upstream/main' into davidpz-refactor-co…

752d632

…nverters-to-numeric-types

david-perez enabled auto-merge (squash) August 19, 2022 11:08

david-perez merged commit 7e7d571 into main Aug 19, 2022

david-perez deleted the davidpz-refactor-converters-to-numeric-types branch August 19, 2022 11:42

david-perez mentioned this pull request Aug 19, 2022

Panic when converting float to int #1235

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor converters to numeric types for `aws_smithy_types::Number` #1274

Refactor converters to numeric types for `aws_smithy_types::Number` #1274

david-perez commented Mar 23, 2022 •

edited

Loading

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

crisidev left a comment

rcoh left a comment

rcoh Mar 23, 2022

david-perez Apr 11, 2022

rcoh Mar 23, 2022

david-perez Apr 11, 2022 •

edited

Loading

rcoh Mar 23, 2022

david-perez Apr 11, 2022

rcoh Apr 11, 2022

david-perez commented Apr 11, 2022

github-actions bot commented Apr 11, 2022

82marbag commented Jul 11, 2022

github-actions bot commented Jul 19, 2022

jdisanti left a comment

github-actions bot commented Aug 12, 2022

github-actions bot commented Aug 16, 2022

david-perez commented Aug 16, 2022

github-actions bot commented Aug 19, 2022

Refactor converters to numeric types for aws_smithy_types::Number #1274

Refactor converters to numeric types for aws_smithy_types::Number #1274

Conversation

david-perez commented Mar 23, 2022 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

github-actions bot commented Mar 23, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

crisidev left a comment

Choose a reason for hiding this comment

rcoh left a comment

Choose a reason for hiding this comment

rcoh Mar 23, 2022

Choose a reason for hiding this comment

david-perez Apr 11, 2022

Choose a reason for hiding this comment

rcoh Mar 23, 2022

Choose a reason for hiding this comment

david-perez Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

rcoh Mar 23, 2022

Choose a reason for hiding this comment

david-perez Apr 11, 2022

Choose a reason for hiding this comment

rcoh Apr 11, 2022

Choose a reason for hiding this comment

david-perez commented Apr 11, 2022

github-actions bot commented Apr 11, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

82marbag commented Jul 11, 2022

github-actions bot commented Jul 19, 2022

jdisanti left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2022

github-actions bot commented Aug 16, 2022

david-perez commented Aug 16, 2022

github-actions bot commented Aug 19, 2022

Refactor converters to numeric types for `aws_smithy_types::Number` #1274

Refactor converters to numeric types for `aws_smithy_types::Number` #1274

david-perez commented Mar 23, 2022 •

edited

Loading

david-perez Apr 11, 2022 •

edited

Loading