Arc - detect usage of unsupported @Specializes annotation and fail fast

[manovotn]

Fixes #49812

Quarkus now detects usage of this annotation and throws an exception that looks something like this:

jakarta.enterprise.inject.spi.DefinitionException: Quarkus does not support CDI Full @Specializes annotation; try using an @Alternative instead. Annotations found: [java.lang.String someProducer(), io.quarkus.arc.test.specialization.SpecializationUnsupportedTest$SomeBean]

[Ladicek]

The error message is gonna be unwieldy if a lot of @Specializes occurences exist in the code base, but I that's a minor issue.

[manovotn]

Hm, the CI failure is related. Apparently, we already have some @Specializes occurrences we didn't know about :)

Error: ]: Build step io.quarkus.arc.deployment.ArcProcessor#initialize threw an exception: jakarta.enterprise.inject.spi.DefinitionException: Quarkus does not support CDI Full @specializes annotation; try using an @Alternative instead.
Annotations found: [io.smallrye.graphql.cdi.context.CDISmallRyeContext]

[manovotn]

@phillip-kruger @jmartisk in SR graphql, is the @Specializes annotation really needed on CDISmallRyeContext?

In Quarkus it was just ignored up until this PR so the only usage must have been outside of Quarkus.
But even then the bean seems a bit awkward as it only declares this annotation and a priority?

[Ladicek]

The @Specializes annotation on CDISmallRyeContext doesn't seem to make any sense. Its parent class is not a bean class.

EDIT: actually let me shed a bit more light on that. The parent class is not a bean class in CDI Lite. It can still be a bean class in CDI Full with bean discovery mode of all. But https://github.com/smallrye/smallrye-graphql/blob/main/server/implementation-cdi/src/main/resources/META-INF/beans.xml explicitly says the bean discovery mode is annotated.

[manovotn]

The @Specializes annotation on CDISmallRyeContext doesn't seem to make any sense. Its parent class is not a bean class.

I am just running a local build of the graphql repo without the annotation (and the priority annotation) to see if there is some magic I might have missed but I agree it does look superfluous.

[phillip-kruger]

I am happy if it works and you remove it. Probably just me or one of us being dumb :) it's also used in wildfly and open liberty, not sure if they need it. @jmartisk ?

[manovotn]

I am happy if it works and you remove it. Probably just me or one of us being dumb :) it's also used in wildfly and open liberty, not sure if they need it. @jmartisk ?

Thanks for quick reply, I have sent a PR against SR repo to get the CI result on WFLY testing as I am too lazy to execute those locally 😅

[phillip-kruger]

I'll leave this for @jmartisk

[jmartisk]

Is it really needed to fail the build? What if an application is using a library that is shared with some other app that runs on a CDI container that supports it? Wouldn't a warning suffice? If we're about to start failing the build, I'd argue this should at least be marked with release/breaking-change and mentioned in the release notes.

Nevertheless, for SmallRye GraphQL, I do believe the annotation is unnecessary. Let me run the Quarkus test suite locally with a build containing smallrye/smallrye-graphql#2299 but I assume it will work.

[Ladicek]

Is it really needed to fail the build? What if an application is using a library that is shared with some other app that runs on a CDI container that supports it?

Great example. That library would not work correctly on Quarkus.

I think we could skip this check for known-compatible libraries, similarly to how we don't fail the build for known-compatible libraries that define bean discovery mode of all in beans.xml -- there's a build item extensions can produce.

[jmartisk]

I would simply log a warning, build failure is too aggressive.

[manovotn]

Is it really needed to fail the build? What if an application is using a library that is shared with some other app that runs on a CDI container that supports it?

If such library actually relies on the specialized bean working, then I'd argue we might as well fail because that will lead to some unexpected state anyway.

I would simply log a warning, build failure is too aggressive.

I thought about that initially but I don't think just a log msg is very helpful as it's very easy to miss.
Your deployment will "look valid and work" even though in an unexpected way.

But if others are in favor, I will of course change it.
What I don't want to end up with is logging + config knob just to disable the logging :-)

[Ladicek]

For the record, we have (for a long time; it is a shame we haven't acted upon it yet) this CDI issue jakartaee/cdi#646 that suggests detecting specialization (and usage of other unsupported APIs) and producing a deployment problem. I 100% agree with this PR (modulo perhaps the addition of known-compatible artifacts for which a deployment problem is not raised -- we already do that for beans.xml, we could do that for other unsupported things). If someone uses specialization and try to run that code in Quarkus, it will not function correctly, but that will be relatively hard to diagnose.

[jmartisk]

My concern is that there might be users depending on some library that contains the annotation and they may be perfectly aware that the annotation doesn't work (it's in some unused code) and they won't be able to build. In that case they should at least be able to turn this off by configuration... (But adding a config property for something this trivial sounds like overkill, so that's why I would probably suggest just logging a warning instead)

[manovotn]

While I favor the fail option, I agree that we need a way for application to effectively say "I know I am breaking the rules but I am certain this will work as expected".
After a discussion with Ladislav and Martin over Zulip, I like the build item option the most.

We can move the annotation validation into ArcProcessor (or anywhere where we can see the whole index) and introduce a build item which will allow application to state which occurrences of this annotation are expected. Or rather what packages and/or classes are to be considered OK.

This is akin to what we already have for beans.xml with discovery mode all.

[Ladicek]

Any reason to add a new build item instead of reusing the existing one?

I know its javadoc is focused on beans.xml, but we should be able to generalize that wording. And artifact granularity seems OK to me.

[manovotn]

