Revert "Deprecate url path cleaning during http request serialization" (#4849)

* Revert "Deprecate url path cleaning during http request serialization"

Author: Steven Yuan

CHANGELOG_PENDING.md

@@ -3,3 +3,7 @@
 ### SDK Enhancements
 
 ### SDK Bugs
+* `rest`: Revert removing unnecessary path normalization behavior.
+  * This behavior would mutate request paths with URI-encoded characters, potentially resulting in misrouted requests.
+* `config`: Revert deprecating `DisableRestProtocolURICleaning` config setting.
+  * This setting will have an effect again. REST-protocol paths will now be normalized after serialization.

aws/config.go

@@ -252,8 +252,19 @@ type Config struct {
 	// and specify a Retryer instead.
 	SleepDelay func(time.Duration)
 
-	// Deprecated: This setting no longer has any effect.
-	// RESTful paths are no longer cleaned after request serialization.
+	// DisableRestProtocolURICleaning will not clean the URL path when making rest protocol requests.
+	// Will default to false. This would only be used for empty directory names in s3 requests.
+	//
+	// Example:
+	//    sess := session.Must(session.NewSession(&aws.Config{
+	//         DisableRestProtocolURICleaning: aws.Bool(true),
+	//    }))
+	//
+	//    svc := s3.New(sess)
+	//    out, err := svc.GetObject(&s3.GetObjectInput {
+	//    	Bucket: aws.String("bucketname"),
+	//    	Key: aws.String("//foo//bar//moo"),
+	//    })
 	DisableRestProtocolURICleaning *bool
 
 	// EnableEndpointDiscovery will allow for endpoint discovery on operations that
@@ -486,8 +497,8 @@ func (c *Config) WithLowerCaseHeaderMaps(t bool) *Config {
 	return c
 }
 
-// Deprecated: This setting no longer has any effect.
-// RESTful paths are no longer cleaned after request serialization.
+// WithDisableRestProtocolURICleaning sets a config DisableRestProtocolURICleaning value
+// returning a Config pointer for chaining.
 func (c *Config) WithDisableRestProtocolURICleaning(t bool) *Config {
 	c.DisableRestProtocolURICleaning = &t
 	return c
@@ -600,7 +611,7 @@ func mergeInConfig(dst *Config, other *Config) {
 	if other.DisableRestProtocolURICleaning != nil {
 		dst.DisableRestProtocolURICleaning = other.DisableRestProtocolURICleaning
 	}
-	
+
 	if other.EnforceShouldRetryCheck != nil {
 		dst.EnforceShouldRetryCheck = other.EnforceShouldRetryCheck
 	}

private/protocol/rest/build.go

@@ -9,6 +9,7 @@ import (
 	"math"
 	"net/http"
 	"net/url"
+	"path"
 	"reflect"
 	"strconv"
 	"strings"
@@ -133,6 +134,9 @@ func buildLocationElements(r *request.Request, v reflect.Value, buildGETQuery bo
 	}
 
 	r.HTTPRequest.URL.RawQuery = query.Encode()
+	if !aws.BoolValue(r.Config.DisableRestProtocolURICleaning) {
+		cleanPath(r.HTTPRequest.URL)
+	}
 }
 
 func buildBody(r *request.Request, v reflect.Value) {
@@ -240,6 +244,19 @@ func buildQueryString(query url.Values, v reflect.Value, name string, tag reflec
 	return nil
 }
 
+func cleanPath(u *url.URL) {
+	hasSlash := strings.HasSuffix(u.Path, "/")
+
+	// clean up path, removing duplicate `/`
+	u.Path = path.Clean(u.Path)
+	u.RawPath = path.Clean(u.RawPath)
+
+	if hasSlash && !strings.HasSuffix(u.Path, "/") {
+		u.Path += "/"
+		u.RawPath += "/"
+	}
+}
+
 // EscapePath escapes part of a URL path in Amazon style
 func EscapePath(path string, encodeSep bool) string {
 	var buf bytes.Buffer

private/protocol/rest/build_test.go

@@ -14,55 +14,17 @@ import (
 	"github.com/aws/aws-sdk-go/aws/request"
 )
 
-func TestBuildURI(t *testing.T) {
-	cases := map[string]struct {
-		Topic         *string
-		ExpectPath    string
-		ExpectRawPath string
-	}{
-		"path without prefix slash": {
-			Topic:         aws.String("devices/123/test"),
-			ExpectPath:    "/topics/devices/123/test",
-			ExpectRawPath: "/topics/devices%2F123%2Ftest",
-		},
-		"path containing single prefix slashes": {
-			Topic:         aws.String("/devices/123/test"),
-			ExpectPath:    "/topics//devices/123/test",
-			ExpectRawPath: "/topics/%2Fdevices%2F123%2Ftest",
-		},
-		"path containing prefix multi-slashes": {
-			Topic:         aws.String("///devices/123/test"),
-			ExpectPath:    "/topics////devices/123/test",
-			ExpectRawPath: "/topics/%2F%2F%2Fdevices%2F123%2Ftest",
-		},
+func TestCleanPath(t *testing.T) {
+	uri := &url.URL{
+		Path:   "//foo//bar",
+		Scheme: "https",
+		Host:   "host",
 	}
+	cleanPath(uri)
 
-	for _, c := range cases {
-		uri := &url.URL{
-			Path:   "/topics/{topic}",
-			Scheme: "https",
-			Host:   "host",
-		}
-
-		req := &request.Request{
-			HTTPRequest: &http.Request{
-				URL: uri,
-			},
-			Params: &struct {
-				Topic *string `location:"uri" locationName:"topic" type:"string" required:"true"`
-			}{
-				c.Topic,
-			},
-		}
-
-		Build(req)
-
-		if a, e := uri.Path, c.ExpectPath; a != e {
-			t.Errorf("expect %q Path, got %q", e, a)
-		}
-		if a, e := uri.RawPath, c.ExpectRawPath; a != e {
-			t.Errorf("expect %q RawPath, got %q", e, a)
-		}
+	expected := "https://host/foo/bar"
+	if a, e := uri.String(), expected; a != e {
+		t.Errorf("expect %q URI, got %q", e, a)
 	}
 }