Replace bool with enum for a function parameter of label::fmt_string (#…

…1875)

* Replace bool with enum in label::fmt_string

This commit addresses code smell where the said function takes a bool
to decide whether it percent-encodes the UTF-8 encoding of the given
string. For clarity, we define an enum EncodingStrategy so that we
can see the callers' intent at the call sites by reading the variant
names rather than true/false.

* Update CHANGELOG.next.toml

* Update CHANGELOG.next.toml

Co-authored-by: John DiSanti <[email protected]>

* Make test use updated function signature of fmt_string

This commit modifies a call site for the updated function signature of
`aws_smithy_http::label::fmt_string`, otherwise the test would fail to
compile, leading to a failure in CI.

Co-authored-by: Saito <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: Zelda Hessler <[email protected]>

CHANGELOG.next.toml

@@ -22,3 +22,9 @@ message = "Support Sigv4 signature generation on PowerPC 32 and 64 bit. This arc
 references = ["smithy-rs#1847"]
 meta = { "breaking" = true, "tada" = false, "bug" = true }
 author = "crisidev"
+
+[[smithy-rs]]
+message = "Replace bool with enum for a function parameter of `label::fmt_string`."
+references = ["smithy-rs#1875"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "ysaito1001"

aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs

@@ -10,5 +10,5 @@ pub(super) fn percent_encode_query(value: &str) -> String {
 }
 
 pub(super) fn percent_encode_path(value: &str) -> String {
-    label::fmt_string(value, true)
+    label::fmt_string(value, label::EncodingStrategy::Greedy)
 }

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/RequestBindingGenerator.kt

@@ -261,7 +261,12 @@ class RequestBindingGenerator(
         when {
             target.isStringShape -> {
                 val func = format(RuntimeType.LabelFormat(runtimeConfig, "fmt_string"))
-                rust("let $outputVar = $func($input, ${label.isGreedyLabel});")
+                val encodingStrategy = if (label.isGreedyLabel) {
+                    RuntimeType.LabelFormat(runtimeConfig, "EncodingStrategy::Greedy")
+                } else {
+                    RuntimeType.LabelFormat(runtimeConfig, "EncodingStrategy::Default")
+                }
+                rust("let $outputVar = $func($input, #T);", encodingStrategy)
             }
             target.isTimestampShape -> {
                 val timestampFormat =

rust-runtime/aws-smithy-http/src/label.rs

@@ -13,32 +13,47 @@ use percent_encoding::AsciiSet;
 
 const GREEDY: &AsciiSet = &BASE_SET.remove(b'/');
 
-pub fn fmt_string<T: AsRef<str>>(t: T, greedy: bool) -> String {
-    let uri_set = if greedy { GREEDY } else { BASE_SET };
+#[non_exhaustive]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum EncodingStrategy {
+    Default,
+    Greedy,
+}
+
+pub fn fmt_string<T: AsRef<str>>(t: T, strategy: EncodingStrategy) -> String {
+    let uri_set = if strategy == EncodingStrategy::Greedy {
+        GREEDY
+    } else {
+        BASE_SET
+    };
     percent_encoding::utf8_percent_encode(t.as_ref(), uri_set).to_string()
 }
 
 pub fn fmt_timestamp(t: &DateTime, format: Format) -> Result<String, DateTimeFormatError> {
-    Ok(crate::query::fmt_string(t.fmt(format)?))
+    Ok(fmt_string(t.fmt(format)?, EncodingStrategy::Default))
 }
 
 #[cfg(test)]
 mod test {
-    use crate::label::fmt_string;
+    use crate::label::{fmt_string, EncodingStrategy};
     use http::Uri;
     use proptest::proptest;
 
     #[test]
     fn greedy_params() {
-        assert_eq!(fmt_string("a/b", false), "a%2Fb");
-        assert_eq!(fmt_string("a/b", true), "a/b");
+        assert_eq!(fmt_string("a/b", EncodingStrategy::Default), "a%2Fb");
+        assert_eq!(fmt_string("a/b", EncodingStrategy::Greedy), "a/b");
     }
 
     proptest! {
         #[test]
         fn test_encode_request(s: String) {
-            let _: Uri = format!("http://host.example.com/{}", fmt_string(&s, false)).parse().expect("all strings should be encoded properly");
-            let _: Uri = format!("http://host.example.com/{}", fmt_string(&s, true)).parse().expect("all strings should be encoded properly");
+            let _: Uri = format!("http://host.example.com/{}", fmt_string(&s, EncodingStrategy::Default))
+                .parse()
+                .expect("all strings should be encoded properly");
+            let _: Uri = format!("http://host.example.com/{}", fmt_string(&s, EncodingStrategy::Greedy))
+                .parse()
+                .expect("all strings should be encoded properly");
         }
     }
 }

rust-runtime/aws-smithy-http/src/urlencode.rs

@@ -6,7 +6,7 @@
 use percent_encoding::{AsciiSet, CONTROLS};
 
 /// base set of characters that must be URL encoded
-pub const BASE_SET: &AsciiSet = &CONTROLS
+pub(crate) const BASE_SET: &AsciiSet = &CONTROLS
     .add(b' ')
     .add(b'/')
     // RFC-3986 §3.3 allows sub-delims (defined in section2.2) to be in the path component.