Enforce constraints for unnamed enums #3884
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
drganjoo
force-pushed
the
fahadzub/unnamed-enum-constrained
branch
from
f2aa580
to
651538a
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
drganjoo
force-pushed
the
fahadzub/unnamed-enum-constrained
branch
from
17ca030
to
bfdf606
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| """ | ||
| impl<T> #{From}<T> for ${context.enumName} where T: #{AsRef}<str> { | ||
| fn from(s: T) -> Self { | ||
| ${context.enumName}(s.as_ref().to_owned()) |
There was a problem hiding this comment.
There's no test for these client changes? Also bot posts no codegen diff, which is concerning since it'd mean we're not exercising this.
There was a problem hiding this comment.
Good point. Let me add a test for this.
There was a problem hiding this comment.
Ah, now I remember—there’s already a test that verifies From<&str> and FromStr are implemented for unnamed enums.
This test for unnamed enums generates the following:
#[test]
fn generate_unnamed_enums() {
let result = "t2.nano"
.parse::<crate::types::UnnamedEnum>()
.expect("static value validated to member");
assert_eq!(result, UnnamedEnum("t2.nano".to_owned()));
}The .parse method calls the FromStr implementation, which in turn calls From<&str>.
I’ll also update the test to ensure no validation occurs on the client side.
| @@ -10,6 +10,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rust | |||
| import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate | |||
| import software.amazon.smithy.rust.codegen.core.rustlang.writable | |||
| import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType | |||
| import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope | |||
| import software.amazon.smithy.rust.codegen.core.util.dq | |||
|
|
|||
| object TestEnumType : EnumType() { | |||
There was a problem hiding this comment.
Huh, I wonder why we have this class at all i.e. why we don't test directly against InfallibleEnumType. It feels wrong to copy over the implementations from the "real" classes to this class.
There was a problem hiding this comment.
I agree with you on this. I'll see if it can be easily fixed in this PR, otherwise will raise another one for this.
There was a problem hiding this comment.
InfallibleEnumType allows UnknownVariants, whereas TestEnumType panics when an unknown variant is encountered. I tried composing TestEnumType to use InfallibleEnumType internally, but InfallibleEnumType is defined in codegen-client, while TestEnumType is in codegen-core.
| impl #{TryFrom}<&str> for ${context.enumName} { | ||
| type Error = #{ConstraintViolation}; | ||
| fn try_from(s: &str) -> #{Result}<Self, <Self as #{TryFrom}<&str>>::Error> { | ||
| s.to_owned().try_into() |
There was a problem hiding this comment.
This is first converting to a heap-allocated String and only then matching on the enum values. So invalid enum values would unnecessarily be heap-allocated. We should make TryFrom<String> and TryFrom<&str> instead delegate to FromStr, which should only heap-allocate when the enum value is valid.
There was a problem hiding this comment.
Unlike named enums, which can use &str for comparison and directly return an enum variant, unnamed enums need to store an owned, heap-allocated String. The TryFrom<String> implementation already receives an owned String as a parameter, so delegating to FromStr would result in an unnecessary heap allocation. Additionally, ConstraintViolation takes ownership of a heap-allocated String. Therefore, both code paths (valid and invalid enum values) require a heap-allocated String. Calling to_owned within TryFrom<&str> only shifts the allocation slightly earlier without adding any additional allocation.
...n/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGenerator.kt
Show resolved
Hide resolved
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
drganjoo
force-pushed
the
fahadzub/unnamed-enum-constrained
branch
from
fbcefa0
to
042fe5c
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Enforces Constraints for Unnamed Enums
This PR addresses the issue where unnamed enums were incorrectly treated as infallible during deserialization, allowing any string value to be converted without validation. The solution introduces a
ConstraintViolationandTryFromimplementation for unnamed enums, ensuring that deserialized values conform to the enum variants defined in the Smithy model.This change prevents invalid values from being deserialized into unnamed enums and raises appropriate constraint violations when necessary.
Fixes issue #3880