Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations #30018

zakkak · 2022-12-21T20:37:29Z

log4j-api is a dependency of elasticsearch-rest-high-level-client

log4j-api is not always present when using the
elasticsearch-rest-client-common extension as it's not a dependency of
it. Instead, it is a dependency of elasticsearch-rest-high-level-client:

+- io.quarkus:quarkus-elasticsearch-rest-high-level-client:jar:999-SNAPSHOT:compile
|  +- ...
|  +- org.jboss.logmanager:log4j2-jboss-logmanager:jar:1.1.1.Final:compile
|  |  \- org.apache.logging.log4j:log4j-api:jar:2.19.0:compile

com.fasterxml.jackson.module.jaxb.JaxbAnnotationIntrospector is not
always present when using the quarkus-jackson extension as it's not a
dependency of it. Instead, it is a dependency of
quarkus-resteasy-jackson:

+- io.quarkus:quarkus-resteasy-jackson:jar:999-SNAPSHOT:compile
|  +- ...
|  +- org.jboss.resteasy:resteasy-jackson2-provider:jar:4.7.7.Final:compile
|  |  +- ...
|  |  +- com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:jar:2.14.1:compile
|  |  |  +- com.fasterxml.jackson.jaxrs:jackson-jaxrs-base:jar:2.14.1:compile
|  |  |  \- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.14.1:compile

This results in failed attempts to register the class for reflection
when using the quarkus-jackson extension without
jackson-module-jaxb-annotations in the classpath.

Since we expect users to manually include the artifact in their
dependencies even when not using quarkus-resteasy-jackson we maintain
the registration in the quarkus-jackson extension but only perform it
when the artifact is present in the classpath.

cc @essobedo

Fixes warnings appearing with #29886

quarkus-bot · 2022-12-21T20:37:48Z

/cc @gsmet(elasticsearch), @loicmathieu(elasticsearch), @yrodiere(elasticsearch)

essobedo · 2022-12-22T09:07:31Z

...eployment/src/main/java/io/quarkus/resteasy/jackson/deployment/ResteasyJacksonProcessor.java

