-
Notifications
You must be signed in to change notification settings - Fork 464
fix: UriTemplate expansion reserved ("+") and fragment("#") should not encode already percent encoded parts #2108
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
Conversation
Hi @suztomo, since you've reviewed our previous contribution. |
@ldetmer maintains this repository these days. |
Hi @ldetmer, It aims to improve the library’s compliance with RFC 6570. Let me know if there’s anything I should adjust or clarify before it can be considered. Thanks a lot! |
Hi @ldetmer, Just wanted to follow up on the RFC 6570 compliance. I know you're probably busy, so no rush at all! I just wanted to make sure it didn't get lost in the shuffle. If you need any additional context or have questions about the changes, I'm happy to provide more details. As I mentioned before we'd love to contribute towards making the library more compliant with the RFC. Also, if there are any preliminary concerns you'd like me to address first, just let me know. Thanks again for your time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, given the following observations I'll leave here for other reviewers:
- new behavior indeed does not affect existing one
- confirmed compliance with the specification
However, support for {#var}
and {+var}
belong to level 2 templates, while we state we support "Level 1 templates and Level 4 composite templates" in
google-http-java-client/google-http-client/src/main/java/com/google/api/client/http/UriTemplate.java
Line 32 in 5d0689a
* <p>This Class supports Level 1 templates and all Level 4 composite templates as described in: <a |
Would you mind clarifying this in that comment? Also, it could be useful to update #2107 as a feature request to support Level 2 templates.
google-http-client/src/test/java/com/google/api/client/http/UriTemplateTest.java
Show resolved
Hide resolved
google-http-client/src/main/java/com/google/api/client/util/escape/PercentEncodedEscaper.java
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [2.0.1](https://togithub.com/googleapis/google-http-java-client/compare/v2.0.0...v2.0.1) (2025-09-24) ### Bug Fixes * UriTemplate expansion reserved ("+") and fragment("#") should not encode already percent encoded parts ([#2108](https://togithub.com/googleapis/google-http-java-client/issues/2108)) ([30766a8](https://togithub.com/googleapis/google-http-java-client/commit/30766a8a74df49c37e80ec41f1021d4ad69a8fda)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Currently the
UriTemplate::expand
method re-encodes already percent encoded triplets when using reserved("+") and fragment ("#") expansions, which should not be the case according to the RFC 6570.The fix adds a new Encoder(
PercentEncodedEscaper
) that escapes already percent encoded parts of the input. It takes in the constructor another Escaper, allowing extensions of already existing Escapers, without changing/breaking any existing implementations.Fixes #2107 ☕️