Upgrade to 151 #157
Upgrade to 151 #157
Conversation
WalkthroughUpgrades Java toolchain to Java 21 across CI, Docker, and build configs; bumps Tycho/JDT.LS/lsp4j versions; renames site bundle IDs to java-analyzer-bundle.*; adds a public LOCATION_LOCK and synchronizes compilation-unit cleanup and location computations; minor logging and regex tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Make sure that you update the tycho target as well to use 1.51 as well as consider updating the eclipse dependencies |
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
…rt and queries like (A|B|C) Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
upgrade-to-151
branch
from
bcefa49 to
0d49166
Compare
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
upgrade-to-151
branch
from
0d49166 to
f440a50
Compare
Dockerfile
Outdated
| FROM registry.access.redhat.com/ubi9/ubi-minimal AS index-download | ||
| RUN microdnf install -y wget zip && microdnf clean all && rm -rf /var/cache/dnf | ||
| ENV JAVA_HOME /usr/lib/jvm/java-21-openjdk | ||
| RUN /usr/local/apache-maven-3.9.11/bin/mvn clean install -DskipTests=true |
There was a problem hiding this comment.
I combined two intermediate images here in one step to avoid setting up maven, java twice
pranavgaikwad
force-pushed
the
upgrade-to-151
branch
2 times, most recently
from
560f483 to
2781aad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java (1)
69-75: Fix file-type check (likely wrong bitmask op) and tighten the new log line formatting/safety.
- Line 71:
(match.getResource().getType() | IResource.FILE) == 0looks like a bug; OR-ing withIResource.FILEdoesn’t test “is file”. This likely should be&and compared to0.- Line 88: log message is missing spaces, and
querymay contain newlines/control chars (log readability / log-forging risk). Consider normalizing before logging.Proposed patch:
- if ((match.getResource().getType() | IResource.FILE) == 0 || match.getElement() == null) { + if ((match.getResource().getType() & IResource.FILE) == 0 || match.getElement() == null) { logInfo("match.getResource().getType() | IResource.FILE"); return; } @@ - logInfo("getting match: " + match + "with provider: " + symbolProvider + " for query: " + this.query); + String safeQuery = String.valueOf(this.query).replaceAll("[\\r\\n\\t]", " "); + logInfo("getting match: " + match + " with provider: " + symbolProvider + " for query: " + safeQuery);Also applies to: 88-90
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (1)
200-282: Fix correctness issues inqueryQualificationMatches: null-safety + string comparisons.
unit.makeConsistent(null)is called even whenunitcan be null (several callers pass null) → potential NPE.- Multiple
!= ""comparisons are reference comparisons in Java → can silently break matching logic.- Line 210 comment says “Remove any parentheses and their contents”, but the new regex only removes groups without
|→ either adjust comment or behavior.Recommended patch (minimal, focused):
default boolean queryQualificationMatches(String query, IJavaElement matchedElement,ICompilationUnit unit, Location location) { - // Make sure that the ICompilationUnit is conistant - try { - unit.makeConsistent(null); - } catch(Exception e) { - logInfo("unable to make unit consistant, will still try as could be class file in a jar" + e); - } + // Make sure that the ICompilationUnit is consistent (if present) + if (unit != null) { + try { + unit.makeConsistent(null); + } catch (Exception e) { + logInfo("unable to make unit consistent, will still try as could be class file in a jar" + e); + } + } // Remove any parentheses and their contents query = query.replaceAll("\\([^|]*\\)", ""); query = query.replaceAll("(?<!\\.)\\*", ".*"); @@ - if (queryQualification != "" && location.getUri().contains(queryQualification.replaceAll("\\.", "/"))) { + if (!queryQualification.isEmpty() + && location != null + && location.getUri() != null + && location.getUri().contains(queryQualification.replaceAll("\\.", "/"))) { return true; } @@ - for (IPackageDeclaration packageDecl : unit.getPackageDeclarations()) { - if (packageQueryQualification!= "" && packageDecl.getElementName().matches(packageQueryQualification)) { + for (IPackageDeclaration packageDecl : unit.getPackageDeclarations()) { + if (!packageQueryQualification.isEmpty() + && packageDecl.getElementName().matches(packageQueryQualification)) { return true; } } @@ - if (importElement.contains("*")) { - if (queryQualification != "") { + if (importElement.contains("*")) { + if (!queryQualification.isEmpty()) { ... } }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java (1)
42-88: Only discard/close when you created a working copy, and do it infinally.Current code discards/closes any non-null
unit, which can include primary compilation units; also it can leak on exceptions before cleanup runs. Track whether you created a working copy and clean up in afinally.- if (this.query.contains(".")) { - ICompilationUnit unit = null; + if (this.query.contains(".")) { + ICompilationUnit unit = null; + boolean shouldCleanup = false; if (m.getElement() instanceof IMethod) { unit = ((IMethod) m.getElement()).getCompilationUnit(); } else if (m.getElement() instanceof IField) { unit = ((IField) m.getElement()).getCompilationUnit(); } @@ if (unit == null) { IClassFile cls = (IClassFile) ((IJavaElement) e).getAncestor(IJavaElement.CLASS_FILE); if (cls != null) { unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + shouldCleanup = true; } } - if (this.queryQualificationMatches(this.query, e, unit, location)) { + try { + if (unit != null && this.queryQualificationMatches(this.query, e, unit, location)) { ASTParser astParser = ASTParser.newParser(AST.getJLSLatest()); astParser.setSource(unit); astParser.setResolveBindings(true); @@ if (visitor.symbolMatches()) { symbols.add(symbol); } } - if (unit != null) { - synchronized (SymbolProvider.LOCATION_LOCK) { - unit.discardWorkingCopy(); - unit.close(); - } - } + } finally { + if (shouldCleanup && unit != null) { + synchronized (SymbolProvider.LOCATION_LOCK) { + unit.discardWorkingCopy(); + unit.close(); + } + } + } } else {java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/TypeSymbolProvider.java (1)
56-109: Fix accuracy heuristic + avoid closing non-working compilation units.
qualification.replaceAll(".", "/")replaces every character (regex.) — almost certainly meantreplace(".", "/")orreplaceAll("\\.", "/").qualification != ""is a reference compare.- Cleanup should only run if a working copy was created.
if (compilationUnit == null) { IClassFile cls = (IClassFile) element.getAncestor(IJavaElement.CLASS_FILE); if (cls != null) { compilationUnit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + // created working copy; safe to close/discard later } } @@ - if (qualification != "" && location.getUri().contains(qualification.replaceAll(".", "/"))) { + if (!qualification.isEmpty() + && location != null + && location.getUri() != null + && location.getUri().contains(qualification.replaceAll("\\.", "/"))) { isAccurate = true; } @@ - if (compilationUnit != null) { + if (compilationUnit != null && compilationUnit.isWorkingCopy()) { synchronized (SymbolProvider.LOCATION_LOCK) { compilationUnit.discardWorkingCopy(); compilationUnit.close(); } }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
99-135: Make cleanup consistent and guardqueryQualificationMatcheswhenunitis null.
- Early-return path closes/discards (Lines 85-88) without
LOCATION_LOCK, while the fallback path uses the lock (Lines 131-134) → make both consistent.queryQualificationMatches(...)currently callsunit.makeConsistent(...)without a null-check (inSymbolProvider) → ifunitis null here, the whole provider can bail out via exception.Suggested local hardening (in addition to fixing
queryQualificationMatchesas noted in SymbolProvider):- if (query.matches(fqdn) || fqdn.matches(query)) { - if (unit.isWorkingCopy()) { - unit.discardWorkingCopy(); - unit.close(); - } + if (query.matches(fqdn) || fqdn.matches(query)) { + if (unit.isWorkingCopy()) { + synchronized (SymbolProvider.LOCATION_LOCK) { + unit.discardWorkingCopy(); + unit.close(); + } + } @@ - if (this.queryQualificationMatches(this.query.replaceAll("\\(([A-Za-z_][A-Za-z0-9_]*(\\|[A-Za-z_][A-Za-z0-9_]*)*)\\)", ".*"), annotationElement, unit, location)) { + if (unit != null && this.queryQualificationMatches( + this.query.replaceAll("\\(([A-Za-z_][A-Za-z0-9_]*(\\|[A-Za-z_][A-Za-z0-9_]*)*)\\)", ".*"), + annotationElement, unit, location)) {java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (1)
61-85: Handleunit == nullbefore AST parse, and ensure cleanup runs viafinally.Right now
astParser.setSource(unit)can be invoked withunit == null. Also, ifgetWorkingCopy(...)succeeds and later code throws before Line 79, the working copy can leak.if (this.query.contains(".")) { ICompilationUnit unit = mod.getCompilationUnit(); + boolean shouldCleanup = false; if (unit == null) { IClassFile cls = (IClassFile) ((IJavaElement) mod).getAncestor(IJavaElement.CLASS_FILE); if (cls != null) { unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + shouldCleanup = true; } } + if (unit == null) { + return null; + } ASTParser astParser = ASTParser.newParser(AST.getJLSLatest()); astParser.setSource(unit); astParser.setResolveBindings(true); CompilationUnit cu = (CompilationUnit) astParser.createAST(null); CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.CONSTRUCTOR_CALL); - cu.accept(visitor); - if (visitor.symbolMatches()) { - symbols.add(symbol); - } - if (unit != null && unit.isWorkingCopy()) { - synchronized (SymbolProvider.LOCATION_LOCK) { - unit.discardWorkingCopy(); - unit.close(); - } - } + try { + cu.accept(visitor); + if (visitor.symbolMatches()) { + symbols.add(symbol); + } + } finally { + if (shouldCleanup && unit.isWorkingCopy()) { + synchronized (SymbolProvider.LOCATION_LOCK) { + unit.discardWorkingCopy(); + unit.close(); + } + } + } } else {Dockerfile (1)
18-32: Docker build likely fails:unzipis used but not installed inaddon-build.
dnf install ... zip ...doesn’t reliably provide theunzipbinary on UBI/RHEL.-FROM registry.access.redhat.com/ubi9/ubi AS addon-build -RUN dnf install -y java-21-openjdk-devel wget zip --nodocs --setopt=install_weak_deps=0 && dnf clean all && rm -rf /var/cache/dnf +FROM registry.access.redhat.com/ubi9/ubi AS addon-build +RUN dnf install -y java-21-openjdk-devel wget zip unzip --nodocs --setopt=install_weak_deps=0 && dnf clean all && rm -rf /var/cache/dnfpom.xml (1)
15-30: Update JaCoCo to 0.8.11 or later to support Java 21.JaCoCo
0.7.9does not support Java 21 class files; the minimum version with official Java 21 support is0.8.11. Since the pom.xml setsexecutionEnvironmenttoJavaSE-21, thejacoco.versionproperty must be updated to avoid test and coverage reporting failures.
🧹 Nitpick comments (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (1)
98-107: Lock scope looks OK, but consider reducing time spent under the lock.Right now logging and
returnhappen inside the synchronized blocks; if this lock becomes contended, it will amplify latency. Consider computing under the lock but logging/returning outside where possible (or at least avoid building log strings under the lock).Also applies to: 137-149, 150-160
Dockerfile (2)
26-32: GitHub “latest release” parsing viagrep|sedis brittle; considerjqor pin a known asset URL.
Multiplebrowser_download_urlmatches or JSON formatting changes can breakDOWNLOAD_URL.
19-25: Maven tarball integrity should be verified against official checksum; this is appropriate for supply chain security.Apache Maven 3.9.11 publishes official SHA-512 checksums. Add verification to the download step—download the
.sha512file alongside the tarball and verify the hash before extraction. This prevents using a compromised or corrupted binary. Optionally verify the PGP signature (.ascfile) for stronger assurance.Example:
RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.11/binaries/apache-maven-3.9.11-bin.tar.gz && \ curl -fsSL -o /tmp/apache-maven.tar.gz.sha512 https://dlcdn.apache.org/maven/maven-3/3.9.11/binaries/apache-maven-3.9.11-bin.tar.gz.sha512 && \ echo "$(cat /tmp/apache-maven.tar.gz.sha512) /tmp/apache-maven.tar.gz" | sha512sum -c - && \ tar -xzf /tmp/apache-maven.tar.gz -C /usr/local/ && \ rm /tmp/apache-maven.tar.gz /tmp/apache-maven.tar.gz.sha512
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/integration-tests.yml(2 hunks)Dockerfile(2 hunks)java-analyzer-bundle.core/META-INF/MANIFEST.MF(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java(5 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/TypeSymbolProvider.java(1 hunks)java-analyzer-bundle.site/category.xml(1 hunks)java-analyzer-bundle.test/META-INF/MANIFEST.MF(1 hunks)java-analyzer-bundle.tp/java-analyzer-bundle.target(2 hunks)pom.xml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T16:47:49.579Z
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.
Applied to files:
java-analyzer-bundle.test/META-INF/MANIFEST.MFjava-analyzer-bundle.site/category.xmlpom.xmljava-analyzer-bundle.tp/java-analyzer-bundle.target
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: JDT.LS Integration Tests (Phase 2)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (8)
java-analyzer-bundle.test/META-INF/MANIFEST.MF (1)
6-8: JavaSE-21 EE bump looks consistent with the repo-wide Java 21 migration.java-analyzer-bundle.core/META-INF/MANIFEST.MF (1)
13-14: JavaSE-21 EE bump is fine.Dockerfile (1)
45-55: Good simplification of index handling; verifycentral.archive-metadata.txtalways exists in the zip you download.
If the upstream release format changes, this COPY will fail at build time.java-analyzer-bundle.site/category.xml (1)
3-16: Verify the published bundle symbolic names match these IDs and that no consumers rely on old IDs.Ensure the renamed bundle IDs (
java-analyzer-bundle.core,java-analyzer-bundle.test, and their source variants) are properly reflected in all MANIFEST.MF files and feature definitions, and that any references to old bundle IDs (such asjava-rule-addon) have been removed or updated throughout the codebase.java-analyzer-bundle.tp/java-analyzer-bundle.target (1)
5-11: Target platform bumps are coherent; verify p2 URLs resolve and the mix resolves cleanly at build time.
This target mixes multiple p2 repos across eras (2021-09, 4.34, plus LSP4J and Xtext releases); resolution failures tend to surface only at build time.Also applies to: 59, 67, 75
pom.xml (1)
43-55: This environment restriction doesn't affect macOS/Windows support since this is a backend JDT.LS extension, not a desktop RCP application.The java-analyzer-bundle is a Language Server Protocol (LSP) backend extension that runs as a service, not a cross-platform UI application. It has no dependencies on platform-specific toolkits (cocoa/win32) and the CI only tests on Linux.
However, consider whether the
x86_64-only restriction is intentional, given that the container build (image-build.yaml) supports multiple CPU architectures (amd64, arm64, ppc64le, s390x). If Tycho builds should support these architectures too, the environment block may need additional entries.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (2)
44-44: LGTM: Improved logging accuracy.Logging
specificSymbols.size()instead of the accumulatedsymbols.size()correctly reports each provider's contribution, improving observability.
50-50: This change is correct—providers are type-exclusive by design.The break statement at line 50 is appropriate. Each provider explicitly handles a distinct SearchMatch type: MethodCallSymbolProvider and ConstructorCallSymbolProvider both cast to MethodReferenceMatch but are mutually exclusive (the latter filters with
isConstructor()), ImportSymbolProvider handles IImportDeclaration, and TypeSymbolProvider handles TypeReferenceMatch/TypeDeclarationMatch/etc. Since a single SearchMatch cannot simultaneously satisfy multiple type casts, only the first matching provider will return non-null results. The break optimizes by preventing wasted null checks.However, the comment "break here to avoid Type and IMPORT providers returning duplicate matches" is misleading—it suggests duplicates could occur, when the actual reason is that providers are mutually exclusive by SearchMatch type. Consider clarifying the comment to reflect that.
| FROM registry.access.redhat.com/ubi9/ubi AS jdtls-download | ||
| WORKDIR /jdtls | ||
| RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.38.0/jdt-language-server-1.38.0-202408011337.tar.gz &&\ | ||
| RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz &&\ | ||
| tar -xvf jdtls.tar.gz --no-same-owner &&\ | ||
| chmod 755 /jdtls/bin/jdtls &&\ | ||
| rm -rf jdtls.tar.gz |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
JDTLS Eclipse 1.51.0 download checksums signature
💡 Result:
The 1.51.0 milestone artifacts are hosted on Eclipse's download site and include a .sha256 checksum file for jdt-language-server-1.51.0-202510022025.tar.gz (see the directory listing). I don't see a separate GPG/.asc signature listed for that file in the same directory. [1][2]
Links:
- Directory showing jdt-language-server-1.51.0-202510022025.tar.gz and jdt-language-server-1.51.0-202510022025.tar.gz.sha256. [1]
- JDTLS download root listing. [2]
If you want, I can fetch the .sha256 contents and/or walk you through verifying the checksum (sha256sum) and checking for a signature.
🌐 Web query:
curl -fsSL flags behavior fail fast error handling
💡 Result:
Summary of flags and their fail-fast / error effects:
-
-f / --fail: treat HTTP 4xx/5xx as errors — curl exits non‑zero and (by default) does not write the HTTP body to stdout. Useful so scripts detect HTTP error responses. [1][2]
-
-s / --silent: suppress progress meter and most messages. It also hides error messages unless combined with --show-error. Use in scripts to avoid noisy progress output. [2]
-
-S / --show-error: when used with --silent, still print error messages to stderr on failure (so you get useful diagnostics while keeping the progress bar hidden). [2][3]
-
-L / --location: follow HTTP redirects (up to curl’s max redirects). Without it curl will not follow 3xx Location responses. Redirects can change method/credentials behavior unless you use --location-trusted or related flags. [4][5]
-
--fail-early: when requesting multiple URLs, return immediately on the first transfer failure so later transfers aren’t attempted. This is global and different from --fail (which concerns HTTP status codes). [6]
Notes on combined usage (e.g. curl -fsSL):
- Typical pattern for script downloads: -f (fail on HTTP error) + -sS (silent but show errors) + -L (follow redirects). This makes curl “fail fast” on HTTP errors, stay quiet on success, still print errors, and follow redirects to reach the final resource. [2][4]
Exit codes / behavior:
- curl returns nonzero on network/transfer errors and (with --fail) for HTTP error status codes; historically HTTP error with --fail produces exit code 22. Check $? for script logic. [1][7]
If you want I can give a minimal example script showing robust error handling and retries using these flags.
Sources:
[1] curl manpage (Ubuntu).
[2] "Correct cURL" guide (dotboris.io).
[3] alexwlchan: using --silent/--show-error and --write-out.
[4] Warp / tutorials on -L and max-redirs.
[5] Redirect notes and --location-trusted.
[6] curl --fail-early documentation (manpage).
[7] gist showing exit code behavior for --fail.
🌐 Web query:
Eclipse JDT Language Server 1.51.0 stable download URL
💡 Result:
Direct download (tar.gz):
https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz. [1]
SHA256 file:
https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz.sha256. [1]
(These files are listed on the Eclipse JDT LS milestones page.) [1]
Harden downloads and verify checksums for the JDTLS tarball.
The curl -s flag masks HTTP errors (4xx/5xx responses can be written to the output file without detection). Use -f to fail on HTTP errors, -S to show errors despite -s, and -L to follow redirects:
-RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz &&\
+RUN curl -fsSLo jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz &&\
tar -xvf jdtls.tar.gz --no-same-owner &&\
chmod 755 /jdtls/bin/jdtls &&\
rm -rf jdtls.tar.gzAdditionally, a .sha256 checksum file is available at the same location (jdt-language-server-1.51.0-202510022025.tar.gz.sha256). Verify the checksum after download:
sha256sum -c jdt-language-server-1.51.0-202510022025.tar.gz.sha256
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Dockerfile around lines 1 to 6, the curl invocation silently allows HTTP
errors and the downloaded tarball is not integrity-checked; update the RUN step
to use curl with flags -fSL so it fails on HTTP errors and follows redirects,
download the matching .sha256 file alongside the tarball, verify the tarball
with sha256sum -c against that .sha256 file and fail the build if verification
fails, then proceed to extract only after successful verification and clean up
the checksum file; ensure the Dockerfile exits non-zero on any failure (e.g.,
keep the RUN command's commands chained so a failed checksum aborts the build).
| public interface SymbolProvider { | ||
| public static final int MAX_PROBLEMS_TO_LOG = 10; | ||
| Object LOCATION_LOCK = new Object(); | ||
|
|
There was a problem hiding this comment.
Avoid a publicly exposed lock object (interface field).
LOCATION_LOCK in an interface is implicitly public static final, so any downstream code can synchronize on it (or block it), creating avoidable deadlock/latency risk. Prefer moving this lock to a package-private final class (or an internal implementation class) so it’s not part of the public API surface.
public interface SymbolProvider {
public static final int MAX_PROBLEMS_TO_LOG = 10;
- Object LOCATION_LOCK = new Object();
+ // NOTE: keep interface free of shared lock objects; prefer SymbolProviderLocks.LOCATION_LOCK.
+ @Deprecated(forRemoval = false)
+ Object LOCATION_LOCK = SymbolProviderLocks.LOCATION_LOCK;// new file in same package
final class SymbolProviderLocks {
static final Object LOCATION_LOCK = new Object();
private SymbolProviderLocks() {}
}🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java
around lines 32 to 35, the public LOCATION_LOCK field should be removed from the
interface and relocated to a package-private final holder class; create a new
final class in the same package (e.g., SymbolProviderLocks) with a static final
Object LOCATION_LOCK and a private constructor, update all references to use
SymbolProviderLocks.LOCATION_LOCK, and keep SymbolProvider free of any public
lock fields to avoid exposing synchronization primitives in the API.
2781aad to
a57f07b
Compare
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
upgrade-to-151
branch
from
a57f07b to
52c5dab
Compare
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
upgrade-to-151
branch
from
bd86edf to
b900843
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T16:47:49.579Z
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.
Applied to files:
pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: JDT.LS Integration Tests (Phase 2)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (1)
pom.xml (1)
44-47: Add documentation explaining why Tycho configuration settings are commented out.Lines 45–47 contain commented-out Tycho settings (
resolver,pomDependencies,ignoreTychoRepositories). While these are optional settings that have sensible defaults when not specified, add an inline comment explaining the rationale for omitting them (e.g., whether they conflict with the target platform definition or are intentionally simplified for the build process).
| if (compilationUnit != null) { | ||
| logInfo("found compliation unit for match: " + match); | ||
| return JDTUtils.toLocation(compilationUnit, match.getOffset(), match.getLength()); | ||
| synchronized (LOCATION_LOCK) { |
There was a problem hiding this comment.
Why do we need to sync on this now?
There was a problem hiding this comment.
@jmle there is a race condition where we might be working on the same compilation unit in two places, that causes intermittent failure in location resolution causing issues with line number 0. This fixes that. I can't find the issue about it (there is one somewhere), but I saw it in rulesets runs intermittently.
There was a problem hiding this comment.
I am absolutely open to doing this the right / idiomatic way btw. This is all I could think of.
There was a problem hiding this comment.
Can we get a copy of hte Compliation unit before working on it?
There was a problem hiding this comment.
@shawn-hurley there's only one call in the api getWorkingCopy() which gets you whatevers open in the index at the time. I am not sure how to make a deep copy of it.
| if (compilationUnit != null) { | ||
| logInfo("found compliation unit for match: " + match); | ||
| return JDTUtils.toLocation(compilationUnit, match.getOffset(), match.getLength()); | ||
| synchronized (LOCATION_LOCK) { |
There was a problem hiding this comment.
I am absolutely open to doing this the right / idiomatic way btw. This is all I could think of.
pom.xml
Outdated
| <pomDependencies>consider</pomDependencies> | ||
| <ignoreTychoRepositories>true</ignoreTychoRepositories> | ||
| <executionEnvironment>JavaSE-21</executionEnvironment> | ||
| <!-- <resolver>p2</resolver> --> |
There was a problem hiding this comment.
These three options no longer exist in the tycho plugin. From what I learned about them, in the past they offered more ways of doing certain thing, but now they are all set to sane defaults and that is only way of doing this moving forward.
There was a problem hiding this comment.
I say delete them, and in the commit message or something, add this note?
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
LGTM, small nits. We can come back on the lock vs trying to get a copy of the working copy or something
pom.xml
Outdated
| <pomDependencies>consider</pomDependencies> | ||
| <ignoreTychoRepositories>true</ignoreTychoRepositories> | ||
| <executionEnvironment>JavaSE-21</executionEnvironment> | ||
| <!-- <resolver>p2</resolver> --> |
There was a problem hiding this comment.
I say delete them, and in the commit message or something, add this note?
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
12-14: Update Maven prerequisite for Tycho 4.x compatibility.Tycho 4.x requires Maven 3.9 or later. The current prerequisite of Maven 3.0 is incompatible and will cause build failures.
🔎 Apply this diff to update the Maven prerequisite:
<prerequisites> - <maven>3.0</maven> + <maven>3.9.0</maven> </prerequisites>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T16:47:49.579Z
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.
Applied to files:
pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests (Phase 1)
🔇 Additional comments (2)
pom.xml (2)
18-19: Tycho 4.0.7 is available in Maven Central and supports Java 21.Tycho 4.0.7 has been released and is available from Maven Central repository. The version is compatible with Java 21 and addressed warning issues in the compiler plugin. No blocking concerns identified for this upgrade.
44-44: Java 21 execution environment is consistently applied across all modules.Verification confirms that the executionEnvironment upgrade to JavaSE-21 in pom.xml is properly reflected in all module manifests (./java-analyzer-bundle.core and ./java-analyzer-bundle.test) and target platform definitions, with no stray Java version references remaining.
…anymore beginning 12/16/2025, wth Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
27-31: Harden the Maven index data download.The Maven index data download has several reliability and security concerns:
wget --quietsuppresses errors, allowing HTTP failures to proceed undetected- No checksum or signature verification of the downloaded ZIP file
- Complex shell parsing of the GitHub API response could fail silently
🔎 Apply this diff to improve error handling and add verification
WORKDIR /maven-index-data -RUN DOWNLOAD_URL=$(wget --quiet -O - https://api.github.com/repos/konveyor/maven-search-index/releases/latest | grep '"browser_download_url".*maven-index-data.*\.zip' | sed -E 's/.*"browser_download_url": "([^"]+)".*/\1/') && \ - wget --quiet ${DOWNLOAD_URL} -O maven-index-data.zip && \ +RUN DOWNLOAD_URL=$(curl -fsSL https://api.github.com/repos/konveyor/maven-search-index/releases/latest | grep '"browser_download_url".*maven-index-data.*\.zip' | sed -E 's/.*"browser_download_url": "([^"]+)".*/\1/') && \ + test -n "${DOWNLOAD_URL}" || (echo "Failed to extract download URL" && exit 1) && \ + curl -fsSL -o maven-index-data.zip "${DOWNLOAD_URL}" && \ unzip maven-index-data.zip && \ rm maven-index-data.zipNote: If the maven-search-index repository provides checksums or signatures for releases, add verification before the unzip step.
♻️ Duplicate comments (1)
Dockerfile (1)
1-6: The JDTLS download still lacks proper error handling and checksum verification.This issue was flagged in a previous review but remains unaddressed. The
curl -sflag masks HTTP errors, and there's no integrity check of the downloaded tarball.As noted in the previous review, apply these changes to harden the download:
🔎 View the suggested fix
-RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz &&\ +RUN curl -fsSLo jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz &&\ + curl -fsSLo jdtls.tar.gz.sha256 https://download.eclipse.org/jdtls/milestones/1.51.0/jdt-language-server-1.51.0-202510022025.tar.gz.sha256 &&\ + sha256sum -c jdtls.tar.gz.sha256 &&\ tar -xvf jdtls.tar.gz --no-same-owner &&\ chmod 755 /jdtls/bin/jdtls &&\ - rm -rf jdtls.tar.gz + rm -rf jdtls.tar.gz jdtls.tar.gz.sha256
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: JDT.LS Integration Tests (Phase 2)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (3)
Dockerfile (3)
35-39: LGTM: Clean Java 21 upgrade with backwards compatibility.The upgrade to Java 21 as the default runtime while maintaining Java 8 for Gradle wrapper compatibility is well-structured. The Maven 3.9.12 setup is also properly configured.
49-50: Good: Maven installation and symlink setup.The Maven installation from the addon-build stage and the symlink creation ensure
mvnis available system-wide.
10-15: Verify the Java version for the Fernflower build.The fernflower stage uses
maven-openjdk17while the rest of the Dockerfile has been upgraded to Java 21. Confirm whether Java 17 is a hard requirement for fernflower's build tooling in this IntelliJ IDEA version or if upgrading to Java 21 is feasible for consistency across stages.
| RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz && \ | ||
| tar -xzf /tmp/apache-maven.tar.gz -C /usr/local/ && \ | ||
| rm /tmp/apache-maven.tar.gz |
There was a problem hiding this comment.
Verify the Maven download with a checksum.
The Maven download uses proper curl flags but lacks integrity verification. Apache Maven releases include SHA512 checksums that should be verified before extraction.
🔎 Apply this diff to add checksum verification
-RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz && \
+RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz && \
+ curl -fsSL -o /tmp/apache-maven.tar.gz.sha512 https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz.sha512 && \
+ cd /tmp && sha512sum -c apache-maven.tar.gz.sha512 && \
tar -xzf /tmp/apache-maven.tar.gz -C /usr/local/ && \
- rm /tmp/apache-maven.tar.gz
+ rm /tmp/apache-maven.tar.gz /tmp/apache-maven.tar.gz.sha512📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz && \ | |
| tar -xzf /tmp/apache-maven.tar.gz -C /usr/local/ && \ | |
| rm /tmp/apache-maven.tar.gz | |
| RUN curl -fsSL -o /tmp/apache-maven.tar.gz https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz && \ | |
| curl -fsSL -o /tmp/apache-maven.tar.gz.sha512 https://dlcdn.apache.org/maven/maven-3/3.9.12/binaries/apache-maven-3.9.12-bin.tar.gz.sha512 && \ | |
| cd /tmp && sha512sum -c apache-maven.tar.gz.sha512 && \ | |
| tar -xzf /tmp/apache-maven.tar.gz -C /usr/local/ && \ | |
| rm /tmp/apache-maven.tar.gz /tmp/apache-maven.tar.gz.sha512 |
🤖 Prompt for AI Agents
In Dockerfile around lines 19 to 21, the Apache Maven tarball is downloaded but
not integrity-checked; download the corresponding SHA512 checksum file for the
exact Maven version, verify the tarball with sha512sum (or an equivalent
verification command) before extracting, fail the build if the checksum does not
match, and clean up checksum and archive files afterward to avoid leaving
sensitive artifacts.
5 participants
Summary by CodeRabbit
New Features
Updates
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.