You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aboueleyes (via GitHub)" <gi...@apache.org> on 2026/08/22 12:06:40 UTC
[PR] Core: Fix REST path encoding to use %20 instead of + [iceberg]
aboueleyes opened a new pull request, #17769:
URL: https://github.com/apache/iceberg/pull/17769
The Java REST client form-encodes URL path segments.
`RESTUtil.encodeString` uses `java.net.URLEncoder`, which implements `application/x-www-form-urlencoded` and represents a space as `+`. `ResourcePaths` used it when building paths, so any namespace level, table or view name, or scan plan ID containing a space was addressed with a literal `+` in the path.
Catalogs that decode path segments per RFC 3986 cannot match such names: Nessie answers 404 for `namespaces/sales+report` while `namespaces/sales%20report` returns 200, so listing such a namespace fails with `NoSuchNamespaceException`, catalogs that answer 200 for unknown namespaces instead report it as empty, and loading a table in it fails with a not-found error naming `sales+report.q1+results`, a name the user never typed.
This adds `RESTUtil.encodePathSegment`, which percent-encodes a space as `%20` while keeping every other `URLEncoder` property: a literal `+` is escaped to `%2B`, `/` stays `%2F`. It uses the new encoder for all path construction: namespace levels in `encodeNamespace` and the table, metrics, signing, planning, and task segments built by `ResourcePaths`. `encodeFormData` continues to use `application/x-www-form-urlencoded`, which is correct for OAuth token requests and leaves them unchanged.
Decoding needs no change: `URLDecoder` reads both `%20` and `+`, so values produced by older clients still round-trip through `decodeString` and `decodeNamespace`. That leniency is deliberate, mirroring how servers already accept both advertised namespace separators per the spec's own compatibility rule.
Compatibility for existing servers: literal `+` names encode identically before and after
(`a+b` becomes `a%2Bb` either way), and any catalog that works today already percent-decodes path segments correctly because the old encoder emitted `%2F`, `%26` and non-ASCII escapes on the same paths.
A prior discussion concluded `encodeString` was acceptable for plan IDs (#13400); that conclusion predates the reproductions below and is reversed here.
Tested with new cases in `TestRESTUtil`, `TestResourcePaths` and `TestHTTPRequest` covering table, metrics, sign, view, plan and task paths plus legacy decode round-trips; each new ResourcePaths assertion fails against unpatched main, and the existing OAuth form-encoding tests are untouched and still pass.
Fixes #17759
Related: #12308, #14263, review discussion in #15948
---
**AI Disclosure**
- Model: ox-alpha
- Platform/Tool: opencode CLI
- Human Oversight: fully reviewed
- Prompt Summary: Drafted the fix and tests for issue #17759 following the review guidance from
#15948: add a path-segment encoder, use it across ResourcePaths and encodeNamespace, keep OAuth form encoding, add regression tests including legacy decoding.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix REST path encoding to use %20 instead of + [iceberg]
Posted by "aboueleyes (via GitHub)" <gi...@apache.org>.
aboueleyes commented on PR #17769:
URL: https://github.com/apache/iceberg/pull/17769#issuecomment-5385961121
Yes, happy to consolidate.
@adutra's #15989 predates this PR, already carries approvals and review, and its RFC 3986 approach via `PercentCodec` is stricter than the `URLEncoder`-based substitution here.
This PR has three small additions that #15989 does not appear to cover; I will offer them over there:
1. A `TestHTTPRequest` case pinning that percent-escaped segments (`%20`, `%7C`) pass through
request-URI construction unmodified.
2. Per-endpoint path coverage with spaces and literal pluses across metrics, remote-signing,
view, plan, plan-ID and scan-task builders, plus the six namespace-only routes.
3. Cross-linking issue #17759 so the Nessie reproduction stays attached to the fix that lands.
Closing this in favor of #15989. Thanks @uros-b for flagging and @adutra for the cleaner implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix REST path encoding to use %20 instead of + [iceberg]
Posted by "aboueleyes (via GitHub)" <gi...@apache.org>.
aboueleyes closed pull request #17769: Core: Fix REST path encoding to use %20 instead of +
URL: https://github.com/apache/iceberg/pull/17769
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org