Skip to content
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

Code generation incorrectly assumes ConstraintViolation exists for unnamed enums, causing compilation errors. #3880

Closed
drganjoo opened this issue Oct 17, 2024 · 1 comment
Labels
high-priority High priority issue server Rust server SDK

Comments

@drganjoo
Copy link
Contributor

Issue Summary

The code generation process for structures containing a member shape that targets an unnamed enum mistakenly assumes that the generated enum structure enforces constraints on deserializable values, treating it as if a corresponding ConstraintViolation structure exists. However, unnamed enums inherently support infallible conversion from strings, allowing any string value to be converted to the enum without restriction.

As a result, the generated code cannot be compiled due to missing ConstraintViolation definitions.

Example Model:

$version: "2"
namespace com.aws.example
use aws.protocols#restJson1
use smithy.framework#ValidationException

@restJson1
service PokemonService {
    operations: [SampleOp]
}

@http(uri: "/dailySummary", method: "POST")
operation SampleOp {
    input := {
        day: WeeklySummary
    }
    errors: [ValidationException]
}

structure WeeklySummary {
    day: DayOfWeek,
}

@enum([
    { value: "MONDAY" },
    { value: "TUESDAY" }
])
string DayOfWeek

Generated Code

For DayOfWeek:

pub struct DayOfWeek(String);

impl DayOfWeek {
    pub fn as_str(&self) -> &str {
        &self.0
    }
    pub const fn values() -> &'static [&'static str] {
        &["MONDAY", "TUESDAY"]
    }
}

impl ::std::str::FromStr for DayOfWeek {
    type Err = ::std::convert::Infallible;

    fn from_str(s: &str) -> ::std::result::Result<Self, <Self as ::std::str::FromStr>::Err> {
        ::std::result::Result::Ok(DayOfWeek::from(s))
    }
}

impl crate::constrained::Constrained for DayOfWeek {
    type Unconstrained = ::std::string::String;
}

impl ::std::convert::From<::std::string::String>
    for crate::constrained::MaybeConstrained<crate::model::DayOfWeek>
{
    fn from(value: ::std::string::String) -> Self {
        Self::Unconstrained(value)
    }
}

However, the generated code for WeeklySummary incorrectly includes a ConstraintViolation enum with a Day variant, expecting an associated ConstraintViolation field of type crate::model::day_of_week::ConstraintViolation, which does not exist. This results in a compilation error.

pub mod weekly_summary {

    #[derive(::std::cmp::PartialEq, ::std::fmt::Debug)]
    /// Holds one variant for each way the builder can fail.
    #[non_exhaustive]
    #[allow(clippy::enum_variant_names)]
    pub enum ConstraintViolation {
        /// Constraint violation occurred building member `day` when building `WeeklySummary`.
        #[doc(hidden)]
        Day(crate::model::day_of_week::ConstraintViolation),
    }
    impl ::std::fmt::Display for ConstraintViolation {
        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
            match self {
                ConstraintViolation::Day(_) => write!(f, "constraint violation occurred building member `day` when building `WeeklySummary"),
            }
        }
    }
    impl ::std::error::Error for ConstraintViolation {}
    impl ConstraintViolation {
        pub(crate) fn as_validation_exception_field(
            self,
            path: ::std::string::String,
        ) -> crate::model::ValidationExceptionField {
            match self {
                ConstraintViolation::Day(inner) => {
                    inner.as_validation_exception_field(path + "/day")
                }
            }
        }
    }

This issue arises because the unnamed enum (DayOfWeek) does not include constraint enforcement, but the generated code for WeeklySummary assumes a ConstraintViolation structure exists for DayOfWeek.

@drganjoo drganjoo added server Rust server SDK high-priority High priority issue labels Oct 17, 2024
@drganjoo
Copy link
Contributor Author

The current implementation assumes that unnamed enum variants don't require validation, but this assumption is incorrect. Even for unnamed enum variants, server-side validation of the input string against the possible enum values is crucial for maintaining data integrity and preventing unexpected behavior.

To address this, we should implement validation for unnamed enum types as well. Something like:

impl TryFrom<&str> for DayOfWeek {
    type Error = crate::model::day_of_week::ConstraintViolation;

    fn try_from(s: &str) -> Result<Self, Self::Error> {
        if matches!(s, "MONDAY" | "TUESDAY") {
            Ok(Self::from(s))
        } else {
            Err(Self::Error(s.to_owned()))
        }
    }
}

github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2024
### Enforces Constraints for Unnamed Enums

This PR addresses the issue where, on the server side, unnamed enums
were incorrectly treated as infallible during deserialization, allowing
any string value to be converted without validation. The solution
introduces a `ConstraintViolation` and `TryFrom` implementation for
unnamed enums, ensuring that deserialized values conform to the enum
variants defined in the Smithy model.

The following is an example of an unnamed enum:

```smithy
@enum([
    { value: "MONDAY" },
    { value: "TUESDAY" }
])
string UnnamedDayOfWeek
```

On the server side the following type is generated for the Smithy shape:

```rust
pub struct UnnamedDayOfWeek(String);

impl ::std::convert::TryFrom<::std::string::String> for UnnamedDayOfWeek {
    type Error = crate::model::unnamed_day_of_week::ConstraintViolation;

    fn try_from(
        s: ::std::string::String,
    ) -> ::std::result::Result<Self, <Self as ::std::convert::TryFrom<::std::string::String>>::Error>
    {
        match s.as_str() {
            "MONDAY" | "TUESDAY" => Ok(Self(s)),
            _ => Err(crate::model::unnamed_day_of_week::ConstraintViolation(s)),
        }
    }
}
```

This change prevents invalid values from being deserialized into unnamed
enums and raises appropriate constraint violations when necessary.

There is one difference between the Rust code generated for
`TryFrom<String>` for named enums versus unnamed enums. The
implementation for unnamed enums passes the ownership of the `String`
parameter to the generated structure, and the implementation for
`TryFrom<&str>` delegates to `TryFrom<String>`.

```rust
impl ::std::convert::TryFrom<::std::string::String> for UnnamedDayOfWeek {
    type Error = crate::model::unnamed_day_of_week::ConstraintViolation;
    fn try_from(
        s: ::std::string::String,
    ) -> ::std::result::Result<Self, <Self as ::std::convert::TryFrom<::std::string::String>>::Error>
    {
        match s.as_str() {
            "MONDAY" | "TUESDAY" => Ok(Self(s)),
            _ => Err(crate::model::unnamed_day_of_week::ConstraintViolation(s)),
        }
    }
}

impl ::std::convert::TryFrom<&str> for UnnamedDayOfWeek {
    type Error = crate::model::unnamed_day_of_week::ConstraintViolation;
    fn try_from(
        s: &str,
    ) -> ::std::result::Result<Self, <Self as ::std::convert::TryFrom<&str>>::Error> {
        s.to_owned().try_into()
    }
}
```

On the client side, the behaviour is unchanged, and the client does not
validate for backward compatibility reasons. An [existing
test](https://github.com/smithy-lang/smithy-rs/pull/3884/files#diff-021ec60146cfe231105d21a7389f2dffcd546595964fbb3f0684ebf068325e48R82)
has been modified to ensure this.

```rust
#[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()));
    let result = "not-a-valid-variant"
        .parse::<crate::types::UnnamedEnum>()
        .expect("static value validated to member");
    assert_eq!(result, UnnamedEnum("not-a-valid-variant".to_owned()));
}
```

Fixes issue #3880

---------

Co-authored-by: Fahad Zubair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority High priority issue server Rust server SDK
Projects
None yet
Development

No branches or pull requests

1 participant