Core, REST: Fix path segment encoding to use RFC 3986 percent-encoding by adutra · Pull Request #15989 · apache/iceberg

adutra · 2026-04-15T19:15:23Z

This PR introduces new methods in RESTUtil:

encodePathSegment/decodePathSegment
encodeNamespaceAsPathSegment/decodeNamespaceAsPathSegment

They use RFC 3986 percent-encoding (spaces as %20) instead of application/x-www-form-urlencoded (spaces as +).

This PR also switches ResourcePaths to use the new path-segment methods, thus fixing the encoding issue on the client (encoding) side.

The decoding methods are free to be used on the server side; servers should switch to them when they can/want to support the new, correct encoding. The REST TCK harness has been updated already.

ML discussion: https://lists.apache.org/thread/c498svln0x18vvm42998b9nm9j6ck5yh
Fixes #17759.

adutra · 2026-04-16T13:11:37Z

A few data points:

In the range of 0 to 32767, there are only 3 characters that encodeString and encodePathSegment encode differently:

Character	Code	`encodeString`	`encodePathSegment`
	32	`+`	`%20`
`*`	42	`*`	`%2A`
`~`	126	`%7E`	`~`

In particular the + sign is encoded equally:

Character	Code	`encodeString`	`encodePathSegment`
`+`	43	`%2B`	`%2B`

And there is only one character that fails a crossed round trip, the space char (32):

Character	Code	Round Trip	Result
	32	`decodePathSegment(encodeString(' '))`	`+`

This shows evidence that:

There is no behavioral regression when a new client (encodePathSegment) talks to an old server (decodeString).
There is a behavioral regression when an old client (encodeString) talks to a new server (decodePathSegment): any space will be interpreted by the server as the + sign.

That's why it's safe to upgrade clients right away, but servers should only upgrade when they feel it's safe to do so.

github-actions · 2026-05-17T00:42:09Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

adutra · 2026-05-18T11:29:24Z

cc @c-thiel – I think this PR should not be closed.

nastra · 2026-06-17T10:12:21Z

    String expected = "+%25%26%2B%C2%A3%E2%82%AC";

    assertThat(RESTUtil.encodeString(utf8)).isEqualTo(expected);
  }


can we add some tests here to make it more explicit how old/new client/server behavior is defined? For example, an old client (encodeString) talking to a new server (decodePathSegment). The same would apply for encodeNamespace/decodeNamespace / encodeNamespaceAsPathSegment/decodeNamespaceAsPathSegment. This makes it easier to reason about what the actual behavior is in these scenarios and can be named similar to encodeAsOldClientAndDecodeAsNewServer()

Added two more tests:

encodePathAsOldAndNewClientDecodeAsOldServer

encodePathAsOldAndNewClientDecodeAsNewServer

I tried to highlight the fact that there is no one-size-fits-all solution for the decoding side.

nastra · 2026-06-17T10:23:13Z

There is a behavioral regression when an old client (encodeString) talks to a new server (decodePathSegment): any space will be interpreted by the server as the + sign.

@adutra couldn't we fix this by keeping decodeString everywhere? It's been a while since I looked into this, but I believe URLDecoder.decode is already lenient and can handle + and %20 properly. Alternatively we might want to explore PercentCodec.decode(content, charset, true) with plusAsBlank=true

nastra · 2026-06-17T10:24:03Z

