Refine how resource classes with same components are merged #25489

Postremus · 2022-05-10T20:00:13Z

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 · 2022-05-11T05:57:56Z

Thanks for looking into this @Postremus!

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

Postremus · 2022-05-11T06:04:01Z

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

geoand · 2022-05-11T06:06:37Z

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 · 2022-05-11T20:16:12Z

@geoand
TCKs are passing.

Postremus · 2022-05-12T04:22:18Z

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

geoand

LGTM, thanks!

@stuartwdouglas any objections?

geoand

Actually, looking at #25462 (comment), it seems like this was an opt-in feature of RESTEasy Classic.

So we should probably do the same here.

Postremus · 2022-05-17T19:49:47Z

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:

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 · 2022-05-17T19:50:47Z

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.

stuartwdouglas

I think this looks ok.

geoand · 2022-05-19T05:27:30Z

@Postremus thanks for this.

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

Postremus · 2022-05-19T05:48:21Z

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 · 2022-05-19T05:52:58Z

Thanks!

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

gsmet

@Postremus could you have another look at this one so that we finalize it?

I added a small suggestion.

gsmet · 2022-06-21T14:01:22Z

.../src/main/java/org/jboss/resteasy/reactive/server/core/startup/RuntimeDeploymentManager.java

+                keyBuilder.append("|");
+            }
+
+            this.key = keyBuilder.toString();


My guess is that it would be better if you could compute the hashCode here and cache it in a final field.

gsmet · 2022-08-09T21:34:22Z

@Postremus friendly ping.

geoand · 2022-09-01T10:36:05Z

@Postremus are you still interested in driving this home?

It seems like a rather good improvement

gsmet · 2023-02-02T10:18:50Z

@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 · 2023-02-02T10:20:50Z

I'll have a look

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.

This all comes from the observations I did in HV: - Objects.hash() can be costly as it creates an array - no real need to compare the classes given we know this object will only be used in a map that is consistent Also: - we don't need to cache the hashCode as String.hashCode() has a cache - better use equals() than compareTo()

geoand · 2023-02-02T11:50:40Z

Fixed the styling issue and applied one more small enhancement

quarkus-bot · 2023-02-02T14:54:41Z

✔️ 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.

Postremus force-pushed the issues/25462 branch 2 times, most recently from ad348f1 to 23bf08e Compare May 10, 2022 20:12

Postremus force-pushed the issues/25462 branch from 23bf08e to 864b184 Compare May 11, 2022 20:12

Postremus marked this pull request as ready for review May 11, 2022 20:16

Postremus requested a review from geoand May 11, 2022 20:17

This comment has been minimized.

Sign in to view

geoand approved these changes May 12, 2022

View reviewed changes

geoand requested changes May 16, 2022

View reviewed changes

stuartwdouglas approved these changes May 18, 2022

View reviewed changes

Postremus requested a review from geoand May 19, 2022 04:28

gsmet reviewed Jun 21, 2022

View reviewed changes

gsmet force-pushed the issues/25462 branch from 864b184 to bddb948 Compare February 2, 2023 10:17

This comment has been minimized.

Sign in to view

Postremus and others added 3 commits February 2, 2023 13:33

Reduce allocations on RuntimeDeploymentManager.MappersKey

e828d27

geoand force-pushed the issues/25462 branch from bddb948 to e828d27 Compare February 2, 2023 11:50

geoand approved these changes Feb 2, 2023

View reviewed changes

geoand merged commit a9070e8 into quarkusio:main Feb 2, 2023

quarkus-bot bot added this to the 3.0 - main milestone Feb 2, 2023

quarkus-bot bot added the kind/bugfix label Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine how resource classes with same components are merged #25489

Refine how resource classes with same components are merged #25489

Postremus commented May 10, 2022 •

edited

Loading

geoand commented May 11, 2022

Postremus commented May 11, 2022

geoand commented May 11, 2022 •

edited

Loading

Postremus commented May 11, 2022

This comment has been minimized.

Postremus commented May 12, 2022

geoand left a comment

geoand left a comment

Postremus commented May 17, 2022

Postremus commented May 17, 2022 •

edited

Loading

stuartwdouglas left a comment

geoand commented May 19, 2022

Postremus commented May 19, 2022

geoand commented May 19, 2022

gsmet left a comment

gsmet Jun 21, 2022

gsmet commented Aug 9, 2022

geoand commented Sep 1, 2022 •

edited

Loading

gsmet commented Feb 2, 2023

geoand commented Feb 2, 2023

This comment has been minimized.

geoand commented Feb 2, 2023

quarkus-bot bot commented Feb 2, 2023

Refine how resource classes with same components are merged #25489

Refine how resource classes with same components are merged #25489

Conversation

Postremus commented May 10, 2022 • edited Loading

geoand commented May 11, 2022

Postremus commented May 11, 2022

geoand commented May 11, 2022 • edited Loading

Postremus commented May 11, 2022

This comment has been minimized.

Postremus commented May 12, 2022

geoand left a comment

Choose a reason for hiding this comment

geoand left a comment

Choose a reason for hiding this comment

Postremus commented May 17, 2022

Postremus commented May 17, 2022 • edited Loading

stuartwdouglas left a comment

Choose a reason for hiding this comment

geoand commented May 19, 2022

Postremus commented May 19, 2022

geoand commented May 19, 2022

gsmet left a comment

Choose a reason for hiding this comment

gsmet Jun 21, 2022

Choose a reason for hiding this comment

gsmet commented Aug 9, 2022

geoand commented Sep 1, 2022 • edited Loading

gsmet commented Feb 2, 2023

geoand commented Feb 2, 2023

This comment has been minimized.

geoand commented Feb 2, 2023

quarkus-bot bot commented Feb 2, 2023

Postremus commented May 10, 2022 •

edited

Loading

geoand commented May 11, 2022 •

edited

Loading

Postremus commented May 17, 2022 •

edited

Loading

geoand commented Sep 1, 2022 •

edited

Loading