Less user friendly message when have rest and resteasy in one project about quarkus

[holly-cummins]

The problem with moving the check to an earlier phase is that we would be able to check only the capabilities declared in the metadata. Those produced as build items directly (should be an exception, i think) would only be available for checking during augmentation (until @holly-cummins captures all of them in metadata).

Excellent - another use case for #40306. (That PR only records things annotated with @AddToMetadata, but this makes me wonder if it should record everything so tools can do analysis without explicit opt-in from the build item owner.)

[geoand]

This likely has something to do with the big rename.

@aloubyansky any idea why this is happening even though both extensions have <provides>io.quarkus.rest</provides> as a provided capability?

[quarkus-bot]

/cc @FroMage (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

[aloubyansky]

Yeah, we got a check failing even before augmentation starts.

Caused by: java.lang.Throwable: This is the location of the conflicting producer (io.quarkus.resteasy.deployment.ResteasyBuiltinsProcessor#setUpDenyAllJaxRs).
	at io.quarkus.builder.BuildStepBuilder.build(BuildStepBuilder.java:195)
	at io.quarkus.builder.BuildStepBuilder.buildIf(BuildStepBuilder.java:209)
	at io.quarkus.deployment.ExtensionLoader.lambda$loadStepsFromClass$80(ExtensionLoader.java:774)
	at java.base/java.util.function.Consumer.lambda$andThen$0(Consumer.java:65)
	at io.quarkus.deployment.ExtensionLoader.lambda$loadStepsFromClass$81(ExtensionLoader.java:882)

When I added capability checks I had an option to do it before the build of during the build. I did it during the build so dev mode could be launched and recover with user changing the POM (there was a discussion about that).
So to get the "user-friendly" error back I'd need to move the check to an earlier phase.

[geoand]

🙏🏼

[aloubyansky]

The problem with moving the check to an earlier phase is that we would be able to check only the capabilities declared in the metadata. Those produced as build items directly (should be an exception, i think) would only be available for checking during augmentation (until @holly-cummins captures all of them in metadata).

[aloubyansky]

So fixing this issue by moving the capability check before the build will break dev mode in the following way: start an app in dev mode with just quarkus-rest, once booted add quarkus-resteasy. AFAIR, the app will just quit.
OTOH, this now will happen anyway due to another check failing.

[gsmet]

I wouldn't do that.

I think the main issue is that we are somehow sharing some config and behavior between the two implementations without having a common artifact to share and handle them. See both JaxRsSecurityConfig and the methods consuming this config.
It's a bit similar to what was done for REST Client config where we share some config for both REST Clients.

IMO, the right fix would be to have a shared artifact to handle this config and the production of the build item once, even with two implementations.

Another option would be to make the build item a multi and deal with potential conflicts in the consumer. Given how things are atm, the values would be exactly the same so we could just take the first one of the list and be done with it (and we could check that the values are consistent across the list if we want to be thorough). Not pretty but probably an easier fix.

Now, is it worth the hassle, I don't know but I think we had quite a few issues with users reporting this and that's what led us to have such a nice error message.

Thoughts?

[michalvavrik]

My 2 cents:

Another option would be to make the build item a multi and deal with potential conflicts in the consumer. Given how things are atm, the values would be exactly the same so we could just take the first one of the list and be done with it (and we could check that the values are consistent across the list if we want to be thorough). Not pretty but probably an easier fix.

Right now, the default check is exactly one, so I think it is not perfect to make this Multi. It's just implementation detail that right now this check is connected to default JAX-RS security, because SecurityCheck is independent concept. IMO it is completely legal that a build item is simple but can be produced by multiple extensions that are mutually exclusive.

TL;DR; I don't think it matter. If there is javadoc and validation that warns developer right when he tries to produce another default check. I wrote the paragraph above because I am surprised how much problems this new item caused.

I think the main issue is that we are somehow sharing some config and behavior between the two implementations without having a common artifact to share and handle them. See both JaxRsSecurityConfig and the methods consuming this config.

I think in past I heard either from you or from Sergey that I should be careful with creating a new modules (but I don't remember reason :-) Maybe many more modules would stress build?). Unless you can reuse quarkus-jaxrs-spi-deployment for this purpose, I think Multi is appropriate action.

[gsmet]

Right now, the default check is exactly one, so I think it is not perfect to make this Multi. It's just implementation detail that right now this check is connected to default JAX-RS security, because SecurityCheck is independent concept. IMO it is completely legal that a build item is simple but can be produced by multiple extensions that are mutually exclusive.

I perfectly agree that what using a SimpleBuildItem was the way to go. And I wouldn't even complain if we didn't have this issue, which is a bit annoying for the users.

I think in past I heard either from you or from Sergey that I should be careful with creating a new modules (but I don't remember reason :-) Maybe many more modules would stress build?). Unless you can reuse quarkus-jaxrs-spi-deployment for this purpose, I think Multi is appropriate action.

Yeah so I had a look as I completely forgot about this module and I don't think it would be ideal as it will bring the security bits in areas where we don't need them.

Creating another artifact would be the most acceptable answer theoretically but I think in this case, I would advice to make it a MultiBuildItem, add a comment and then we could handle it two ways:

• consume Capabilities to make sure the capability aggregation step is executed first and then also fail if we have more than one item in the Multi
• accept multiple values as long as they are identical

I think I would be in favor of the former with a comment explaining why we consume Capabilities.

If this looks acceptable to everyone, do you want to handle it or should I?

[michalvavrik]

If this looks acceptable to everyone, do you want to handle it or should I?

I am happy to do it little later this week. Thank you for elaborate comments.