Skip to content

Commit

Permalink
refactor!: use parse-display instead of serde
Browse files Browse the repository at this point in the history
To ensure enum string representations are consistent with their variant
names we want to derive the string handling (`Display`/`FromStr`). We've
so far used the somewhat convoluted approach of deriving
`serde::{Deserialize, Serialize}` and using `serde_plain`'s proc macros
to generate implementations of `Display` and `FromStr` that forward to
`serde`, but this puts quite a lot in the public API for each enum that
we don't really want:

- `serde::Deserialize`
- `serde::Serialize`
- `serde_plain::Error`

`parse-display` is another derive macro the specifically generates a
`Display` and corresponding `FromStr` implementation. It still leaks an
error type into the API but this is only one thing instead of 3.

BREAKING CHANGE: `Capability`, `ChangeSetStatus`, `ResourceStatus`,
`StackStatus`, `change_set::Evaluation`, `change_set::ExecutionStatus`,
`change_set::ModifyScope`, `change_set::Replacement`, and
`change_set::RequiresRecreation` no longer implement `serde::{Deserialize,
Serialize}`, and their `FromStr` implementations now use
`parse_display::ParseError` as the `Err` type.
  • Loading branch information
connec committed Oct 30, 2022
1 parent 45d977b commit 56dc415
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 84 deletions.
7 changes: 3 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,13 @@ aws-sdk-cloudformation = "0.19.0"
aws-sdk-sts = "0.19.0"
aws-smithy-types-convert = { version = "0.49.0", features = ["convert-chrono"] }
chrono = "0.4.19"
enumset = { version = "1.0.6", features = ["serde"] }
enumset = "1.0.6"
futures-util = "0.3.14"
lazy_static = "1.4.0"
parse-display = { version = "0.6.0", default-features = false }
regex = "1.5.4"
serde = { version = "1.0.125", features = ["derive"] }
serde_json = "1.0.85"
serde_plain = "0.3.0"
tokio = { version = "1.4.0" }
tokio = "1.4.0"

