Skip to content

URIBuilder: do not produce '//' when appending to a path that ends with '/' - #745

Open
PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:uribuilder-append-trailing-slash
Open

PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:uribuilder-append-trailing-slash

Conversation

@PDGGK

@PDGGK PDGGK commented Nov 11, 2026

Copy link
Copy Markdown

A path that ends with / has an empty last segment, and appendPath and appendPathSegments add the new segments after it. So a base URI with a trailing slash, which is how many people write one, picks up a double slash:

new URIBuilder("http://localhost/").appendPath("api").build();          // http://localhost//api
new URIBuilder("http://localhost/api/").appendPath("items/42").build(); // http://localhost/api//items/42

The classic client sends these as GET //api and GET /api//items/42. Against an HttpCore classic server with handlers registered for /api and /api/items/*, the first request gets 421 Not authoritative and the second gets 501. With this change both get 200.

The Jakarta REST client in HttpClient runs into the same thing, because RestInvocationHandler adds the resource path to baseUri with appendPathSegments. With baseUri("http://127.0.0.1:<port>/api/") and a resource at items/{id}, the server receives GET /api//items/42, and with baseUri("http://127.0.0.1:<port>/") it receives GET //items/42. Against a core built from this branch it receives /api/items/42 and /items/42. ResourceIfaceScanner#join already drops a trailing empty segment when it joins the class and method paths, so this change brings URIBuilder in line with it.

appendPathSegments now replaces an empty last segment with the appended segments. A trailing slash on the appended value is kept (/api/ plus /v1/ gives /api/v1/, and /api/ plus / stays /api/). Appending null or an empty list leaves the path alone, as before.

Tests: testAppendToPathWithTrailingSlash and testAppendSegmentsToPathWithTrailingSlash in TestURIBuilder. Without the change they fail with expected: <https://somehost/api> but was: <https://somehost//api> and expected: <http://myhost/api/v3/products> but was: <http://myhost//api/v3/products>. With it, all 82 tests in TestURIBuilder and the 141 tests in org.apache.hc.core5.net pass, and checkstyle passes. I also ran the 84 tests of httpclient5-jakarta-rest-client (HttpClient master) against a core built from this branch, and they pass.

All of this ran on JDK 17 only. I did not run the full HttpCore test suite.

…th '/'

A path that ends with '/' has an empty last segment. appendPath and
appendPathSegments added the new segments after it, so
new URIBuilder("http://host/api/").appendPath("items") built
http://host/api//items, and a base of http://host/ built
http://host//items.

The empty last segment is now replaced by the appended segments.

@arturobernalg arturobernalg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some concerns about changing the semantics of appendPathSegments().

RFC 3986 permits empty path segments, and consecutive slashes are not necessarily redundant. The proposed change silently removes an existing empty segment, potentially altering the intended URI.

I think path segment manipulation and path joining should remain separate concerns. Would it make more sense to handle this in the REST client rather than change the behavior of URIBuilder?

@ok2c

ok2c commented Nov 11, 2026

Copy link
Copy Markdown
Member

This fix clearly belongs to the Jakarta REST client, not core.

@arturobernalg

Copy link
Copy Markdown
Member

This fix clearly belongs to the Jakarta REST client, not core.

Agreed with @ok2c

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.

3 participants