Handle trailing slashes for endpoints with regex path params #43016

jonomorris · 2024-09-04T13:29:16Z

For Reactive REST endpoints don't include the path end slash when performing regex path param matches.

Fixes: Reactive REST endpoints do not handle trailing slashes for paths w/ regex #42687

gsmet · 2024-09-05T08:37:39Z

@jonomorris Thanks for the PR! I think the test failures above look related ^

jonomorris · 2024-09-05T13:23:56Z

gsmet yeah there are some problems, I'll look into it.

jonomorris · 2024-09-06T21:53:08Z

@gsmet and @stuartwdouglas, I made a small update and the unit tests pass locally in independent-projects/resteasy-reactive and extensions/resteasy-reactive.

geoand · 2024-09-24T09:12:14Z

cc @FroMage

FroMage · 2024-09-24T14:10:42Z

...ertx/src/test/java/org/jboss/resteasy/reactive/server/vertx/test/matching/RegexPathTest.java

+                .statusCode(200)
+                .body(equalTo("Hello! 1"));
+
+        get("/hello/1/")


What happens if you do get /hello/1////?

Intermediate slashes are encoded, for instance the path becomes /1%2F%2F%2F%2F/, which doesn't match the regex path matcher \\d+.

So, we only alow a single optional slash at the end, then?

Yes, that's correct.

I'm not sure how I feel about this.
It feels special-casy.

Also, I looked for articles about tailing slashes, and I only find SEO articles that advise to only serve one variant (with, or without).

And last, I've come across this issue spring-projects/spring-framework#28552 where Sprint went the other way around and removed the default of allowing optional trailing slashes by default.

So, I'll ask for more comments from HTTP/REST colleagues who might have a relevant opinion: @cescoffier @vietj:

Do you think we should allow trailing slashes for routes defined without one?

Now, on the other hand, this issue is about fixing the particular difference of assuming a trailing slash for regex paths, so that it's coherent with normal paths, which already assume a trailing slash. And I agree both should be the same. I'm not entirely sure regex matches are right, though, I wonder if non-regex path matching might be in the wrong here and the one that needs fixing. WDYT @geoand ?

/foo and /foo/ are not equivalent. They refer to different resources due to the trailing slash:

/foo refers to a resource.

/foo/ often suggests that it’s a collection or directory-like structure containing resources.

So @cescoffier you're saying we should change the current behaviour of Quarkus REST to not treat these as the same when routing?

I suppose making the change and running the TCK will tell us what JAX-RS thinks about that :)

That's the problem - many servers consider them equivalent, but the "theory" says they are not.

There was a similar conversation here #26016 (comment)

Well, OK, so the TCK did not test this, and the spec says we have to treat them equivalent. Probably people rely on that now, so let's just harmonize and make sure regexes also behave in a similar way, and if we want to change the default later, we'll wait for users reporting an issue and revisit if required, for both regular and regex.

FroMage · 2024-09-24T14:11:24Z

...e/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/mapping/RequestMapper.java