Because they both serve a different purpose.
You can have an artifact with discovery mode all and register that as compatible but still have it contain @Specializes that is an error. Or vice versa.

[manovotn]

Furthermore, the KnownCompatibleBeanArchiveBuildItem is "only" used for a warning suppression during build whereas here we have a failure.

[Ladicek]

I understand that, but I wouldn't like us add a new build item for each unsupported CDI Full feature (there's like 10 of them or so?) that we want to suppress for an archive / class / whatever. One seems like a reasonable number. Two starts feeling wrong.

[manovotn]

I see what you mean...

I am thinking whether we could perhaps some Reason enum to KnownCompatibleBeanArchiveBuildItem that would specify what is that build item meant for. Or something along those lines.
I don't like the idea what we would consider such archive compatible in all current and future aspects.

But let's first see if we can agree on the "fail fast + build item" approach :)

[Ladicek]

An enum with selection of what is known to be compatible sounds like a great idea.

[Ladicek]

Or an enum set, actually. I guess you know that :-)

[mkouba]

But let's first see if we can agree on the "fail fast + build item" approach :)

+1

Or an enum set, actually.

+1 for KnownCompatibleBeanArchiveBuildItem#ingoredUnsupportedFeatures (or a better name ;-) defaulted to BEANS_XML_ALL.

[Ladicek]

+1 for KnownCompatibleBeanArchiveBuildItem#ingoredUnsupportedFeatures (or a better name ;-) defaulted to BEANS_XML_ALL.

I don't think we should default to BEANS_XML_ALL. I think we should:

1. Add a builder API to create an instance.
2. Deprecate the existing constructors and let them default to BEANS_XML_ALL.
3. Everyone should use the builder API.

[mkouba]

+1 makes sense. I did not talk about a concrete implementation but the way of solution ;-).

[manovotn]

I have reworked the PR in the following way:

• Detection is now in Arc extension as is the automated test
• There is a new build item which allows specifying packages or classes that are to be considered compatible despite possibly having the annotation
  • The build item is mentioned in the error message so that users have an idea how to circumvent it
  • The name of the build item is obnoxiously long, ideas are welcome 😁
• There is an extra commit which uses this new build item in GraphQL processor in order to pass the build/tests in that module
  • This can of course be removed as soon as we have new version of SR GraphQL

The error message now looks like this:

jakarta.enterprise.inject.spi.DefinitionException: Quarkus does not support CDI Full @Specializes annotation; try using an @Alternative instead. If you want to mark one or more of these occurrences as Quarkus compatible, take a look at io.quarkus.arc.deployment.KnownSpecializesAnnotationOccurrenceBuildItem. Annotation was found in the following classes: [io.quarkus.arc.test.specialization.SpecializationUnsupportedTest$SomeBean]

[manovotn]

@mkouba @Ladicek I've reworked KnownCompatibleBeanArchiveBuildItem to allow declaring a compatibilty reason with the initial two values being all mode in beans.xml and presence of @Specializes. There is also a builder through which you can create several of these build items at once.
The validation is now happening in BeanArchiveProcessor as that's where we have a notion of various archives and can match the GAV.

Please re-review at your convenience :)

[Ladicek]

I think we should have 2 builder() methods, one accepting groupId and artifactId, the other accepting groupId, artifactId and classifier (similarly to existing constructors that are being deprecated). These parameters are mandatory. The builder would then allow adding reasons: there may be multiple, KnownCompatibleBeanArchiveBuildItem should maintain a set, not just a single reason. Finally, build() would return a single KnownCompatibleBeanArchiveBuildItem.

[mkouba]

I think that it should actually have a method for each, i.e. for groupId, artifactId and classifier. What's the point of KnownCompatibleBeanArchives.Key anyway?

public static class Builder {
    Builder setGroupId(String groupId) {...}
    Builder setArtifactId(String artifactId) {...}
    Builder setClassifier(String classifier) {...}
    Builder addReason(String reason) {...}
    KnownCompatibleBeanArchiveBuildItem build() {...}
}

[Ladicek]

I agree that classifier, being optional, should be a method on the builder, but groupId and artifactId are mandatory. Since the number of mandatory members here is low, it feels better to enforce their setting at compile time rather than at runtime (build() would have to check if they were set). But I'd be fine with what you suggest too.

[mkouba]

If they're mandatory then the right thing to do is IMO to add them as params to the builder() method and make the fields final in the builder class.

[manovotn]

I do not quite agree but since I am outnumbered, I have adjusted it accordingly :)

[Ladicek]

LGTM otherwise, but the builder looks really weird here :-)

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 81ae265.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✔️	JVM Integration Tests - JDK 17			Logs	Raw logs	🔍
✔️	JVM Integration Tests - JDK 17 Windows			Logs	Raw logs	🔍
❌	JVM Integration Tests - JDK 21	Build	Failures	Logs	Raw logs	🔍

You can consult the Develocity build scans.

Failures

⚙️ JVM Integration Tests - JDK 21 #

- Failing: integration-tests/observability-lgtm 

📦 integration-tests/observability-lgtm

❌ Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.3:test (devmode-test) on project quarkus-integration-test-observability-lgtm:

See /home/runner/_work/quarkus/quarkus/integration-tests/observability-lgtm/target/surefire-reports for the individual test results.
See dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
There was an error in the forked process

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-openapi/deployment

❌ io.quarkus.smallrye.openapi.test.vertx.OpenApiHttpRootPathCorsTestCase.testCorsFilterProperties - History

• 1 expectation failed. Expected status code <200> but was <500>. - java.lang.AssertionError

Details

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
	at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)