🐛 Do special search for on-demand imports #143
Conversation
WalkthroughAdds AST-based scanning of in-scope Java compilation units (triggered for import-related symbol queries) to detect on-demand (wildcard) imports, create Namespace-kind SymbolInformation with LSP Locations via a new public helper, and merge those import-derived symbols with standard JDT search results. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Handler as SampleDelegateCommandHandler
participant JDT as JDT Search
participant Requestor
participant Units as In-scope Units
participant AST as ASTParser
Client->>Handler: searchSymbols(query, location=8?)
Handler->>JDT: perform standard symbol search
JDT-->>Requestor: stream matching symbols
Requestor-->>Handler: receive standard results
alt location == import-related (8)
Handler->>Units: gather ICompilationUnit list
loop per unit
Handler->>AST: parse(unit) -> CompilationUnit
AST-->>Handler: AST with ImportDeclaration nodes
Handler->>Handler: filter on-demand imports matching query
Handler->>Handler: call getLocationForImport(unit, import, cuAst)
Handler->>Handler: create SymbolInformation(kind=Namespace) with Location
end
end
Handler->>Handler: merge standard + import-derived symbols
Handler-->>Client: combined symbol list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
165-166: Prefer enum constants over magic numbers for location types.The TODO comment correctly identifies that these location values should be refactored to enums for better maintainability and type safety. This would prevent errors from using incorrect integer values and improve code readability.
371-389: AST parsing implementation is sound, but consider performance optimizations.The AST parsing approach correctly identifies on-demand imports and creates appropriate symbols. However, parsing AST for each compilation unit could be expensive for large projects.
Consider:
- Moving AST parsing to a separate method for better modularity
- Adding early termination if a result limit is reached
- Potentially caching parsed ASTs if they're used elsewhere
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (1)
DefaultSymbolProvider(12-66)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ImportSymbolProvider.java (1)
ImportSymbolProvider(14-32)
⏰ 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: Build tackle2-addon-analyzer
🔇 Additional comments (9)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (9)
245-245: Collection initialization is appropriate.The initialization of the units list to track compilation units is well-placed and follows standard practices.
301-304: Good implementation of compilation unit collection.The code correctly collects compilation units from source packages within the included fragments. The K_SOURCE check ensures only source packages are processed.
334-335: Symbols list initialization moved correctly.Moving the symbols list initialization before the search operations is appropriate for collecting results from multiple sources.
361-361: Symbol collection approach is correct.Using
addAllto merge symbols from the requestor maintains the existing search results.
363-365: Well-documented approach for handling on-demand imports.The comment clearly explains why special handling is needed for wildcard imports that aren't available through standard JDT search.
402-427: Well-implemented location calculation method.The
getLocationForImportmethod correctly:
- Calculates 0-based line and column positions for LSP compatibility
- Creates proper Range and Location objects
- Handles URI conversion appropriately
398-399: Correct return of merged results.Returning the combined symbols list ensures both standard search results and on-demand imports are included in the final output.
366-367: Fix the regex pattern for matching wildcard imports.The regex pattern
[^A-Z*]+\\*appears incorrect for matching package wildcard imports. It incorrectly excludes uppercase letters and won't match valid patterns likejava.util.*orjavax.persistence.*. The pattern should match any package name ending with.*.Apply this fix to correctly match wildcard import patterns:
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + Matcher matcher = Pattern.compile(".*\\.\\*$").matcher(query);Likely an incorrect or invalid review comment.
314-324: Ignore removal ofscope.encloses(unit); it’s required for filtering compilation units to the configured IJavaSearchScope(help.eclipse.org)Likely an incorrect or invalid review comment.
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
| symbol.setKind(SymbolKind.Module); | ||
| symbol.setContainerName(unit.getElementName()); | ||
| symbol.setLocation(getLocationForImport(unit, imp, cu)); | ||
| System.out.println("Found in " + unit.getElementName()); |
There was a problem hiding this comment.
Remove debug print statement.
The System.out.println statement should be replaced with proper logging using logInfo.
- System.out.println("Found in " + unit.getElementName());
+ logInfo("Found on-demand import in " + unit.getElementName());📝 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.
| System.out.println("Found in " + unit.getElementName()); | |
| logInfo("Found on-demand import in " + unit.getElementName()); |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
around line 384, replace the debug System.out.println("Found in " +
unit.getElementName()); with a proper logging call using logInfo; remove the
println and invoke logInfo with a clear message (e.g., "Found in " plus
unit.getElementName()) so the information is recorded via the project's logging
facility instead of standard output.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
384-384: Replace println with proper logging
Use the project logger instead of stdout.- System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName());
371-379: Precompile query pattern once and handle invalid regex
Compiling inside the inner loop is wasteful and unhandled PatternSyntaxException can abort the search.- ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); - for (ICompilationUnit unit : units) { + ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); + Pattern queryPattern; + try { + queryPattern = Pattern.compile(query); + } catch (java.util.regex.PatternSyntaxException e) { + logInfo("Invalid regex for import query '" + query + "': " + e.getMessage()); + queryPattern = null; + } + for (ICompilationUnit unit : units) { parser.setSource(unit); CompilationUnit cu = (CompilationUnit) parser.createAST(null); for (Object o : cu.imports()) { ImportDeclaration imp = (ImportDeclaration) o; if (imp.isOnDemand()) { - if (Pattern.compile(query).matcher(imp.getName().getFullyQualifiedName()).matches()) { + if (queryPattern != null && queryPattern.matcher(imp.getName().getFullyQualifiedName()).matches()) { SymbolInformation symbol = new SymbolInformation();
🧹 Nitpick comments (4)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (4)
245-245: Avoid duplicate unit scanning and scale better on large workspaces
The same compilation units may be added from included fragments and later from scope traversal; dedupe before parsing.Minimal change (dedupe at iteration):
- for (ICompilationUnit unit : units) { + java.util.Set<ICompilationUnit> uniqueUnits = new java.util.LinkedHashSet<>(units); + for (ICompilationUnit unit : uniqueUnits) {Optional: skip scope traversal if included units already exist:
- // Get all compilation units in scope - for (IJavaProject p : targetProjects) { + // Get all compilation units in scope (only if none were collected from includedPaths) + if (units.isEmpty()) { + for (IJavaProject p : targetProjects) { ... - } + } + }Also applies to: 301-304, 358-366, 371-373
357-368: Add cancellation checks in long-running loops
Honor user cancellation to keep the server responsive.- for (IJavaProject p : targetProjects) { + for (IJavaProject p : targetProjects) { + if (monitor != null && monitor.isCanceled()) return symbols; ... } ... - for (ICompilationUnit unit : uniqueUnits) { + for (ICompilationUnit unit : uniqueUnits) { + if (monitor != null && monitor.isCanceled()) return symbols; parser.setSource(unit);Also applies to: 371-389
402-419: Prefer ResourceUtils for URIs and guard against null resource
Ensures properly encoded document URIs and avoids NPEs.- String uri = icu.getResource().getLocationURI().toString(); - return new Location(uri, range); + var res = icu.getResource(); + String uri = (res != null) ? ResourceUtils.toUri(res) : icu.getPath().toFile().toURI().toString(); + return new Location(uri, range);
371-389: Batch-parse ASTs for better throughput (optional)
Using ASTParser#createASTs is significantly faster for many units.I can provide a follow-up patch converting the per-unit createAST loop to createASTs with an ASTRequestor if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (1)
DefaultSymbolProvider(12-66)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ImportSymbolProvider.java (1)
ImportSymbolProvider(14-32)
⏰ 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: Build tackle2-addon-analyzer
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
371-373: Good: pattern compiled once outside inner loopThis addresses the earlier concern about compiling the regex inside the loop. ✅
385-385: Replace println with structured loggingUse
logInfoinstead ofSystem.out.println.- System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName());
352-355: Fix wildcard detection to include static on-demand imports and uppercase identifiersThe current regex misses queries like
java.util.Collections.*. Use a stricter, anchored pattern that allows uppercase letters and$:- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + // Accept package or type-qualified wildcard imports, e.g., "java.util.*" or "java.util.Collections.*" + Matcher matcher = Pattern.compile("^[\\w$.]+\\.\\*$").matcher(query);
🧹 Nitpick comments (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
358-368: Prune package traversal by prefix to reduce AST parsesWhen scanning on-demand imports, restrict package iteration to the query’s prefix to avoid walking unrelated packages:
- for (IJavaProject p : targetProjects) { + // Extract "java.util" from "java.util.*" + String pkgPrefix = query.substring(0, query.length() - 2); + for (IJavaProject p : targetProjects) { for (IPackageFragment pkg : p.getPackageFragments()) { if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) { + if (!pkg.getElementName().startsWith(pkgPrefix)) { + continue; + } for (ICompilationUnit unit : pkg.getCompilationUnits()) { if (scope.encloses(unit)) { units.add(unit); } } } } }
370-391: Deduplicate compilation units before parsing
unitscan accumulate duplicates (from included fragments and scoped traversal). Deduplicate to prevent double parsing and duplicate symbols:ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); Pattern regex = Pattern.compile(query); + // Deduplicate to avoid double parsing of the same unit + units = new ArrayList<>(new java.util.LinkedHashSet<>(units)); for (ICompilationUnit unit : units) {
381-383: Optional: reflect wildcard in the symbol’s nameConsider appending
.*so the symbol name mirrors the on-demand import:- symbol.setName(imp.getName().getFullyQualifiedName()); + symbol.setName(imp.getName().getFullyQualifiedName() + ".*");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (1)
DefaultSymbolProvider(12-66)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ImportSymbolProvider.java (1)
ImportSymbolProvider(14-32)
⏰ 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: Build tackle2-addon-analyzer
🔇 Additional comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
371-371: Confirm JDT Core version supports AST.getJLSLatest(). The bundle’s Require-Bundle entry for org.eclipse.jdt.core has no version qualifier—verify your OSGi runtime or build’s JDT Core is ≥ 3.27 (when getJLSLatest() was added); if not, switch to the older AST.JLS_LATEST constant for compatibility.
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
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)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (1)
DefaultSymbolProvider(12-66)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ImportSymbolProvider.java (1)
ImportSymbolProvider(14-32)
🔇 Additional comments (12)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (12)
8-8: LGTM!Good addition of the Matcher import to support the regex pattern matching functionality for on-demand import detection.
18-21: LGTM!Appropriate imports for AST-based parsing functionality. These are necessary for traversing the AST to detect on-demand imports.
29-29: LGTM!Good addition of LSP types for creating proper location information. These imports are essential for constructing the Location objects with proper Position and Range data.
Also applies to: 34-38
41-42: LGTM!Adding ImportSymbolProvider import enhances the symbol processing capabilities and aligns with the existing symbol provider pattern.
165-166: LGTM!Good practice to add TODO comments for future improvements. Moving these constants to enums would indeed improve code maintainability.
245-246: LGTM!Good initialization of collections to accumulate compilation units and symbols from both standard search and import scanning.
Also applies to: 337-338
314-318: LGTM!Efficient collection of compilation units from included fragments. The condition check for source fragments ensures only relevant units are processed.
364-364: LGTM!Correct integration of standard search results with the symbols collection.
366-382: Fix the wildcard detection patternThis issue was already identified in the past review comments. The current regex
[^A-Z*]+\\*excludes uppercase letters and misses static on-demand imports likejava.util.Collections.*.Apply this fix:
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + Matcher matcher = Pattern.compile("^[\\w$.]+\\.\\*$").matcher(query);
384-404: Optimize pattern compilation and improve loggingTwo issues were identified in past reviews:
- Pattern compilation inside the loop (performance issue)
- Debug print statement instead of proper logging
Apply these fixes:
// Now run ImportScanner only on units in scope ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); - Pattern regex = Pattern.compile(query); + Pattern queryPattern = Pattern.compile(query); for (ICompilationUnit unit : units) { parser.setSource(unit); CompilationUnit cu = (CompilationUnit) parser.createAST(null); for (Object o : cu.imports()) { ImportDeclaration imp = (ImportDeclaration) o; if (imp.isOnDemand()) { - if (regex.matcher(imp.getName().getFullyQualifiedName()).matches()) { + if (queryPattern.matcher(imp.getName().getFullyQualifiedName()).matches()) { SymbolInformation symbol = new SymbolInformation(); symbol.setName(imp.getName().getFullyQualifiedName()); symbol.setKind(SymbolKind.Namespace); symbol.setContainerName(unit.getElementName()); symbol.setLocation(getLocationForImport(unit, imp, cu)); - System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName()); symbols.add(symbol); } } } }
405-406: LGTM!Good consolidation of the conditional import scanning within the location check.
413-414: LGTM!Correct return of the accumulated symbols collection that includes both standard search results and import-derived symbols.
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
384-405: Extract AST compilation to improve performance and maintainability.The Pattern compilation and AST parsing logic inside the loop can be optimized and the debug print statement should use proper logging.
Apply this refactor to optimize the import scanning:
- // Now run ImportScanner only on units in scope - ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); - Pattern regex = Pattern.compile(query); - for (ICompilationUnit unit : units) { - parser.setSource(unit); - CompilationUnit cu = (CompilationUnit) parser.createAST(null); - for (Object o : cu.imports()) { - ImportDeclaration imp = (ImportDeclaration) o; - if (imp.isOnDemand()) { - if (regex.matcher(imp.getName().getFullyQualifiedName()).matches()) { - SymbolInformation symbol = new SymbolInformation(); - symbol.setName(imp.getName().getFullyQualifiedName()); - symbol.setKind(SymbolKind.Namespace); - symbol.setContainerName(unit.getElementName()); - symbol.setLocation(getLocationForImport(unit, imp, cu)); - System.out.println("Found in " + unit.getElementName()); - symbols.add(symbol); - } - } - } - } + // Now run ImportScanner only on units in scope + ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); + Pattern regex = Pattern.compile(query); + for (ICompilationUnit unit : units) { + parser.setSource(unit); + CompilationUnit cu = (CompilationUnit) parser.createAST(null); + for (Object o : cu.imports()) { + ImportDeclaration imp = (ImportDeclaration) o; + if (imp.isOnDemand()) { + if (regex.matcher(imp.getName().getFullyQualifiedName()).matches()) { + SymbolInformation symbol = new SymbolInformation(); + symbol.setName(imp.getName().getFullyQualifiedName()); + symbol.setKind(SymbolKind.Namespace); + symbol.setContainerName(unit.getElementName()); + symbol.setLocation(getLocationForImport(unit, imp, cu)); + logInfo("Found on-demand import in " + unit.getElementName()); + symbols.add(symbol); + } + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/DefaultSymbolProvider.java (1)
DefaultSymbolProvider(12-66)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ImportSymbolProvider.java (1)
ImportSymbolProvider(14-32)
⏰ 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: Build tackle2-addon-analyzer
🔇 Additional comments (7)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (7)
245-246: LGTM!The initialization of the compilation units list provides a clean foundation for accumulating units from included fragments and supporting the new on-demand import scanning functionality.
314-318: LGTM!The collection of compilation units from included fragments is correctly implemented and integrates well with the existing fragment processing logic.
337-338: LGTM!The symbols list initialization follows the established pattern and supports the accumulation of results from both standard search and import scanning.
364-365: LGTM!The aggregation of symbols from the requestor correctly maintains the complete result set for return to the caller.
413-414: LGTM!The return statement correctly provides the accumulated symbols from both standard search results and on-demand import scanning.
417-424: LGTM!The helper method correctly delegates to
JDTUtils.toLocation()for proper LSP Location construction and includes appropriate error handling. This resolves the previous review comment about completing the method implementation.
366-406: **Wildcard pattern detection still excludes uppercase letters and static imports.**Based on the web search results, the current regex pattern[^A-Z*]+\\*has significant limitations that will miss valid on-demand imports. Java supports both regular on-demand imports likeimport java.util.*;and static on-demand imports likeimport static java.lang.Math.*;. The current pattern excludes uppercase letters, which means it would miss:
- Static imports:
import static java.util.Arrays.*;orimport static java.lang.Math.*;- Class-qualified imports: Package names containing uppercase letters like
javax.persistence.*- Inner class imports: Imports like
import graphics.Rectangle.*;for nested classesThe fix needs to support both regular and static on-demand imports with a more comprehensive pattern.
Apply this fix to properly detect on-demand imports:
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + // Match both regular and static on-demand imports: package.* or static package.Class.* + Matcher matcher = Pattern.compile("^(static\\s+)?[\\w$.]+\\.\\*$").matcher(query);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
381-389: Use string equality on trimmed prefix instead of regex for on-demand import matching.Regex semantics here differ from JDT’s wildcard semantics and can lead to false matches; direct prefix equality is simpler and faster.
- Pattern regex = Pattern.compile(query); + // Trim trailing ".*" to match ImportDeclaration name exactly + String onDemandPrefix = query.endsWith(".*") ? query.substring(0, query.length() - 2) : query; @@ - if (imp.isOnDemand()) { - if (regex.matcher(imp.getName().getFullyQualifiedName()).matches()) { + if (imp.isOnDemand()) { + if (imp.getName().getFullyQualifiedName().equals(onDemandPrefix)) {
240-241: Avoid duplicate parsing by collecting compilation units in a Set.The same unit can be added from both included fragments and the scope walk, producing duplicate symbols. Use a LinkedHashSet to dedupe without changing order.
- List<ICompilationUnit> units = new ArrayList<>(); + Set<ICompilationUnit> units = new LinkedHashSet<>();Add imports:
import java.util.LinkedHashSet; import java.util.Set;
391-391: Prefer SymbolKind.Package for imports.LSP has a dedicated Package kind; it better conveys that the symbol represents a package import.
- symbol.setKind(SymbolKind.Namespace); + symbol.setKind(SymbolKind.Package);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(10 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). (1)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
361-364: Fix wildcard detection regex to include type-qualified/static imports.Current pattern excludes uppercase segments, missing queries like "java.util.Collections.*". Use a stricter, anchored pattern that allows word chars, '$' and '.'.
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + // Accept package or type-qualified wildcard imports, e.g., "java.util.*" or "java.util.Collections.*" + Matcher matcher = Pattern.compile("^[\\w$.]+\\.\\*$").matcher(query);
394-395: Replace println with structured logging.Use logInfo for consistency with the rest of the file.
- System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName());
412-419: Location helper LGTM.Good use of JDTUtils.toLocation for accurate LSP ranges.
jmle
added
the
cherry-pick/release-0.8
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
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)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
164-169: Wildcard queries misclassified as exact matches (OR vs AND bug).Using OR makes queries containing '' or '?' fall into exact-match mode. This breaks wildcard searches (e.g., 'javax.persistence.').
Fix the condition:
- if ((!query.contains("?") || !query.contains("*")) && (location != 11)) { + if (!query.contains("?") && !query.contains("*") && (location != 11)) { logInfo("Using full match"); pattern = SearchPattern.R_EXACT_MATCH | SearchPattern.R_CASE_SENSITIVE; }
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
394-395: Replace stdout with structured logging.Prefer
logInfooverSystem.out.println.- System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName());
362-364: Wildcard detection misses uppercase/static on-demand imports.The gate regex excludes uppercase segments and static imports (e.g., java.util.Collections.*). Update to accept both.
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + // Accept package or type-qualified wildcard imports, with optional 'static' prefix + Matcher matcher = Pattern.compile("^(?:static\\s+)?[\\w$.]+\\.\\*$").matcher(query);
🧹 Nitpick comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
240-241: Possible duplicate units → duplicate symbols; deduplicate compilation units.Units are added from included fragments and again from all source fragments in-scope. Dedup to avoid duplicate AST work and duplicate SymbolInformation.
Apply:
- List<ICompilationUnit> units = new ArrayList<>(); + java.util.Set<ICompilationUnit> units = new java.util.LinkedHashSet<>();- if (fragment.getKind() == IPackageFragmentRoot.K_SOURCE) { - units.addAll(List.of(fragment.getCompilationUnits())); - } + if (fragment.getKind() == IPackageFragmentRoot.K_SOURCE) { + for (ICompilationUnit cu : fragment.getCompilationUnits()) { + units.add(cu); + } + }No loop changes needed below; the enhanced-for over Set still works.
Also applies to: 306-313, 366-377
379-399: Minor: pre-validated regex but still guard PatternSyntaxException.While the gate regex reduces risk, compiling arbitrary queries can still throw. Wrap compile in try/catch to log and skip AST scan instead of failing the whole search.
- Pattern regex = Pattern.compile(query); + Pattern regex; + try { + regex = Pattern.compile(query); + } catch (Exception e) { + logInfo("Skipping import scan; invalid wildcard pattern: " + e); + regex = null; + } for (ICompilationUnit unit : units) { + if (regex == null) break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(10 hunks)
🔇 Additional comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
412-419: Good reuse of JDTUtils for precise Location.Using
JDTUtils.toLocationsimplifies and aligns with existing URI handling. LGTM.
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
362-363: Fix the regex pattern to match actual package imports.The pattern
[^A-Z*]+\\*excludes uppercase letters, which means it will never match real Java package imports likejavax.persistence.*orjakarta.persistence.*since they contain uppercase letters. This defeats the entire purpose of the PR, which is to detectjavax.persistence.*imports.Based on past review feedback, apply this fix:
- Matcher matcher = Pattern.compile("[^A-Z*]+\\*").matcher(query); + // Match package or type-qualified wildcard imports, e.g., "javax.persistence.*" + Matcher matcher = Pattern.compile("^[\\w$.]+\\.\\*$").matcher(query);This pattern matches identifiers (word characters, dots, dollar signs) followed by
.*, which correctly matches bothjavax.persistence.*andjakarta.persistence.*.
392-392: Replace debug print with proper logging.The
System.out.printlnstatement should use the project's logging facility.Apply this fix:
- System.out.println("Found in " + unit.getElementName()); + logInfo("Found on-demand import in " + unit.getElementName());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(10 hunks)
🔇 Additional comments (6)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (6)
8-8: LGTM!The new imports support the on-demand import scanning functionality (regex matching, AST parsing, and LSP Location construction).
Also applies to: 18-21, 29-29, 31-31, 35-35, 37-37
306-314: LGTM!The logic correctly collects compilation units from source package fragments within the included paths for subsequent import scanning.
332-333: LGTM!Moving the symbols list initialization earlier allows accumulation of results from both the standard JDT search and the new import scanning logic.
410-417: LGTM!The helper method correctly delegates to
JDTUtils.toLocationfor converting import declarations to LSP Locations, with appropriate error handling. This addresses the incompleteness flagged in previous reviews.
377-391: LGTM!The AST-based import scanning logic correctly:
- Compiles the regex pattern once before the loops (addressing past review feedback)
- Parses each compilation unit to access imports
- Creates
SymbolInformationentries withNamespacekind for on-demand imports- Uses the helper method to compute LSP Locations
- Adds matching symbols to the results list
Note: Ensure the critical regex fix (lines 362-363) and the redundant collection fix (lines 364-375) are applied for this logic to work correctly.
Also applies to: 393-398
401-406: LGTM!The log statement formatting and return of the accumulated symbols list are correct.
| // Get all compilation units in scope | ||
| for (IJavaProject p : targetProjects) { | ||
| for (IPackageFragment pkg : p.getPackageFragments()) { | ||
| if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) { | ||
| for (ICompilationUnit unit : pkg.getCompilationUnits()) { | ||
| if (scope.encloses(unit)) { | ||
| units.add(unit); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid redundant compilation unit collection.
When includedPaths is specified, compilation units are already collected from included fragments at lines 310-313. The code then re-collects all units from all target projects (lines 364-375), creating duplicates and wasting resources.
Apply this fix to collect units only when not already gathered:
- // Get all compilation units in scope
- for (IJavaProject p : targetProjects) {
- for (IPackageFragment pkg : p.getPackageFragments()) {
- if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) {
- for (ICompilationUnit unit : pkg.getCompilationUnits()) {
- if (scope.encloses(unit)) {
- units.add(unit);
- }
- }
- }
- }
- }
+ // Get all compilation units in scope (if not already collected via included paths)
+ if (units.isEmpty()) {
+ for (IJavaProject p : targetProjects) {
+ for (IPackageFragment pkg : p.getPackageFragments()) {
+ if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) {
+ for (ICompilationUnit unit : pkg.getCompilationUnits()) {
+ if (scope.encloses(unit)) {
+ units.add(unit);
+ }
+ }
+ }
+ }
+ }
+ }📝 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.
| // Get all compilation units in scope | |
| for (IJavaProject p : targetProjects) { | |
| for (IPackageFragment pkg : p.getPackageFragments()) { | |
| if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) { | |
| for (ICompilationUnit unit : pkg.getCompilationUnits()) { | |
| if (scope.encloses(unit)) { | |
| units.add(unit); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // Get all compilation units in scope (if not already collected via included paths) | |
| if (units.isEmpty()) { | |
| for (IJavaProject p : targetProjects) { | |
| for (IPackageFragment pkg : p.getPackageFragments()) { | |
| if (pkg.getKind() == IPackageFragmentRoot.K_SOURCE) { | |
| for (ICompilationUnit unit : pkg.getCompilationUnits()) { | |
| if (scope.encloses(unit)) { | |
| units.add(unit); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
around lines 364-375, the code re-collects compilation units from all target
projects even when includedPaths already added units earlier, producing
duplicates and wasting resources; change the logic to skip this full-project
collection when units were already gathered from included fragments (e.g., check
a flag or whether includedPaths is non-null/non-empty or units is non-empty) so
the loop over all package fragments only runs if no units were previously
collected; optionally ensure the collection uses a Set or deduplication if you
must merge sources.
* Do special search for on-demand imports Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Calculate compilation units only if there is a * import search Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Improvements Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Fix and simplify getLocationForImport Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Add missing import Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Do not addAll and add explanatory comment Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
* Do special search for on-demand imports * Calculate compilation units only if there is a * import search * Improvements * Fix and simplify getLocationForImport * Add missing import * Do not addAll and add explanatory comment --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
4 participants
https://issues.redhat.com/browse/MTA-4027
Fixes konveyor/rulesets#272
Summary by CodeRabbit