[dev-dependencies]
assert_matches = "1.5.0"
Expand Down
33 changes: 25 additions & 8 deletions src/apply_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use aws_sdk_cloudformation::{
use aws_smithy_types_convert::date_time::DateTimeExt;
use chrono::{DateTime, Utc};
use futures_util::{Stream, TryFutureExt, TryStreamExt};
use serde_plain::{forward_display_to_serde, forward_from_str_to_serde};

use crate::{
change_set::{
Expand Down Expand Up @@ -316,18 +315,18 @@ impl ApplyStackInput {
/// [2]: http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/template-macros.html
/// [`AWS::Include`]: http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/create-reusable-transform-function-snippets-and-add-to-your-template-with-aws-include-transform.html
/// [`AWS::Serverless`]: http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/transform-aws-serverless.html
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
pub enum Capability {
/// Acknowledge IAM resources (*without* custom names only).
#[serde(rename = "CAPABILITY_IAM")]
#[display("CAPABILITY_IAM")]
Iam,

/// Acknowledge IAM resources (with or without custom names).
#[serde(rename = "CAPABILITY_NAMED_IAM")]
#[display("CAPABILITY_NAMED_IAM")]
NamedIam,

/// Acknowledge macro expansion.
#[serde(rename = "CAPABILITY_AUTO_EXPAND")]
#[display("CAPABILITY_AUTO_EXPAND")]
AutoExpand,
}

Expand All @@ -341,9 +340,6 @@ impl Capability {
}
}

forward_display_to_serde!(Capability);
forward_from_str_to_serde!(Capability);

/// An input parameter for an `apply_stack` operation.
///
/// Note that, unlike when directly updating a stack, it is not possible to reuse previous
Expand Down Expand Up @@ -856,3 +852,24 @@ async fn describe_output(
.expect("DescribeStacksOutput empty stacks");
Ok(ApplyStackOutput::from_raw(stack))
}

#[cfg(test)]
mod tests {
use super::Capability;

#[test]
fn test_parse_display() {
assert_eq!(Capability::Iam.to_string(), "CAPABILITY_IAM");
assert_eq!(Capability::Iam, "CAPABILITY_IAM".parse().unwrap());
assert_eq!(Capability::NamedIam.to_string(), "CAPABILITY_NAMED_IAM");
assert_eq!(
Capability::NamedIam,
"CAPABILITY_NAMED_IAM".parse().unwrap(),
);
assert_eq!(Capability::AutoExpand.to_string(), "CAPABILITY_AUTO_EXPAND");
assert_eq!(
Capability::AutoExpand,
"CAPABILITY_AUTO_EXPAND".parse().unwrap(),
);
}
}
30 changes: 7 additions & 23 deletions src/change_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use aws_smithy_types_convert::date_time::DateTimeExt;
use chrono::{DateTime, Utc};
use enumset::EnumSet;
use futures_util::TryFutureExt;
use serde_plain::{forward_display_to_serde, forward_from_str_to_serde};
use tokio::time::{interval_at, Instant};

use crate::{
Expand Down Expand Up @@ -179,8 +178,8 @@ impl ChangeSet {
}

/// The change set's execution status.
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
#[display(style = "SNAKE_CASE")]
pub enum ExecutionStatus {
/// The change set is not available to execute.
Unavailable,
Expand All @@ -201,9 +200,6 @@ pub enum ExecutionStatus {
Obsolete,
}

forward_display_to_serde!(ExecutionStatus);
forward_from_str_to_serde!(ExecutionStatus);

/// A parameter set for a change set.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Parameter {
Expand Down Expand Up @@ -420,7 +416,7 @@ impl ModifyDetail {
/// [`Never`]: RequiresRecreation::Never
/// [`Static`]: Evaluation::Static
/// [`Dynamic`]: Evaluation::Dynamic
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
pub enum Replacement {
/// The resource will be replaced.
True,
Expand All @@ -432,18 +428,15 @@ pub enum Replacement {
Conditional,
}

forward_display_to_serde!(Replacement);
forward_from_str_to_serde!(Replacement);

// The derive for EnumSetType creates an item that triggers this lint, so it has to be disabled
// at the module level. We don't want to disable it too broadly though, so we wrap its declaration
// in a module and re-export from that.
mod modify_scope {
#![allow(clippy::expl_impl_clone_on_copy)]

/// Indicates which resource attribute is triggering this update.
#[derive(Debug, enumset::EnumSetType, serde::Deserialize, serde::Serialize)]
#[enumset(no_ops, serialize_as_list)]
#[derive(Debug, enumset::EnumSetType, parse_display::Display, parse_display::FromStr)]
#[enumset(no_ops)]
pub enum ModifyScope {
/// A change to the resource's properties.
Properties,
Expand All @@ -466,9 +459,6 @@ mod modify_scope {
}
pub use modify_scope::ModifyScope;

forward_display_to_serde!(ModifyScope);
forward_from_str_to_serde!(ModifyScope);

/// A change that AWS CloudFormation will make to a resource.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct ResourceChangeDetail {
Expand Down Expand Up @@ -592,7 +582,7 @@ impl ChangeSource {
}
}

#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
pub enum Evaluation {
/// AWS CloudFormation can determine that the target value will change, and its value.
///
Expand All @@ -611,9 +601,6 @@ pub enum Evaluation {
Dynamic,
}

forward_display_to_serde!(Evaluation);
forward_from_str_to_serde!(Evaluation);

/// The field that AWS CloudFormation will change, such as the name of a resource's property, and
/// whether the resource will be recreated.
#[derive(Clone, Debug, Eq, PartialEq)]
Expand Down Expand Up @@ -703,7 +690,7 @@ impl ResourceTargetDefinition {
}

/// Indicates whether a change to a property causes the resource to be recreated.
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
pub enum RequiresRecreation {
/// The resource will not need to be recreated.
Never,
Expand All @@ -718,9 +705,6 @@ pub enum RequiresRecreation {
Always,
}

forward_display_to_serde!(RequiresRecreation);
forward_from_str_to_serde!(RequiresRecreation);

pub(crate) struct ChangeSetWithType {
pub(crate) change_set: ChangeSet,
pub(crate) change_set_type: ChangeSetType,
Expand Down
4 changes: 1 addition & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ pub use apply_stack::{
pub use delete_stack::{DeleteStack, DeleteStackError, DeleteStackEvents, DeleteStackInput};
pub use event::{StackEvent, StackEventDetails};
pub use stack::{StackFailure, StackWarning};
pub use status::{
ChangeSetStatus, InvalidStatus, ResourceStatus, StackStatus, Status, StatusSentiment,
};
pub use status::{ChangeSetStatus, ResourceStatus, StackStatus, Status, StatusSentiment};
pub use tag::Tag;

/// A client for performing cloudformatious operations.
Expand Down
54 changes: 8 additions & 46 deletions src/status.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
//! Types and values representing various CloudFormation statuses.
#![allow(clippy::module_name_repetitions)]

use std::str::FromStr;

use serde_plain::forward_display_to_serde;

/// Common operations for statuses.
pub trait Status: std::fmt::Debug + std::fmt::Display + private::Sealed {
/// Indicates whether or not a status is terminal.
Expand Down Expand Up @@ -50,13 +46,9 @@ impl StatusSentiment {
}
}

/// An error marker returned when trying to parse an invalid status.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct InvalidStatus;

/// Possible change set statuses.
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
#[display(style = "SNAKE_CASE")]
pub enum ChangeSetStatus {
CreatePending,
CreateInProgress,
Expand Down Expand Up @@ -91,19 +83,9 @@ impl Status for ChangeSetStatus {
}
}

forward_display_to_serde!(ChangeSetStatus);

impl FromStr for ChangeSetStatus {
type Err = InvalidStatus;

fn from_str(s: &str) -> Result<Self, Self::Err> {
serde_plain::from_str(s).map_err(|_| InvalidStatus)
}
}

/// Possible stack statuses.
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
#[display(style = "SNAKE_CASE")]
pub enum StackStatus {
CreateInProgress,
CreateFailed,
Expand Down Expand Up @@ -185,19 +167,9 @@ impl Status for StackStatus {
}
}

forward_display_to_serde!(StackStatus);

impl FromStr for StackStatus {
type Err = InvalidStatus;

fn from_str(s: &str) -> Result<Self, Self::Err> {
serde_plain::from_str(s).map_err(|_| InvalidStatus)
}
}

/// Possible resource statuses.
#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
#[derive(Clone, Copy, Debug, Eq, PartialEq, parse_display::Display, parse_display::FromStr)]
#[display(style = "SNAKE_CASE")]
pub enum ResourceStatus {
CreateInProgress,
CreateFailed,
Expand Down Expand Up @@ -261,16 +233,6 @@ impl Status for ResourceStatus {
}
}

forward_display_to_serde!(ResourceStatus);

impl FromStr for ResourceStatus {
type Err = InvalidStatus;

fn from_str(s: &str) -> Result<Self, Self::Err> {
serde_plain::from_str(s).map_err(|_| InvalidStatus)
}
}

mod private {
/// An unreachable trait used to prevent some traits from being implemented outside the crate.
pub trait Sealed {}
Expand All @@ -295,7 +257,7 @@ mod tests {
"UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS".parse(),
Ok(StackStatus::UpdateRollbackCompleteCleanupInProgress)
);
assert_eq!("oh no".parse::<StackStatus>(), Err(InvalidStatus));
assert!("oh no".parse::<StackStatus>().is_err());
}

#[test]
Expand All @@ -309,6 +271,6 @@ mod tests {
"IMPORT_ROLLBACK_IN_PROGRESS".parse(),
Ok(ResourceStatus::ImportRollbackInProgress)
);
assert_eq!("oh no".parse::<ResourceStatus>(), Err(InvalidStatus));
assert!("oh no".parse::<ResourceStatus>().is_err());
}
}

0 comments on commit 56dc415

Please sign in to comment.