+
+ @BuildStep
+ ReflectiveClassBuildItem register() {
+ return new ReflectiveClassBuildItem(true, false, "com.fasterxml.jackson.module.jaxb.JaxbAnnotationIntrospector");


IMHO Jackson stuff should be managed by the Jackson extension, which makes more sense here as resteasy-jackson depends on jackson.

In theory I agree, but the thing is that the jackson extension does not depend on com.fasterxml.jackson.module:jackson-module-jaxb-annotations so there seems no need for it to register classes in it. On the other hand, resteasy-jackson does depend on com.fasterxml.jackson.module:jackson-module-jaxb-annotations (and is the reason we need to register the classes for reflection) so in practice it makes sense to me to move the registration there unless we expect people to manually add com.fasterxml.jackson.module:jackson-module-jaxb-annotations as a depedency to their project and assume that using quarkus-jackson would suffice to support it.

The Quarkus team should have better insight on this than me, since they probably have a better picture of how these extensions are used in the wild.

The alternative here is to keep the registration in quarkus-jackson and only perform it when com.fasterxml.jackson.module:jackson-module-jaxb-annotations is in the classpath.

.../io/quarkus/elasticsearch/restclient/common/deployment/ElasticsearchRestClientProcessor.java

Invalid.

yrodiere

LGTM for the Elasticsearch part.

I'm less sure about the Jackson part, since it's entirely possible that another extension makes use of jackson-module-jaxb-annotations without using RestEasy. Ideally we'd have a quarkus-jackson-jaxb-annotations extension, but that kind of strategy obviously cannot scale. I'd be in favor of not applying this change (the Jackson part) and instead finding some way of supressing warnings when we know they are not problematic, but I'll let more knowledgeable people decide.

zakkak · 2022-12-22T10:26:37Z

I'd be in favor of not applying this change (the Jackson part) and instead finding some way of supressing warnings when we know they are not problematic, but I'll let more knowledgeable people decide.

As said in #30018 (comment):

The alternative here is to keep the registration in quarkus-jackson and only perform it when com.fasterxml.jackson.module:jackson-module-jaxb-annotations is in the classpath.

Happy to proceed this way if there is consensus.

geoand · 2022-12-23T09:58:39Z

The alternative here is to keep the registration in quarkus-jackson and only perform it when com.fasterxml.jackson.module:jackson-module-jaxb-annotations is in the classpath.

Happy to proceed this way if there is consensus.

+1

log4j-api is a dependency of elasticsearch-rest-high-level-client `log4j-api` is not always present when using the elasticsearch-rest-client-common extension as it's not a dependency of it. Instead, it is a dependency of elasticsearch-rest-high-level-client: ``` +- io.quarkus:quarkus-elasticsearch-rest-high-level-client:jar:999-SNAPSHOT:compile | +- ... | +- org.jboss.logmanager:log4j2-jboss-logmanager:jar:1.1.1.Final:compile | | \- org.apache.logging.log4j:log4j-api:jar:2.19.0:compile ``` `com.fasterxml.jackson.module.jaxb.JaxbAnnotationIntrospector` is not always present when using the quarkus-jackson extension as it's not a dependency of it. Instead, it is a dependency of quarkus-resteasy-jackson: ``` +- io.quarkus:quarkus-resteasy-jackson:jar:999-SNAPSHOT:compile | +- ... | +- org.jboss.resteasy:resteasy-jackson2-provider:jar:4.7.7.Final:compile | | +- ... | | +- com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:jar:2.14.1:compile | | | +- com.fasterxml.jackson.jaxrs:jackson-jaxrs-base:jar:2.14.1:compile | | | \- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.14.1:compile ``` This results in failed attempts to register the class for reflection when using the `quarkus-jackson` extension without `jackson-module-jaxb-annotations` in the classpath. Since we expect users to manually include the artifact in their dependencies even when not using `quarkus-resteasy-jackson` we maintain the registration in the `quarkus-jackson` extension but only perform it when the artifact is present in the classpath.

zakkak · 2022-12-23T16:23:33Z

I updated the jackson changes based on the provided feedback, please re-review.

quarkus-bot · 2022-12-24T16:15:01Z

Failing Jobs - Building `690eeaf`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
⌛	JVM Tests - JDK 17 MacOS M1	`Build`	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

yrodiere

Regarding the Elasticsearch part, no change, LGTM.

Regarding the Jackson part, personally I would very much prefer to fix #29886 so that there's no warning when a class enabled for reflection is not found, be it only when we enable a "nowarning" flag on ReflectiveClassBuildItem. Because I very much doubt we will want to jump through hoops like this every time we need to handle an optional dependency.

But if this solution is what @geoand prefers... I'll let @geoand approve ;)

geoand · 2023-01-03T10:45:54Z

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java

+ x -> x.getGroupId().equals("com.fasterxml.jackson.module")
+ && x.getArtifactId().equals("jackson-module-jaxb-annotations"))) {


I know I've asked elsewhere too so apologies for that, but did we reach a consensus about this pattern?

It looks like there is no better way at the moment (see #29886 (comment))

zakkak · 2023-01-03T13:13:32Z

Thank for the (re-)review @yrodiere

prefer to fix #29886 so that there's no warning when a class enabled for reflection is not found

IMHO that would not be a fix, but instead it would mean maintaining an old issue. I personally prefer having someone manually going through such errors and jumping through hooks from having classes that should be registered, not being registered without anyone noticing.

zakkak added the area/native-image label Dec 21, 2022

quarkus-bot bot added area/elasticsearch area/jackson Issues related to Jackson (JSON library) labels Dec 21, 2022

zakkak mentioned this pull request Dec 21, 2022

Move reflection and serialization configuration from Feature to json #29886

Merged

This comment has been minimized.

Sign in to view

essobedo reviewed Dec 22, 2022

View reviewed changes

yrodiere previously requested changes Dec 22, 2022

View reviewed changes

.../io/quarkus/elasticsearch/restclient/common/deployment/ElasticsearchRestClientProcessor.java Show resolved Hide resolved

yrodiere reviewed Dec 22, 2022

View reviewed changes

zakkak force-pushed the 2022-12-21-move-registrations-to-proper-extensions branch from c5009f1 to 690eeaf Compare December 23, 2022 16:21

zakkak changed the title ~~Move log4j-api and jackson-module-jaxb-annotations registrations to correct quarkus modules~~ Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations Dec 23, 2022

zakkak requested review from essobedo and yrodiere and removed request for essobedo December 23, 2022 16:23

essobedo approved these changes Dec 23, 2022

View reviewed changes

yrodiere reviewed Jan 2, 2023

View reviewed changes

yrodiere requested a review from geoand January 2, 2023 08:10

geoand reviewed Jan 3, 2023

View reviewed changes

geoand approved these changes Jan 3, 2023

View reviewed changes

yrodiere merged commit 533f315 into quarkusio:main Jan 3, 2023

quarkus-bot bot added this to the 2.16 - main milestone Jan 3, 2023

zakkak deleted the 2022-12-21-move-registrations-to-proper-extensions branch January 3, 2023 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations #30018

Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations #30018

zakkak commented Dec 21, 2022 •

edited

Loading

quarkus-bot bot commented Dec 21, 2022

This comment has been minimized.

essobedo Dec 22, 2022

zakkak Dec 22, 2022

yrodiere left a comment

zakkak commented Dec 22, 2022 •

edited

Loading

geoand commented Dec 23, 2022

zakkak commented Dec 23, 2022

quarkus-bot bot commented Dec 24, 2022

yrodiere left a comment

geoand Jan 3, 2023

zakkak Jan 3, 2023

geoand Jan 3, 2023

zakkak commented Jan 3, 2023

		x -> x.getGroupId().equals("com.fasterxml.jackson.module")
		&& x.getArtifactId().equals("jackson-module-jaxb-annotations"))) {

Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations #30018

Move log4j-api to correct quarkus module and add condition for jackson-module-jaxb-annotations registrations #30018

Conversation

zakkak commented Dec 21, 2022 • edited Loading

quarkus-bot bot commented Dec 21, 2022

This comment has been minimized.

essobedo Dec 22, 2022

Choose a reason for hiding this comment

zakkak Dec 22, 2022

Choose a reason for hiding this comment

yrodiere left a comment

Choose a reason for hiding this comment

zakkak commented Dec 22, 2022 • edited Loading

geoand commented Dec 23, 2022

zakkak commented Dec 23, 2022

quarkus-bot bot commented Dec 24, 2022

Failing Jobs - Building 690eeaf

yrodiere left a comment

Choose a reason for hiding this comment

geoand Jan 3, 2023

Choose a reason for hiding this comment

zakkak Jan 3, 2023

Choose a reason for hiding this comment

geoand Jan 3, 2023

Choose a reason for hiding this comment

zakkak commented Jan 3, 2023

zakkak commented Dec 21, 2022 •

edited

Loading

zakkak commented Dec 22, 2022 •

edited

Loading

Failing Jobs - Building `690eeaf`