Refine how resource classes with same components are merged

[Postremus]

Fixes #25462

Resources classes where merged by using all components as map key. This leads to situations where the only differing property of a component may be its name. The name is for a DEFAULT_REGEX however not enough to be considered a different path itself.

The merge is now based on a custom (String) key, build on properties which should enforce uniqueness when needed: type, literal, and pattern.

[geoand]

Thanks for looking into this @Postremus!

While working on this, please make sure that the JAX-RS TCK is passing with this change

[Postremus]

@geoand Where are the tcks located for resteasy-reactive? Is it this module? https://github.com/quarkusio/quarkus/tree/main/tcks/resteasy-reactive

[geoand]

Nope, it's this one: https://github.com/quarkusio/resteasy-reactive-testsuite. Just run mvn install on that module once you've built your change in Quarkus.

[Postremus]

@geoand TCKs are passing.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Failing Jobs - Building 864b184755d078db0a0484ed3247e836c1c9cac0

Status	Name	Step	Failures	Logs	Raw logs
:heavy_check_mark:	JVM Tests - JDK 11
✖	JVM Tests - JDK 17	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

:gear: JVM Tests - JDK 17 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 3 more

:package: extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

[Postremus]

Test failures do not look related. The kafka dev service container could not be started.

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

[Postremus]

I don't see how resteasy.wider.request.matching is related to this issue.

This flag was first mentioned, as far as I found, in the migration guide for 3.0-beta-5 to 3.0-beta-6: https://docs.jboss.org/resteasy/docs/3.0.6.Final/userguide/html/Migration_from_older_versions.html

[..] For example, resource classes are scanned for a best match, other are ignored in the match. [..] There is one config switch I added so that Resteasy will ignore the Spec defined class expression filtering step and instead match base on the full expressions of each JAX-RS method. resteasy.wider.request.matching. [..]

I found these integration tests in resteasy:

• https://github.com/resteasy/resteasy/blob/main/testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/resource/basic/WiderMappingTest.java
• https://github.com/resteasy/resteasy/blob/main/testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/resource/basic/WiderMappingNegativeTest.java

The test consisist of:

• one resource class with path "/hello"
• one resource class with path "{x:.*}"

In WiderMappingTest (resteasy.wider.request.matching=true), {x:.*} must match, while "/hello" must match in the negative test.

Also, as far as I can tell, the handling of the flag resteasy.wider.request.matching is done in ResourceMethodRegistry. In each method of this class, widerMatching is checked. if true, do operations on rootNode (all endpoints), otherwise operate on root (only resource classes).

This flag only changes the order of how resources with different paths are merged, but has no influence in how resources with the same paths are matched.

I therefore believe that this fix is still correct, and no config switch is needed. We merge resources with the same path (components) on class level. No change is done to the specific resource matching.

[Postremus]

I am btw. happy to introduce a config switch for this @geoand , I just don't want to do anything based on false assumptions on how the old stack worked.

However, this change passes the tck, and the resource class merge is imo what users (i.e. me) expect to happen anyway.

[geoand]

@Postremus thanks for this.

Any chance you can do a small benchmark to see if this introduces any noticeable overhead?

[Postremus]

yep, will prepare a test project and do a simple benchmark (e.g. look at either profiler data or check how long dev mode restarts take comperativly)

[geoand]

Thanks!

We also want to make sure throughput for simple hello-world style rest calls is not affected

[gsmet]

@Postremus friendly ping.

[geoand]

@Postremus are you still interested in driving this home?

It seems like a rather good improvement

[gsmet]

@geoand I rebased and applied some minor tweaks to reduce the cost of this new layer. I think you're more equipped than me to see if it introduces some significant overhead?

[geoand]

I'll have a look

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Failing Jobs - Building bddb948eccc699469c3a3a517b8928a343589d19

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	Failures	Logs	Raw logs

Failures

:gear: Initial JDK 11 Build #

- Failing: independent-projects/resteasy-reactive/server/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/agroal/deployment and 311 more

:package: independent-projects/resteasy-reactive/server/runtime

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.8.0:check (check-imports) on project resteasy-reactive: Imports are not sorted in /home/runner/work/quarkus/quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/startup/RuntimeDeploymentManager.java

[geoand]

Fixed the styling issue and applied one more small enhancement

[quarkus-bot[bot]]

:heavy_check_mark: 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.