Unverified Commit b4f294c2 authored by ysaito1001's avatar ysaito1001 Committed by GitHub
Browse files

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: default avatarJohn DiSanti <jdisanti@amazon.com>

* 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: default avatarSaito <awsaito@c889f3b5ddc4.ant.amazon.com>
Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>
Co-authored-by: default avatarZelda Hessler <zhessler@amazon.com>
parent 7e666dab

CHANGELOG.next.toml

+6 −0
Original line number Diff line number Diff line
@@ -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

+1 −1
Original line number Diff line number Diff line
@@ -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

+6 −1
Original line number Diff line number Diff line
@@ -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

+23 −8
Original line number Diff line number Diff line
@@ -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

+1 −1
Original line number Diff line number Diff line
@@ -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.