Support overlapping paths on resource classes

[Postremus]

Having partially matching paths on resource classes could lead to 404s, even though a matching resource method existed.

Lets take the example from #26496.

@Path("/base")
class Base {
   @GET
   @Path("{id}") 
   public Uni<RestResponse<?>> base() {..}
}
@Path("/base/{id}")
class Extension {
   @GET
   @Path("extension") 
   public Uni<RestResponse<?>> extension() {..}
}

Calling GET /base/123 would result in a 404. /base/{id} is a perfect match for that request, which result in the Extension resource class being used.
Quarkus-rest always only works on the basis of one resource class - the one with the best match.

I however believe that is not up to spec.

https://jakarta.ee/specifications/restful-ws/4.0/jakarta-restful-ws-spec-4.0.pdf

Chapter 3.5.2 Request Matching
Stage 1 only handles matching of the resource class path to the request uri -> both classes match.
Stage 2 figures out which of all resource methods in all matched resource classes matches against the request uri.

So basically, the current implementation results in Stage 1 only reporting one matching resource class, altough it should report 2.

This PR adds additional logic, so that all matching resource classes are remembered, and the handler chain is restarted if the current resource class does not contain a matchin sub resource method, or sub resource locator.

• Closes: Matching a base path of another controller, results in 405 Method Not Allowed #26496

[geoand]

Very clever, well done!

@FroMage do you also want to take a look at this?

[geoand]

cc @franz1981 as this one could potentially have a slight performance impact

[franz1981]

I have added few comments

[franz1981]

Please @geoand wait to merge this till the comments are addressed or at least a round of profiling is performed 🙏

[geoand]

That's why I removed the waiting-for-ci label :)

[geoand]

@franz1981 PTAL

[franz1981]

I would suggest to just return the match without allocating any list, but just the top match 🙏
Sorry for not commenting but I'm on PTO till next week (Fri) so I can comment with phone :P

[Postremus]

Sorry for not commenting but I'm on PTO till next week (Fri) so I can comment with phone :P

@franz1981 no worries, I don't mind the back and forth as long as its not too nitpicky :)

[Postremus]

I would suggest to just return the match without allocating any list, but just the top match

I now return an Iterator, which allows to try the next match.

[gsmet]

I had a quick look at the patch and I have a small question: is the very common case where there is only one match fully optimized?

I think we should try to reduce allocations and logic to the minimum in this case.

Maybe it already is but it wasn't obvious so sorry if it's the case.

[Postremus]

drafting this PR, I might be able to simplify more in the common case (where only one path matches).

[gsmet]

BTW, I'm not expecting things to go overcomplicated for very minor gains. I.e. maybe keeping the iterator makes sense but making sure that we don't allocate additional things given there's only one item.

I will let you judge of finding the best compromise between readability and performance impact.

[Postremus]

@gsmet @franz1981
Next attempt. This time no additional allocations in the common path; simpler diff.

[FroMage]

Very clever, well done!

@FroMage do you also want to take a look at this?

I looked. I don't understand it anymore than the original implementation.

But the test is valid, the improvement is there, and if it doesn't break anything, it's progress, so +1 from me 🤷

[geoand]

@franz1981 any final comments?

[franz1981]

Will take a look tomorrow morning 🙏

[geoand]

👌

[Postremus]

@franz1981 Sorry for the ping, but it has been a few days since the last update. And I kind of want to get this off my plate :)

[geoand]

@franz1981 is on PTO, so if he doesn't respond this week (which I fully expect him not to), I will just go ahead and merge this

[geoand]

FYI: I rebased the PR onto main

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6b67e05.

✅ 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.

Comments addressed