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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
