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.

Sign up for GitHub

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

Jump to bottom

Deprecate url path cleaning during http request serialization #4832

Merged
merged 11 commits into from
May 18, 2023
Merged

Deprecate url path cleaning during http request serialization #4832

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c1308d6
preserve IoT published topic slash
May 9, 2023
d107311
add changelog
May 9, 2023
afc6b87
optimize code
May 9, 2023
6e99df2
Merge remote-tracking branch 'origin/iot-topic' into iot-topic
May 9, 2023
e8df529
remove http request url path cleaning logic and deprecate setting
May 17, 2023
966f546
Merge branch 'main' into iot-topic
May 17, 2023
970e5a0
add unit test cases
May 17, 2023
16b4c86
modify changelog description
May 17, 2023
30b3388
modify changelog and remove deprecated setting usage
May 17, 2023
ceeb008
recover some deprecated func
May 17, 2023
2168800
recover some deprecated func
May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changelog/09dc2a7906374b818a70a25a4b238623.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "09dc2a79-0637-4b81-8a70-a25a4b238623",
"type": "bugfix",
"description": "remove url path cleaning logic during http request serialization and deprecate its setting",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use the changelog tool in v1, you have to add an entry to CHANGELOG_PENDING.md.

It should look something like:

* `rest`: Remove unnecessary path normalization behavior.
  * This behavior would incorrectly mutate request paths with URI-encoded characters, potentially resulting in misrouted requests.
* `config`: Deprecate `DisableRestProtocolURICleaning` config setting.
  * This setting no longer has any effect. REST-protocol paths will now never be normalized after serialization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to the SDK Bugs section)

"modules": [
"."
]
}
2 changes: 2 additions & 0 deletions aws/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ type Config struct {
// Bucket: aws.String("bucketname"),
// Key: aws.String("//foo//bar//moo"),
// })
// Deprecated: DisableRestProtocolURICleaning exists for historical compatibility of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this setting is now effectively useless, let's remove everything but the deprecation notice. Let's also reword to clarify the behavior now:
Deprecated: This setting has no longer has any effect. RESTful paths are no longer cleaned after request serialization.

// http request path cleaning setting and should not be used.
DisableRestProtocolURICleaning *bool

// EnableEndpointDiscovery will allow for endpoint discovery on operations that
Expand Down
17 changes: 0 additions & 17 deletions private/protocol/rest/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"math"
"net/http"
"net/url"
"path"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -134,9 +133,6 @@ 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) {
Expand Down Expand Up @@ -244,19 +240,6 @@ 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
Expand Down
56 changes: 47 additions & 9 deletions private/protocol/rest/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,55 @@ import (
"github.com/aws/aws-sdk-go/aws/request"
)

func TestCleanPath(t *testing.T) {
uri := &url.URL{
Path: "//foo//bar",
Scheme: "https",
Host: "host",
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"),
isaiahvita marked this conversation as resolved.
Show resolved Hide resolved
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",
},
}
cleanPath(uri)

expected := "https://host/foo/bar"
if a, e := uri.String(), expected; a != e {
t.Errorf("expect %q URI, got %q", e, a)
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)
}
}
}

Expand Down