+   * Decodes a URL path segment per RFC 3986. Unlike {@link #decodeString(String)}, this method does
+   * <b>not</b> treat {@code +} as a space — it is left as a literal {@code +} character.
+   *
+   * <p>Note: this method is introduced in this release but is not yet wired into server-side


I think we should aim for a solution that we can wire everywhere and that doesn't introduce a behavioral regression

adutra · 2026-06-18T11:50:34Z

@adutra couldn't we fix this by keeping decodeString everywhere? It's been a while since I looked into this, but I believe URLDecoder.decode is already lenient and can handle + and %20 properly.

Yes, keeping decodeString for decoding path segments encoded with encodePathSegment would work. This is the "newer client with older server" use case and the reason why upgrading clients is a safe move.

This is also why decodePathSegment (and decodeNamespaceAsPathSegment) are currently "unplugged".

Are you implying that you would rather have decodePathSegment (and decodeNamespaceAsPathSegment) completely removed?

adutra · 2026-06-18T12:14:37Z

@nastra also the problem is broader ecosystem compatibility.

Using decodeString to decode encodePathSegment only works because in Java, encodePathSegment would encode + as %20.

But RFC 3986 does not require this sign to be percent-encoded. Other clients could send + unencoded; and decodeString would wrongly decode that as a space, not a plus sign.

For a general RFC 3986-compliant server that may receive requests from any client written in any language, decodeString is incorrect for path segments; decodePathSegment is the right decoder.

adutra · 2026-06-18T12:47:54Z

I did a small survey over PyIceberg, Iceberg Rust and Iceberg Go:

Language	`+` encoded as	Ok to decode with `decodeString`?
PyIceberg	`%2B`	✅
Iceberg Rust	`+`	🛑
Iceberg Go	`+`	🛑

github-actions · 2026-07-19T00:32:54Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

adutra · 2026-07-20T08:27:45Z

Not stale.

ajantha-bhat · 2026-08-18T13:37:27Z

hi @nastra, Do we need anything else for this PR? Can you please take another look?

This PR introduces new methods in `RESTUtil`: - `encodePathSegment`/`decodePathSegment` - `encodeNamespaceAsPathSegment`/`decodeNamespaceAsPathSegment` They use RFC 3986 percent-encoding (spaces as `%20`) instead of `application/x-www-form-urlencoded` (spaces as `+`). This PR also switches `ResourcePaths` to use the new path-segment methods, thus fixing the encoding issue on the client (encoding) side. The decoding methods are free to be used on the server side; servers should switch to them when they can/want to support the new, correct encoding. The REST TCK harness has been updated already.

adutra · 2026-08-21T14:44:46Z

FYI I rebased to fix conflicts with #17697.

uros-b

cc @aboueleyes, regarding #17769 (review)

aboueleyes · 2026-08-23T12:20:59Z

Thanks @uros-b

Cross-linking from #17769, which we are closing in favor of this PR as part of consolidation.

Three small things from that PR that this one does not seem to cover, offered in case they are useful:

TestHTTPRequest: an added case asserting percent-escaped segments survive request-URI construction unchanged (v1/main%7Cwarehouse/namespaces/sales%20report/... round-trips the escapes byte-for-byte through hc5's URIBuilder).
Endpoint coverage in TestResourcePaths beyond table(): metrics, remoteSign, view,
planTableScan, plan (including a plan ID holding both a space and a literal plus), fetchScanTasks, and the namespace-only routes (namespace, namespaceProperties, tables, register, views, registerView) with a spaced namespace.
Issue REST client uses form encoding for URL path segments #17759 as a fixes-reference: it documents the live Nessie reproduction (sales%20report -> 200 vs sales+report -> 404) that motivates the client-side change.

Happy to provide any of these as a patch on top of your branch if easier than hand-porting

adutra · 2026-08-24T08:41:52Z

Thanks @aboueleyes I incorporated your extra tests.

I also linked your issue #17759 - although the issue was first reported on the dev ML: https://lists.apache.org/thread/c498svln0x18vvm42998b9nm9j6ck5yh.

RussellSpitzer · 2026-09-01T16:20:15Z

+   * Decodes a URL path segment per RFC 3986. Unlike {@link #decodeString(String)}, this method does
+   * <b>not</b> treat {@code +} as a space — it is left as a literal {@code +} character.
+   *
+   * <p>Note: this method is introduced in this release but is not yet wired into server-side


Different note, This doc note doesn't belong here. It just describes PR planning

Agreed, removed the whole note.

RussellSpitzer · 2026-09-01T16:43:05Z

So if I get this correctly, we are doing this fix to make sure that path's are encoded with %20 and not +. (There are 2 other characters I see we are covering but they are also ... very unlikely to really be used)

The worry is that we may have clients sending "+" that mean "+" and will get decoded differently on server side if the server changes it's decoder? Is that right @nastra ?

Honestly the surface area is so small I'm not that worried about broken folks. From the table @adutra posted we are already broken if you are using multiple implementations with your catalog correct? The Python version and the Java client are currently in agreement as are Go and Rust. The fact that this is coming up so rarely does make me think we should just go compliant with the RFC in Java and convince the Python impl to do the same.

Does this fix come from a real user who broke on this? I'm just wondering what kind of names they were using

RussellSpitzer · 2026-09-01T17:04:23Z

    assertThat(withoutPrefix.namespaceProperties(ns)).isEqualTo("v1/namespaces/n%1Fs/properties");
  }

+  @Test


This PR is already pretty large, do we need a separate test for both " " and "+" for every endpoint? Couldn't we just do "foo +bar" and cover everything at once? We have coverage for the individual correctness in the utility, here we just want to make sure the right encoder was used.

I just imported these tests from @aboueleyes PR. I think the whole test class could be reduced by 60% at least if we adopt parameterized tests instead of the tests we have today. However I would do that as a follow-up task. Wdyt?

adutra · 2026-09-02T15:45:37Z

Does this fix come from a real user who broke on this? I'm just wondering what kind of names they were using

It came initially from a report by @c-thiel on the dev ML. I think that @aboueleyes also posted another real-life report in #17759.

RussellSpitzer · 2026-09-02T16:16:44Z

Does this fix come from a real user who broke on this? I'm just wondering what kind of names they were using

It came initially from a report by @c-thiel on the dev ML. I think that @aboueleyes also posted another real-life report in #17759.

17759 just looks like an AI issue :) But I trust @c-thiel if he actually saw this in the wild.

I do want to make sure @nastra gets a chance to close on this too before we merge

nastra

LGTM but I think we need to highlight it explicitly in the PR/release notes that we're breaking old java clients that would send a + to mean a space

nssalian · 2026-09-10T16:06:04Z

Thanks @nastra. I'll keep in mind for the release notes

github-actions Bot added the core label Apr 15, 2026

adutra mentioned this pull request Apr 16, 2026

Isolate encode/decode logic used in Polaris entities apache/polaris#4214

Merged

6 tasks

adutra mentioned this pull request Apr 22, 2026

Parse Iceberg client language and version from request headers apache/polaris#4236

Closed

6 tasks

github-actions Bot added the stale label May 17, 2026

github-actions Bot removed the stale label May 19, 2026

nastra self-requested a review June 10, 2026 16:22

c-thiel approved these changes Jun 10, 2026

View reviewed changes

nastra reviewed Jun 17, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java

nastra reviewed Jun 17, 2026

View reviewed changes

adutra force-pushed the fix-path-segment-encoding branch from bb92658 to 595a7ea Compare June 18, 2026 13:10

github-actions Bot added the stale label Jul 19, 2026

github-actions Bot removed the stale label Jul 21, 2026

ajantha-bhat added this to the Iceberg 1.12.0 milestone Aug 6, 2026

adutra added 3 commits August 21, 2026 16:38

fix remoteSign()

dfa4321

add more tests

9e596d4

adutra force-pushed the fix-path-segment-encoding branch from 595a7ea to 9e596d4 Compare August 21, 2026 14:38

uros-b mentioned this pull request Aug 23, 2026

Core: Fix REST path encoding to use %20 instead of + #17769

Closed

uros-b reviewed Aug 23, 2026

View reviewed changes

uros-b mentioned this pull request Aug 23, 2026

Fix #17759: REST client uses form encoding for URL path segments #17766

Open

add more tests

2019537

nssalian requested review from nastra, singhpk234 and uros-b and removed request for uros-b August 30, 2026 00:51

RussellSpitzer reviewed Sep 1, 2026

View reviewed changes

review

da9eefa

RussellSpitzer reviewed Sep 1, 2026

View reviewed changes

RussellSpitzer approved these changes Sep 1, 2026

View reviewed changes

nastra approved these changes Sep 8, 2026

View reviewed changes

Uh oh!

Conversation

adutra commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Apr 16, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

adutra commented May 18, 2026

Uh oh!

Uh oh!

nastra Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

adutra Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

nastra commented Jun 17, 2026

Uh oh!

nastra Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

adutra commented Jun 18, 2026

Uh oh!

adutra commented Jun 18, 2026

Uh oh!

adutra commented Jun 18, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

adutra commented Jul 20, 2026

Uh oh!

ajantha-bhat commented Aug 18, 2026

Uh oh!

adutra commented Aug 21, 2026

Uh oh!

uros-b left a comment

Choose a reason for hiding this comment

Uh oh!

aboueleyes commented Aug 23, 2026

Uh oh!

adutra commented Aug 24, 2026

Uh oh!

RussellSpitzer Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

adutra Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Sep 1, 2026

Uh oh!

RussellSpitzer Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

adutra Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

adutra commented Sep 2, 2026

Uh oh!

RussellSpitzer commented Sep 2, 2026

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

nssalian commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adutra commented Apr 15, 2026 •

edited

Loading