Skip to content

Support overlapping paths on resource classes #47386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Postremus
Copy link
Member

@Postremus Postremus commented Apr 15, 2025

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.

@Postremus Postremus requested a review from geoand April 15, 2025 18:31

This comment has been minimized.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Very clever, well done!

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

@geoand
Copy link
Contributor

geoand commented Apr 16, 2025

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

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 16, 2025
@franz1981
Copy link
Contributor

I have added few comments

This comment has been minimized.

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 17, 2025
@franz1981
Copy link
Contributor

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

@geoand
Copy link
Contributor

geoand commented Apr 17, 2025

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

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Apr 18, 2025

@franz1981 PTAL

@franz1981
Copy link
Contributor

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

This comment has been minimized.

@Postremus
Copy link
Member Author

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
Copy link
Member Author

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.

Copy link

quarkus-bot bot commented Apr 20, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5d81443.

✅ 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 geoand requested a review from franz1981 April 21, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matching a base path of another controller, results in 405 Method Not Allowed
3 participants