feat: Rust codegen for Constraints #582
Conversation
| pub(crate) async fn send( | ||
| client: &crate::client::Client, | ||
| input: crate::operation::get_resource::GetResourceInput, | ||
| ) -> ::std::result::Result< | ||
| crate::operation::get_resource::GetResourceOutput, | ||
| crate::types::error::Error, | ||
| > { | ||
| if input.name.is_none() { | ||
| return ::std::result::Result::Err(::aws_smithy_types::error::operation::BuildError::missing_field( |
There was a problem hiding this comment.
I feel like going through a aws_smithy_types::error is adding complexity without adding value.
Couldn't we just make a wrap_validation_err out of a String and leave it as that?
There was a problem hiding this comment.
I think it's valuable: it makes it possible to programmatically handle validation errors, and I can see that being useful for middleware that handles translation of Smithy-modeled types, for example.
As long as displaying these errors gives a good user experience I say we keep them.
...n/src/main/java/software/amazon/polymorph/smithyrust/generator/RustLibraryShimGenerator.java
Show resolved
Hide resolved
| // scalar values | ||
| ? "x.chars().count()" | ||
| // bytes | ||
| : "x.len()"; |
There was a problem hiding this comment.
This is technically wrong, but it looks like the other languages are also wrong according to the Smithy spec: #610.
Would it be a massive pain to call something that calculates the number of UTF-16 code points instead, to at least be consistent with the other languages?
There was a problem hiding this comment.
It turns out that it's not too difficult to count UTF-16 code points instead, so I've implemented it in the latest commit.
...n/src/main/java/software/amazon/polymorph/smithyrust/generator/RustLibraryShimGenerator.java
Show resolved
Hide resolved
codegen/smithy-dafny-codegen/src/main/resources/templates/runtimes/rust/types/error.rs
Show resolved
Hide resolved
.../src/main/java/software/amazon/polymorph/smithyrust/generator/AbstractRustShimGenerator.java
Show resolved
Hide resolved
| list: ::std::vec::Vec<Self>, | ||
| message: ::std::string::String, | ||
| }, | ||
| ValidationError(ValidationError), |
There was a problem hiding this comment.
Could this be ValidationError(BuildError) instead?
There was a problem hiding this comment.
For implementing constraint trait validation only, yes, I believe this could just directly wrap a BuildError.
There's two levels of indirection here:
- This PR uses an
Rc<dyn std::error::Error>instead ofBuildErrordirectly, because I was not (and am not) sure that all validation errors will beBuildErrors. - In order to safely up-/down-cast the contents of the
Error::ValidationErrorvariant from thedyn Anyin an arbitrarydafny_runtime::Object, it's best to define a unique type that we own - that is, theValidationErrorstruct (not the enum variant). If we definedEnum::ValidationError(Rc<...>)orEnum::ValidationError<BuildError>, then it seems possible for us to mishandle adafny_runtime::Objectcontaining anRc<?>orBuildErrorfrom somewhere else.
In my mind, this doesn't make the library harder to use: the typical library user would mainly care whether or not the error variant is a ValidationError or something else. If they want the message or the underlying dyn std::error::Error, there's no need to manually traverse the indirections: they can use to_string() and source() as you would for any other std::error::Error.
There was a problem hiding this comment.
Fair points. What would it look like for an end-user to react differently to a validation error, and actually dig out the offending field from the wrapped BuildError?
There was a problem hiding this comment.
There isn't a clean way to dig out the name of the offending field right now. Neither BuildError nor its inner BuildErrorKind give public visibility to their members.
Did you have a particular use-case in mind for needing to programmatically retrieve the name of the invalid field, rather than showing the full error message?
There was a problem hiding this comment.
I see what you mean - I had thought BuildErrorKind was structured rather than just a string to enable programmatic handling, but it looks more like the main purpose is just to display a nice error message string.
Looks like we're meeting the bar of the existing smithy-rs behavior so I think this is the right call for now. We can potentially revisit and improve as part of #598
*Issue #, if available:* #533 *Description of changes:* This PR implements Rust library codegen for input validation of supported constraint traits (`@required`, `@length`, `@range`). Notes: * Unlike in Java and C#, library users may directly construct the generated operation input structures in Rust (in order to aid in idiomatic usage, especially w.r.t. pattern matching, like the AWS SDK for Rust does) and are not restricted to using the builder pattern. Therefore, the builder can't take sole responsibility for validation; instead, validation happens whenever an operation is called. * Unlike other target languages, Rust is limited in its ability to propagate arbitrary errors up the call stack unless the API is designed for doing so. Our models don't define a specific error shape for validation, so in order to represent validation errors in the generated `Error` enum, we generate a `ValidationError` variant that only exists on the Rust side. When converting to Dafny, we upcast the error into the `Opaque` variant; when converting from Dafny, we downcast it back after checking that doing so is safe using `dafny_runtime::is_object!`. * By Polymorph's design, values crossing the boundary from Dafny-compiled code are assumed to be "valid", but the `WrappedSimpleConstraintsTest` intentionally breaks this assumption in order to test the constraint validation logic, forcing invalid values across the boundary by (ab)using `{:axiom}`. This would be okay except that the Dafny tests in the Constraints test model *also* pass across malformed UTF-8 for `@DafnyUtf8Bytes` strings (which is not a constraint trait). The Rust type conversion process does not allow for failure when converting values from Dafny (doing so would be a major refactor and probably some performance impact), so I've isolated the malformed UTF-8 test assertions and used a bit of `sed` to simply disable them for Rust specifically, while allowing the other languages to continue testing them.
Issue #, if available: #533
Description of changes:
This PR implements Rust library codegen for input validation of supported constraint traits (
@required,@length,@range).Notes:
Errorenum, we generate aValidationErrorvariant that only exists on the Rust side. When converting to Dafny, we upcast the error into theOpaquevariant; when converting from Dafny, we downcast it back after checking that doing so is safe usingdafny_runtime::is_object!.WrappedSimpleConstraintsTestintentionally breaks this assumption in order to test the constraint validation logic, forcing invalid values across the boundary by (ab)using{:axiom}. This would be okay except that the Dafny tests in the Constraints test model also pass across malformed UTF-8 for@DafnyUtf8Bytesstrings (which is not a constraint trait). The Rust type conversion process does not allow for failure when converting values from Dafny (doing so would be a major refactor and probably some performance impact), so I've isolated the malformed UTF-8 test assertions and used a bit ofsedto simply disable them for Rust specifically, while allowing the other languages to continue testing them.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.