Substitute org.lz4:lz4-java with at.yawk.lz4:lz4-java in all subprojects#39297
Substitute org.lz4:lz4-java with at.yawk.lz4:lz4-java in all subprojects#39297sjvanrossum wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses security vulnerabilities and dependency conflicts associated with the unmaintained 'org.lz4:lz4-java' library. By enforcing a global dependency substitution, the project ensures that the more secure 'at.yawk.lz4:lz4-java' is used consistently, simplifying the build configuration and removing the need for manual conflict resolution in individual modules. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies dependency management by removing the custom resolveCapabilitiesConflict helper and its usages across multiple subprojects. Instead, it centralizes the substitution of org.lz4:lz4-java with at.yawk.lz4:lz4-java:1.10.1 globally in the root build.gradle.kts to address known vulnerabilities. The review feedback suggests optimizing build performance by applying this resolution strategy only to resolvable configurations and centralizing the version definition in gradle.properties for better maintainability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| configureEach { | ||
| resolutionStrategy { | ||
| dependencySubstitution { | ||
| // Substitute dependencies (including transitive) on org.lz4:lz4-java regardless of capability conflicts. | ||
| // All published versions of org.lz4:lz4-java have known vulnerabilities. | ||
| // At present, all versions of at.yawk.lz4:lz4-java <1.10.1 have known vulnerabilities. | ||
| // Substituting org.lz4:lz4-java ensures that at.yawk.lz4:lz4-java is always selected. | ||
| // Gradle resolves version conflicts by selecting the highest version by default. | ||
| // Gradle only resolves capability conflicts when multiple modules provide the same capability. | ||
| // This would allow org.lz4:lz4-java to still be selected, which is undesirable because of vulnerabilities. | ||
| substitute(module("org.lz4:lz4-java")).using(module("at.yawk.lz4:lz4-java:1.10.1")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two improvement opportunities in this block:
- Performance Optimization: Configuring the resolution strategy for every single configuration across all subprojects introduces unnecessary configuration overhead, especially for non-resolvable configurations (e.g.,
api,implementation,compileOnly). Restricting this to only resolvable configurations usingisCanBeResolvedimproves build performance. - Centralized Dependency Management: Hardcoding the version
1.10.1directly in the root build script duplicates the dependency version definition and bypasses centralized dependency management. Defining this version in a centralized location (such asgradle.properties) and referencing it here usingproject.findPropertyimproves maintainability.
configureEach {
if (isCanBeResolved) {
val lz4Version = project.findProperty("lz4.version") ?: "1.10.1"
resolutionStrategy {
dependencySubstitution {
// Substitute dependencies (including transitive) on org.lz4:lz4-java regardless of capability conflicts.
// All published versions of org.lz4:lz4-java have known vulnerabilities.
// At present, all versions of at.yawk.lz4:lz4-java <1.10.1 have known vulnerabilities.
// Substituting org.lz4:lz4-java ensures that at.yawk.lz4:lz4-java is always selected.
// Gradle resolves version conflicts by selecting the highest version by default.
// Gradle only resolves capability conflicts when multiple modules provide the same capability.
// This would allow org.lz4:lz4-java to still be selected, which is undesirable because of vulnerabilities.
substitute(module("org.lz4:lz4-java")).using(module("at.yawk.lz4:lz4-java:$lz4Version"))
}
}
}
}There was a problem hiding this comment.
Suggestion 1 is not commonly applied as far as I can tell from other Gradle projects.
Suggestion 2 seems fair given that all versions are defined in BeamModulePlugin.groovy, but it doesn't look particularly elegant:
var lz4Java = (project.ext.get("library") as Map<String, Map<String, *>>).get("java")!!.get("lz4_java").toString()
subprojects {
configurations {
configureEach {
resolutionStrategy {
dependencySubstitution {
// Substitute dependencies (including transitive) on org.lz4:lz4-java regardless of capability conflicts.
// All published versions of org.lz4:lz4-java have known vulnerabilities.
// At present, all versions of at.yawk.lz4:lz4-java <1.10.1 have known vulnerabilities.
// Substituting org.lz4:lz4-java ensures that at.yawk.lz4:lz4-java is always selected.
// Gradle resolves version conflicts by selecting the highest version by default.
// Gradle only resolves capability conflicts when multiple modules provide the same capability.
// This would allow org.lz4:lz4-java to still be selected, which is undesirable because of vulnerabilities.
substitute(module("org.lz4:lz4-java")).using(module(lz4Java))
}
}
}
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
assign set of reviewers |
|
Assigning reviewers: R: @ahmedabu98 for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Test failures seem to be caused by missing |
| } | ||
| } | ||
|
|
||
| subprojects { |
There was a problem hiding this comment.
The consideration I made capability resolution opt-in, instead of the default, in #39061 was to make sure we don't introduce new capability conflict in other Beam modules unintentionally. Because Gradle's resolutionStrategy.capabilitiesResolution.withCapability closure isn't transitive. i.e., downstream module compilation still need the same override. This was exactly what happens to beam-runners-flink-2.1 (after Flink 2.1 override "org.lz4:lz4-java" -> "at.yawk.lz4:lz4-java") and beam-examples-java (after Beam Flink runner override "org.lz4:lz4-java" -> "at.yawk.lz4:lz4-java")
Flink runner related components currently have this conflict due to Flink 2.1+ and Kafka 2.4 each have transitive dependency on 'org.lz4:lz4-java' capability from different packages
The bottom line is we don't propagate this conflict to user if they use a critical setup (Beam core + GCP dependencies + Dataflow runner)
I recommend we remain the opt-in mechism (use resolveCapabilitiesConflict in individual module), and if ever it propagates to (Beam core + GCP dependencies + Dataflow runner), we then need to bump their transitive dependency version to make sure there isn't capability conflict in the first place
It's fine to bump Beam's Kafka version requirement to 3.9.2, if it eliminates the capability conflict
2 participants
Initially I ran into capability conflicts with #39284 and #39285 because Kafka 3.9.2 and up depend on at.yawk.lz4:lz4-java.
I figured I'd set up conflict resolution for org.lz4:lz4-java and at.yawk.lz4:lz4-java at the root, but realized that usage of org.lz4:lz4-java is undesirable to begin with because it's unmaintained and every version has known security vulnerabilities.
Capability conflict resolution isn't enough to replace every dependency (including transitive) on org.lz4:lz4-java, but this can be solved with dependency substitutions instead (including transitive). Building KafkaIO (prior to #39284) on its own does not trigger a capability conflict since no modules providing the org.lz4:lz4-java capability are requested by dependencies of KafkaIO.
KafkaIO specifies a compileOnly/provided dependency on Kafka client libraries so the substitution shouldn't propagate in any Beam artifacts, which means that users of KafkaIO are responsible for substituting
org.lz4:lz4-javawhere/when applicable.The substitution would be propagated in Beam artifacts with a direct or transitive implementation dependency on
org.lz4:lz4-java, meaning that any dependency on Beam artifacts will transitively includeat.yawk.lz4:lz4-javainstead.If the above is considered overly paranoid, then retaining the current resolution strategy while simplifying maintenance of capability conflict resolution for org.lz4:lz4-java looks something like this:
This would at least unburden contributors and maintainers from patching new capability conflicts when dependency upgrades replace
org.lz4:lz4-javaas is the case for #39284.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.