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.

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

Fix: URI path element replace issue due to elements name overlap #553

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

wty-Bryant
Copy link
Contributor

@wty-Bryant wty-Bryant commented Nov 13, 2024

Issue #, if available:
fixes #552

Description of changes:
Old serialization code only indexes first occurrence of target element with left bracket (e.g. {key) without considering common prefix cases like {key1111} and {key}. this pr fixes that by directly searching index of the whole element with the format of {element} or {element+}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Madrigal
Copy link
Contributor

Old serialization code only indexes first occurrence of target element via prefix without considering duplication cases,

true, but to expand on it. The old code only looked for {key instead of {key} (unsure if optimization or off-by-one error on original implementation). Because of this, {name and {namespace} both matched {name}

@wty-Bryant wty-Bryant marked this pull request as ready for review November 13, 2024 22:16
@wty-Bryant wty-Bryant requested review from a team as code owners November 13, 2024 22:16
Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

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

Can we add an error test case? e.g. path is /{bucket}/{key+}, try to replace key buck with some value, expect error

encoding/httpbinding/path_replace.go Outdated Show resolved Hide resolved
@wty-Bryant wty-Bryant merged commit 84c6c7e into main Nov 14, 2024
11 checks passed
@wty-Bryant wty-Bryant deleted the fix-path-replacing branch November 14, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error replacing URI path elements when element names overlap
3 participants