Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jackson bom to control version of all transitive jackson deps #848

Closed
wants to merge 1 commit into from

Conversation

robobario
Copy link
Contributor

@robobario robobario commented Oct 25, 2023

Why:
The 0.27 dist contains a mix of jackson versions, and there is what I think is a rotten comment in the pom.xml that jackson-databind is a test only dependency when we have usages in src/main like src/main/java/io/strimzi/kafka/bridge/http/converter/JsonUtils.java

./com.fasterxml.jackson.core.jackson-annotations-2.15.2.jar
./com.fasterxml.jackson.core.jackson-core-2.15.0.jar
./com.fasterxml.jackson.core.jackson-databind-2.15.2.jar
./com.fasterxml.jackson.dataformat.jackson-dataformat-yaml-2.14.2.jar
./com.fasterxml.jackson.datatype.jackson-datatype-jsr310-2.14.2.jar

We can use jackson-bom so everything is aligned on 2.15.2 without having to exclude transitive deps and include jackson-databind in the dependency analysis when checking for undeclared-but-used dependencies.

With the change you get:

mvn dependency:tree | grep jackson
[INFO] |  \- com.fasterxml.jackson.core:jackson-core:jar:2.15.2:compile
[INFO] +- com.fasterxml.jackson.core:jackson-databind:jar:2.15.2:compile
[INFO] |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.15.2:compile
[INFO] |  |  |  +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.15.2:compile
[INFO] |  |  |  \- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.15.2:compile

@scholzj
Copy link
Member

scholzj commented Oct 25, 2023

From my experience, using BOMs makes it hard to override their own transitive dependencies for example to fix CVEs. It is also not always desired to use the same Jackson versions for everything as there are many libraries using Jackson and no 100% compatibility between minor versions. So there are times when you have to use different versions (not sure the current alignment really requires it).

@ppatierno
Copy link
Member

Nothing to add to what Jakub said, same experience here.

@robobario
Copy link
Contributor Author

Separate from the BOM changes, am I right that the comment on databind

		<!-- Used only for test in the bridge, but needs to be here because OAuth brings it but a potential Maven bug remove it as runtime if only for test -->

is rotten, and we should remove that and the ignoredNonTestScopedDependencies for databind?

@ppatierno
Copy link
Member

is rotten, and we should remove that and the ignoredNonTestScopedDependencies for databind?

@robobario I know I added that line (Git blames me for that) but right now even the comment is not so clear to me :-(
If you think it's wrong, I would try to remove it, build the bridge and check if it really works but configuring OAuth as well and it could be a long way. @mstruk I know it's not specific OAuth issue, but do you have any hint here?

@mstruk
Copy link

mstruk commented Oct 26, 2023

@mstruk I know it's not specific OAuth issue, but do you have any hint here?

Historically jackson versions have never been a problem for OAuth library so whichever one we bumped to it always worked. The specific issue here is apparently the way to prevent pulling a specific transitive version of jackson (via OAuth dependency) into the bridge build. One way to work around would be via pom.xml dependencyManagement section setting jackson versions and aligning all transitive dependencies to the same jackson versions. I guess @ppatierno wants to let transitive versions through as they come and any conflicts between them are automatically resolved by Maven, but possibly to the older version rather than the newer version. To address that you then have to either exclude some transitive jackson dependencies from a direct dependency (like OAuth) or to explicitly add a dependency in order to end up with the version you prefer.

I personally would prefer the dependencyManagement approach at least to resolve individual issues like this one, as it's a cleaner solution. Also, if this specific dependency is only there for testing then it should probably have <scope>test</scope>.

@ppatierno
Copy link
Member

Also, if this specific dependency is only there for testing then it should probably have test.

I would agree but this comment "a potential Maven bug remove it as runtime if only for test" let me think we had a problem doing so.

@ppatierno
Copy link
Member

@robobario I am going to close this because it seems the direction is not the right one from our experience. Let me know if you want to reopen with more feedback. Thanks!

@ppatierno ppatierno closed this Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants