Deprecate url path cleaning during http request serialization #4832
Conversation
There was a problem hiding this comment.
This is not the right way to do this:
- You're trying to make service-specific concessions (and keying off of a field name), the protocol serializer should have no notion of these things. Imagine if this were to scale up to special-casing all sorts of different fields, the code would become untenable.
- This isn't actually escaping the value if it has slashes, it's just hiding it from the path normalization logic and injecting it back after the fact.
We need to start over here. The only change we should need to make is to ensure forward slashes are escaped in values that are present in the request path / query.
private/protocol/rest/build_test.go
Outdated
| @@ -20,7 +20,7 @@ func TestCleanPath(t *testing.T) { | |||
| Scheme: "https", | |||
| Host: "host", | |||
| } | |||
| cleanPath(uri) | |||
| cleanPath(uri, "") | |||
There was a problem hiding this comment.
We definitely want a unit test for this behavior (i.e. where the test input has slashes, and we verify they get escaped).
There was a problem hiding this comment.
So it is incorrect to assume that it is safe to alter topic name by removing prefix forward slashes.
I thought I misunderstood how he wants to escape prefix slashes, I'm investigating it in another way
wty-Bryant
changed the title
wty-Bryant
changed the title
aws/config.go
Outdated
| @@ -265,6 +265,8 @@ type Config struct { | |||
| // Bucket: aws.String("bucketname"), | |||
| // Key: aws.String("//foo//bar//moo"), | |||
| // }) | |||
| // Deprecated: DisableRestProtocolURICleaning exists for historical compatibility of | |||
There was a problem hiding this comment.
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.
| { | ||
| "id": "09dc2a79-0637-4b81-8a70-a25a4b238623", | ||
| "type": "bugfix", | ||
| "description": "remove url path cleaning logic during http request serialization and deprecate its setting", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(to the SDK Bugs section)
CHANGELOG_PENDING.md
Outdated
| @@ -3,3 +3,7 @@ | |||
| ### SDK Enhancements | |||
|
|
|||
| ### SDK Bugs | |||
| * `rest`: Remove unnecessary path normalization behavior. | |||
| * This behavior would incorrectly mutate request paths with URI-encoded characters, potentially resulting in misrouted requests. | |||
There was a problem hiding this comment.
Two spaces here to indent instead of 4. IIRC the code that handles this is incredibly picky, best to be safe.
| @@ -497,13 +486,6 @@ func (c *Config) WithLowerCaseHeaderMaps(t bool) *Config { | |||
| return c | |||
| } | |||
|
|
|||
| // WithDisableRestProtocolURICleaning sets a config DisableRestProtocolURICleaning value | |||
| // returning a Config pointer for chaining. | |||
| func (c *Config) WithDisableRestProtocolURICleaning(t bool) *Config { | |||
There was a problem hiding this comment.
We can't remove this (breaking change).
Do please add the same deprecation message as we did on the field itself, though.
There was a problem hiding this comment.
Gotcha, just over-understand deprecate
Remove extra path cleaning logic while serializing http request to preserve path fields containing multiple prefix slashes. Solve #4816