Smithy 1.9/1.10 Upgrade #618
Conversation
This upgrades to Smithy 1.10, but the major change is a complete overhaul of how primitives are formatted and parsed. Primitive serialization was migrated and unified into Smithy Types with the end requirement of dealing with special float serialization semantics.
Smithy 1.9.1 brings S3UnwrappedXmlOutput as a vended trait. This commit pulls in the new model & uses that trait.
There was a problem hiding this comment.
This looks good! Just some minor comments to improve clarity. I think we should decide on how tied to the specifics of Smithy the JSON runtime crate should be. It has smithy- in the name, but I originally intended for it to be mostly usable outside of the context of Smithy. I guess it has the timestamp logic in it, so maybe I'm clinging to something that doesn't exist.
| ModelTransformer.create().mapShapes(model) { shape -> | ||
| // Apply the S3UnwrappedXmlOutput customization to GetBucketLocation (more | ||
| // details on the S3UnwrappedXmlOutputTrait) | ||
| if (shape is StructureShape && shape.id == ShapeId.from("com.amazonaws.s3#GetBucketLocationOutput")) { |
...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGenerator.kt
Outdated
Show resolved
Hide resolved
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt
Outdated
Show resolved
Hide resolved
| if (innerMemberType.isPrimitive()) { | ||
| rust("let mut encoder = #T::from(*$innerField);", Encoder) | ||
| } |
There was a problem hiding this comment.
Is there any way to have this be in closer proximity to the encoder.encode() call?
There was a problem hiding this comment.
sigh, not without a bunch of refactoring. The one downside of the encoder pattern is that it needs to be kept alive until the slice has made it to its final home
| @@ -354,6 +361,8 @@ class RequestBindingGenerator( | |||
| return true | |||
| } | |||
|
|
|||
| private val Encoder = CargoDependency.SmithyTypes(runtimeConfig).asType().member("primitive::Encoder") | |||
There was a problem hiding this comment.
This should go at the top of the class with the other private members so that it's discoverable.
rust-runtime/smithy-json/Cargo.toml
Outdated
| @@ -6,7 +6,6 @@ edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| itoa = "0.4" | |||
| Some(Token::ValueNull { .. }) => Ok(None), | ||
| Some(Token::ValueNumber { value, .. }) => Ok(Some(value)), | ||
| Some(Token::ValueString { value, offset }) => match value.to_unescaped() { | ||
| Err(_) => Err(Error::custom("expected a valid string, escape was invalid")), |
There was a problem hiding this comment.
I think you're discarding some details about the underlying cause of the error here.
There was a problem hiding this comment.
good point, recovered them
| Some(Token::ValueNumber { value, .. }) => Ok(Some(value)), | ||
| Some(Token::ValueString { value, offset }) => match value.to_unescaped() { | ||
| Err(_) => Err(Error::custom("expected a valid string, escape was invalid")), | ||
| Ok(v) => f64::parse(v.as_ref()) |
There was a problem hiding this comment.
It's easy to mistake this for the std library from_str operation, which accepts inf instead of Infinity. It works, but it may lead people astray. Maybe call the function parse_primitive so it's more apparent that smithy-types is adding it?
There was a problem hiding this comment.
renamed to parse_smithy_primitive
| // JSON doesn't support NaN, Infinity, or -Infinity, so we're matching | ||
| // Smithy has specific behavior for infinity & NaN |
There was a problem hiding this comment.
I think we're conflating things here. smithy-json is intended to just do JSON, and this is a Smithy detail on top of JSON. Seems like it should be behavior that is added via inlineable. What are your thoughts?
There was a problem hiding this comment.
given that these are smithy specific libraries I think it's pretty reasonable. The behavior only augments the expect_or helpers instead of the raw token stream. We could consider extracting those somewhere protocol specific
There was a problem hiding this comment.
but I think for now, a JSON library that "does the right thing" is probably the best option
| #[non_exhaustive] | ||
| Bool(bool), | ||
| #[non_exhaustive] | ||
| I8(i8, itoa::Buffer), |
There was a problem hiding this comment.
Clever putting the buffers inside the encoder to avoid allocations! Nice.
Co-authored-by: John DiSanti <[email protected]>
Issue #, if available: Fixes #603
Description of changes: This diff upgrades smithy-rs to Smithy 1.10. The most significant change is updates to the float parsing behavior. To handle this, I centralized all of our primitive parsing behavior into
smithy_types::primitiveand refactored the numerous protocols:to use the implementation. This has the side benefit that we now serialize primitives without incurring an additional allocation to
Stringin most cases becauseprimitive::Encoderreturns&strand uses a stack allocation to store the resulting value.The second commit updates the S3 unwrapped response customization to use the new smithy-provided trait.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.