@@ -76,12 +76,18 @@ private RequestMatch<T> mapFromPathMatcher(String path, PathMatcher.PathMatch<Ar
            for (int i = 1; i < potentialMatch.template.components.length; ++i) {
                URITemplate.TemplateComponent segment = potentialMatch.template.components[i];
                if (segment.type == URITemplate.Type.CUSTOM_REGEX) {
-                    Matcher matcher = segment.pattern.matcher(path);
+                    // exclude any path end slash when matching a subdir, but include it in the matched length
+                    boolean endSlash = matchPos < path.length() && path.charAt(path.length() - 1) == '/';


Perhaps we need to consider more than one slash at the end? See my next question for how this behaves for non-regexes.

FroMage · 2024-10-02T11:16:11Z

Agreed @cescoffier @geoand ?

jorsol · 2024-11-06T13:26:34Z

Will this fix match more complex regex like in #44242?

Edit: Just checked the patch and it works as expected. Thanks!

jorsol · 2024-11-06T15:43:28Z

Can this be backported to 3.15 too? 🙏🏽

geoand · 2024-11-06T15:53:34Z

~~New features do not get backported to LTS releases~~ Never mind, I thought this was something else. Let's first get it merged first, and then we'll see about backporting

geoand · 2024-11-06T16:02:07Z

Forgot about this one, sorry.

Can the PR be rebased on main so we can get a recent CI run?

quarkus-bot · 2024-11-07T17:08:42Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 6687b4b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

geoand · 2024-11-07T17:15:48Z

@FroMage mind taking a last peak to make sure nothing changed?

FroMage

LGTM, let's see how this pans out

jorsol · 2024-12-05T12:52:28Z

Can this have label:triage/backport-3.15 ?

geoand · 2024-12-05T13:05:47Z

Perhaps down the line when this patch has been stress tested by later non LTS versions

geoand requested a review from stuartwdouglas September 4, 2024 14:19

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Sep 7, 2024

FroMage reviewed Sep 24, 2024

View reviewed changes

FroMage approved these changes Oct 2, 2024

View reviewed changes

jorsol approved these changes Nov 6, 2024

View reviewed changes

jonomorris added 3 commits November 7, 2024 23:18

quarkusio#42687 handle trailing slash

c955f0e

quarkusio#42687 remove comment

f653a52

quarkusio#42687 ignore end-slash when matching path subdir

6687b4b

jonomorris force-pushed the fix-42687_handle_trailing_slashes branch from c53f4f0 to 6687b4b Compare November 7, 2024 10:21

FroMage approved these changes Nov 12, 2024

View reviewed changes

FroMage merged commit be016b7 into quarkusio:main Nov 12, 2024
46 checks passed

quarkus-bot bot added this to the 3.17 - main milestone Nov 12, 2024

quarkus-bot bot added the kind/bugfix label Nov 12, 2024

jonomorris deleted the fix-42687_handle_trailing_slashes branch November 12, 2024 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle trailing slashes for endpoints with regex path params #43016

Handle trailing slashes for endpoints with regex path params #43016

jonomorris commented Sep 4, 2024 •

edited by geoand

Loading

This comment has been minimized.

gsmet commented Sep 5, 2024

jonomorris commented Sep 5, 2024

jonomorris commented Sep 6, 2024

This comment has been minimized.

geoand commented Sep 24, 2024

FroMage Sep 24, 2024

jonomorris Sep 30, 2024

FroMage Sep 30, 2024

jonomorris Oct 1, 2024

FroMage Oct 1, 2024

cescoffier Oct 1, 2024

FroMage Oct 1, 2024

cescoffier Oct 1, 2024

jonomorris Oct 2, 2024

FroMage Oct 2, 2024

FroMage Sep 24, 2024

FroMage commented Oct 2, 2024

jorsol commented Nov 6, 2024 •

edited

Loading

jorsol commented Nov 6, 2024

geoand commented Nov 6, 2024 •

edited

Loading

geoand commented Nov 6, 2024

quarkus-bot bot commented Nov 7, 2024 •

edited by github-actions bot

Loading

geoand commented Nov 7, 2024

FroMage left a comment

jorsol commented Dec 5, 2024

geoand commented Dec 5, 2024

Handle trailing slashes for endpoints with regex path params #43016

Handle trailing slashes for endpoints with regex path params #43016

Conversation

jonomorris commented Sep 4, 2024 • edited by geoand Loading

This comment has been minimized.

gsmet commented Sep 5, 2024

jonomorris commented Sep 5, 2024

jonomorris commented Sep 6, 2024

This comment has been minimized.

geoand commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FroMage commented Oct 2, 2024

jorsol commented Nov 6, 2024 • edited Loading

jorsol commented Nov 6, 2024

geoand commented Nov 6, 2024 • edited Loading

geoand commented Nov 6, 2024

quarkus-bot bot commented Nov 7, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

geoand commented Nov 7, 2024

FroMage left a comment

Choose a reason for hiding this comment

jorsol commented Dec 5, 2024

geoand commented Dec 5, 2024

jonomorris commented Sep 4, 2024 •

edited by geoand

Loading

jorsol commented Nov 6, 2024 •

edited

Loading

geoand commented Nov 6, 2024 •

edited

Loading

quarkus-bot bot commented Nov 7, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`