🐛 Fix annotation matching #149
Conversation
WalkthroughAdds dot-qualified query support for annotations: AnnotationSymbolProvider now implements WithQuery with a settable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ASP as AnnotationSymbolProvider
participant Annotation
participant Unit as Source/Class Unit
participant Parser as ASTParser
participant Visitor as CustomASTVisitor
Caller->>ASP: setQuery(query)
Caller->>ASP: requestSymbols(annotation)
ASP->>Annotation: annotation.getPrimaryElement()
ASP->>ASP: determine kind & location
alt query contains dot (qualified)
ASP->>Unit: resolve source or class-file working copy
ASP->>Parser: configure with bindings
Parser->>Visitor: parse & visit (ANNOTATION)
Visitor-->>ASP: symbolMatches?
note right of Parser #f9f5f0: parse problems logged (non-fatal)
ASP->>Unit: discard/close working copy
alt symbolMatches
ASP-->>Caller: emit symbol
else
ASP-->>Caller: skip symbol
end
else (non-qualified)
ASP->>ASP: compare annotationQuery or accept symbol
alt matches
ASP-->>Caller: emit symbol
else
ASP-->>Caller: skip symbol
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
📜 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/symbol/AnnotationSymbolProvider.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(22-192)
⏰ 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 (4)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (4)
9-9: LGTM!The imports and interface additions appropriately support the new qualified query handling and AST parsing functionality.
Also applies to: 14-20, 26-26, 29-29, 32-32
38-46: LGTM!Storing the annotatable reference and setting the location earlier are appropriate changes that support the subsequent qualified query logic.
58-77: LGTM with note on problem handling.The AST parser setup and visitor usage are correct. The code logs compilation problems but continues processing, which is appropriate as indicated by the comment at line 63 that bindings may return null under tests with problems.
124-127: LGTM!The
setQuerymethod correctly implements theWithQueryinterface requirement.
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
Show resolved
Hide resolved
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
Show resolved
Hide resolved
|
Looks like the CI failures are legitimate? The missing tags are from https://github.com/konveyor/rulesets/blob/main/default/generated/technology-usage/34-database-technology-usage.windup.yaml#L63. After a quick look at tackle-testapp, looks like these tags should be present. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
49-94: Guard working-copy lifecycle before cleanup.When no working copy is created,
unitcan benullor a primary CU. Unconditionally callingdiscardWorkingCopy()/close()throws (or NPEs). Track whether a working copy was opened and only discard/close in that case, skipping cleanup whenunitstayednullor primary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(25-274)
⏰ 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
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Outdated
Show resolved
Hide resolved
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/symbol/CustomASTVisitor.java(3 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 (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
94-94: FQN construction is correct.The past review concern about duplicated annotation names (e.g.,
com.example.MyAnnotation.MyAnnotation) has been properly addressed. The code now uses onlydeclaringClass.getQualifiedName()without appendingbinding.getName().
| this.symbolMatches = true; | ||
| return false; | ||
| } else { | ||
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); |
There was a problem hiding this comment.
Fix copy-paste errors in log messages.
The log messages reference "method" and "MethodInvocation" but should reference "annotation" since this is the annotation visitor:
- Line 100: "method fqn" → "annotation fqn"
- Line 105: "MethodInvocation" → "Annotation"
- Line 111: "MethodInvocation" → "Annotation"
Apply this diff:
- logInfo("method fqn " + fullyQualifiedName + " did not match with " + query);
+ logInfo("annotation fqn " + fullyQualifiedName + " did not match with " + query);- logInfo("failed to get accurate info for MethodInvocation, falling back");
+ logInfo("failed to get accurate info for Annotation, falling back");- logInfo("KONVEYOR_LOG: error visiting MethodInvocation node: " + e);
+ logInfo("KONVEYOR_LOG: error visiting Annotation node: " + e);Also applies to: 105-105, 111-111
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
around lines 100, 105 and 111, fix copy-paste errors in the log messages that
incorrectly reference "method" and "MethodInvocation": change the message at
line 100 from "method fqn" to "annotation fqn", and replace occurrences of
"MethodInvocation" at lines 105 and 111 with "Annotation" so the logs correctly
refer to annotations in this visitor.
dymurray
added
the
cherry-pick/release-0.8
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
Show resolved
Hide resolved
| classes.add(ResolvedSourceMethod.class); | ||
| classes.add(ResolvedSourceField.class); | ||
| classes.add(ResolvedSourceType.class); | ||
| if (matchesAnnotationQuery(match, classes)) { |
There was a problem hiding this comment.
is there a way to make the matchesAnnotationQuery be like a Go vardic function?
| if (binding != null) { | ||
| // get fqn | ||
| ITypeBinding declaringClass = binding.getAnnotationType(); | ||
| if (declaringClass != null) { |
There was a problem hiding this comment.
can we invert and return early so that we don't have nested depth of logic?
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
111-126: Fix copy‑paste log messages in annotation visitor.Messages should reference “annotation/Annotation”, not “method/MethodInvocation”.
- logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); + logInfo("annotation fqn " + fullyQualifiedName + " did not match with " + query);- logInfo("failed to get accurate info for MethodInvocation, falling back"); + logInfo("failed to get accurate info for Annotation, falling back");- logInfo("KONVEYOR_LOG: error visiting MethodInvocation node: " + e); + logInfo("KONVEYOR_LOG: error visiting Annotation node: " + e);
📜 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/symbol/CustomASTVisitor.java(3 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 (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
71-85: Good consolidation via shared Annotation path.Routing Marker/Normal/SingleMemberAnnotation to a single handler reduces duplication. LGTM.
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Outdated
Show resolved
Hide resolved
jmle
force-pushed
the
fix-annotation-matching
branch
from
cb5a3ba to
90ea336
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
115-127: Fix annotation log messages.Log lines still say “method fqn” / “MethodInvocation”, which is misleading in the annotation visitor. Please rename them to “annotation fqn” / “Annotation” so diagnostics stay accurate.
Apply this diff:
- logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); + logInfo("annotation fqn " + fullyQualifiedName + " did not match with " + query); ... - logInfo("failed to get accurate info for MethodInvocation, falling back"); + logInfo("failed to get accurate info for Annotation, falling back"); ... - logInfo("KONVEYOR_LOG: error visiting MethodInvocation node: " + e); + logInfo("KONVEYOR_LOG: error visiting Annotation node: " + e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(27-260)
⏰ 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
| if (this.query.contains(".")) { | ||
| // First try to get compilation unit for source files | ||
| ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); | ||
| if (unit == null) { | ||
| // If not in source, try to get class file for compiled classes | ||
| IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE); | ||
| if (cls != null) { | ||
| unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); | ||
| } | ||
| } | ||
| if (this.queryQualificationMatches(this.query, unit, location)) { | ||
| 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.ANNOTATION); | ||
| // Under tests, resolveConstructorBinding will return null if there are problems | ||
| IProblem[] problems = cu.getProblems(); | ||
| if (problems != null && problems.length > 0) { | ||
| logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling"); | ||
| int count = 0; | ||
| for (IProblem problem : problems) { | ||
| logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage()); | ||
| count++; | ||
| if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) { | ||
| logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " + (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed"); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| cu.accept(visitor); | ||
| if (visitor.symbolMatches()) { | ||
| if (annotationQuery != null) { | ||
| List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); | ||
| classes.add(ResolvedSourceMethod.class); | ||
| classes.add(ResolvedSourceField.class); | ||
| classes.add(ResolvedSourceType.class); | ||
| if (matchesAnnotationQuery(match, classes)) { | ||
| symbols.add(symbol); | ||
| } | ||
| } else { | ||
| symbols.add(symbol); | ||
| } | ||
| } | ||
| } | ||
| unit.discardWorkingCopy(); | ||
| unit.close(); |
There was a problem hiding this comment.
Guard compilation unit resolution before parsing.
If the match comes from an element without source and class-file ancestors (e.g., binary with no attached source), unit stays null. We then call queryQualificationMatches(this.query, unit, …) and later unit.discardWorkingCopy()/close(), which blows up with a NullPointerException. Even when unit is a primary compilation unit, unconditionally discarding the working copy is illegal. Please short-circuit when unit is null and only dispose when we actually created a working copy. Also guard this.query before invoking .contains.
Apply this diff:
- if (this.query.contains(".")) {
- // First try to get compilation unit for source files
- ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT);
- if (unit == null) {
- // If not in source, try to get class file for compiled classes
- IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE);
- if (cls != null) {
- unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null);
- }
- }
- if (this.queryQualificationMatches(this.query, unit, location)) {
+ if (this.query != null && this.query.contains(".")) {
+ ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT);
+ boolean isWorkingCopy = false;
+ if (unit == null) {
+ IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE);
+ if (cls != null) {
+ unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null);
+ isWorkingCopy = true;
+ }
+ }
+ if (unit != null && this.queryQualificationMatches(this.query, unit, location)) {
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.ANNOTATION);
// Under tests, resolveConstructorBinding will return null if there are problems
IProblem[] problems = cu.getProblems();
if (problems != null && problems.length > 0) {
logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling");
int count = 0;
for (IProblem problem : problems) {
logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage());
count++;
if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) {
logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " + (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed");
break;
}
}
}
cu.accept(visitor);
if (visitor.symbolMatches()) {
if (annotationQuery != null) {
List<Class<? extends SourceRefElement>> classes = new ArrayList<>();
classes.add(ResolvedSourceMethod.class);
classes.add(ResolvedSourceField.class);
classes.add(ResolvedSourceType.class);
if (matchesAnnotationQuery(match, classes)) {
symbols.add(symbol);
}
} else {
symbols.add(symbol);
}
}
}
- unit.discardWorkingCopy();
- unit.close();
+ if (unit != null) {
+ if (isWorkingCopy) {
+ unit.discardWorkingCopy();
+ }
+ unit.close();
+ }
} else {📝 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.
| if (this.query.contains(".")) { | |
| // First try to get compilation unit for source files | |
| ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); | |
| if (unit == null) { | |
| // If not in source, try to get class file for compiled classes | |
| IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE); | |
| if (cls != null) { | |
| unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); | |
| } | |
| } | |
| if (this.queryQualificationMatches(this.query, unit, location)) { | |
| 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.ANNOTATION); | |
| // Under tests, resolveConstructorBinding will return null if there are problems | |
| IProblem[] problems = cu.getProblems(); | |
| if (problems != null && problems.length > 0) { | |
| logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling"); | |
| int count = 0; | |
| for (IProblem problem : problems) { | |
| logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage()); | |
| count++; | |
| if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) { | |
| logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " + (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed"); | |
| break; | |
| } | |
| } | |
| } | |
| cu.accept(visitor); | |
| if (visitor.symbolMatches()) { | |
| if (annotationQuery != null) { | |
| List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); | |
| classes.add(ResolvedSourceMethod.class); | |
| classes.add(ResolvedSourceField.class); | |
| classes.add(ResolvedSourceType.class); | |
| if (matchesAnnotationQuery(match, classes)) { | |
| symbols.add(symbol); | |
| } | |
| } else { | |
| symbols.add(symbol); | |
| } | |
| } | |
| } | |
| unit.discardWorkingCopy(); | |
| unit.close(); | |
| if (this.query != null && this.query.contains(".")) { | |
| // First try to get compilation unit for source files | |
| ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); | |
| boolean isWorkingCopy = false; | |
| if (unit == null) { | |
| // If not in source, try to get class file for compiled classes | |
| IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE); | |
| if (cls != null) { | |
| unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); | |
| isWorkingCopy = true; | |
| } | |
| } | |
| if (unit != null && this.queryQualificationMatches(this.query, unit, location)) { | |
| 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.ANNOTATION); | |
| // Under tests, resolveConstructorBinding will return null if there are problems | |
| IProblem[] problems = cu.getProblems(); | |
| if (problems != null && problems.length > 0) { | |
| logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling"); | |
| int count = 0; | |
| for (IProblem problem : problems) { | |
| logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage()); | |
| count++; | |
| if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) { | |
| logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " + (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed"); | |
| break; | |
| } | |
| } | |
| } | |
| cu.accept(visitor); | |
| if (visitor.symbolMatches()) { | |
| if (annotationQuery != null) { | |
| List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); | |
| classes.add(ResolvedSourceMethod.class); | |
| classes.add(ResolvedSourceField.class); | |
| classes.add(ResolvedSourceType.class); | |
| if (matchesAnnotationQuery(match, classes)) { | |
| symbols.add(symbol); | |
| } | |
| } else { | |
| symbols.add(symbol); | |
| } | |
| } | |
| } | |
| if (unit != null) { | |
| if (isWorkingCopy) { | |
| unit.discardWorkingCopy(); | |
| } | |
| unit.close(); | |
| } | |
| } else { | |
| // ...rest of the else-branch... |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
around lines 47 to 93, guard against nulls and improper disposal: first check
that this.query is non-null before calling this.query.contains(...); after
trying to obtain a compilation unit from the element, short-circuit and skip
parsing/visiting if unit is null (so you don't call queryQualificationMatches or
create an AST on a null unit); track whether you created a working copy (only
call discardWorkingCopy() and close() when a working copy was actually created),
and avoid unconditionally discarding/closing primary compilation units to
prevent illegal operations. Ensure all null checks occur before any use of unit
so no NPEs occur.
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
…rt and queries like (A|B|C) Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
jmle
force-pushed
the
fix-annotation-matching
branch
from
90ea336 to
3c2c2d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
79-89: Extract duplicate annotation query matching logic.The annotation query matching logic (checking
annotationQuerynull, creating the classes list, callingmatchesAnnotationQuery, and conditionally adding symbols) is duplicated in both the qualified (lines 79-89) and non-qualified (lines 95-105) branches.Apply this diff to extract the duplicate logic:
+ private void addSymbolIfMatches(List<SymbolInformation> symbols, SymbolInformation symbol, SearchMatch match) { + if (annotationQuery != null) { + List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); + classes.add(ResolvedSourceMethod.class); + classes.add(ResolvedSourceField.class); + classes.add(ResolvedSourceType.class); + if (matchesAnnotationQuery(match, classes)) { + symbols.add(symbol); + } + } else { + symbols.add(symbol); + } + } // In the qualified branch: if (visitor.symbolMatches()) { - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { - symbols.add(symbol); - } + addSymbolIfMatches(symbols, symbol, match); } // In the non-qualified branch: - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { - symbols.add(symbol); - } + addSymbolIfMatches(symbols, symbol, match);Based on learnings
47-93: Critical: Address working copy lifecycle bug and NPE risks.This code has three critical issues previously flagged:
- NPE risk: Line 47 calls
this.query.contains(".")without null check; line 57 may passnullunit toqueryQualificationMatches.- Working copy lifecycle bug: Lines 92-93 unconditionally call
discardWorkingCopy()andclose()onunit, but this is only valid when a working copy was created at line 54. Whenunitcomes from line 49, these calls are incorrect.- Missing guard: If
unitis null after line 56, line 57 will cause issues.Apply this diff:
- if (this.query.contains(".")) { + if (this.query != null && this.query.contains(".")) { // First try to get compilation unit for source files ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); + boolean isWorkingCopy = false; if (unit == null) { // If not in source, try to get class file for compiled classes IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE); if (cls != null) { unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + isWorkingCopy = true; } } - if (this.queryQualificationMatches(this.query, annotationElement, unit, location)) { + if (unit != null && this.queryQualificationMatches(this.query, annotationElement, unit, location)) { 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.ANNOTATION); // Under tests, resolveConstructorBinding will return null if there are problems IProblem[] problems = cu.getProblems(); if (problems != null && problems.length > 0) { logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling"); int count = 0; for (IProblem problem : problems) { logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage()); count++; if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) { logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " + (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed"); break; } } } cu.accept(visitor); if (visitor.symbolMatches()) { if (annotationQuery != null) { List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); classes.add(ResolvedSourceMethod.class); classes.add(ResolvedSourceField.class); classes.add(ResolvedSourceType.class); if (matchesAnnotationQuery(match, classes)) { symbols.add(symbol); } } else { symbols.add(symbol); } } + if (isWorkingCopy) { + unit.discardWorkingCopy(); + unit.close(); + } } - unit.discardWorkingCopy(); - unit.close(); } else {Based on learnings
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (2)
111-111: Fix copy-paste errors in log messages.The log messages at lines 111, 116, and 122 incorrectly reference "method" and "MethodInvocation" but should reference "annotation" since this is the annotation visitor.
Apply this diff:
- logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); + logInfo("annotation fqn " + fullyQualifiedName + " did not match with " + query);- logInfo("failed to get accurate info for MethodInvocation, falling back"); + logInfo("failed to get accurate info for Annotation, falling back");- logInfo("KONVEYOR_LOG: error visiting MethodInvocation node: " + e); + logInfo("KONVEYOR_LOG: error visiting Annotation node: " + e);Based on learnings
86-94: Add proper annotation filtering to prevent false positives in annotation matching.The code currently lacks position-based filtering for annotation nodes. Unlike
MethodInvocationandConstructorInvocationwhich useshouldVisit()for position-based verification, thevisit(Annotation)method only checks location type without verifying the annotation node is at the actual search match position. This causes all annotations of a matching type to be flagged as matches, not just the one at the search offset.Add the helper method and apply the check as suggested:
+ private boolean shouldVisitAnnotation(Annotation node) { + org.eclipse.jdt.core.dom.Name name = node.getTypeName(); + if (name == null) { + return false; + } + int start = name.getStartPosition(); + int end = start + name.getLength(); + int m = this.match.getOffset(); + return m >= start && m < end; + } + private boolean visit(Annotation node) { - // There is a problem with trying to run shouldVisit() here because - // matches on annotations aren't directly on the annotation node, - // but on the annotated one (class, method, field, etc). So we can't - // use shouldVisit() to filter out nodes we don't want to visit. - // TODO: think of a better way to handle this if (this.location != QueryLocation.ANNOTATION) { return true; } + if (!shouldVisitAnnotation(node)) { + return true; + } try {
🧹 Nitpick comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
379-380: Good improvement to wildcard-to-regex conversion.The negative lookbehind correctly handles cases where the query already contains
.*, preventing double conversion. This is a clear improvement over using the raw query as a regex pattern.For additional robustness, consider escaping other regex metacharacters in the query before replacing wildcards:
-// when creating the regex, replace * with .* -Pattern regex = Pattern.compile(query.replaceAll("(?<!\\.)\\*", ".*")); +// when creating the regex, escape special chars then replace * with .* +String escaped = Pattern.quote(query).replace("\\*", "\\E.*\\Q"); +Pattern regex = Pattern.compile(escaped.replaceAll("^\\\\Q|\\\\E$", ""));This would prevent characters like
.from being interpreted as regex metacharacters, though the current implementation works fine for well-formed package names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(2 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/CustomASTVisitor.java(3 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(2 hunks)java-analyzer-bundle.tp/java-analyzer-bundle.target(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(27-256)
⏰ 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 (6)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
165-165: Correct fix for wildcard detection logic.The change from OR to AND is correct. The previous logic would incorrectly trigger full-match mode if a query contained only one wildcard type (e.g.,
foo*would match because it doesn't contain?). The new logic correctly uses pattern matching whenever any wildcard (?or*) is present.java-analyzer-bundle.tp/java-analyzer-bundle.target (2)
67-67: LSP4J 0.22.0 downgrade is required by JDT.LS 1.35.0.JDT.LS 1.35.0 specifies LSP4J 0.22.0 in its target platform configuration, making this downgrade necessary for dependency compatibility rather than optional. The change is correct and justified.
5-10: Verify the justification for both dependency downgrades: JDT.LS (1.40.0 → 1.35.0) and LSP4J (0.23.1 → 0.22.0).The downgrades lack documented justification in the PR. LSP4J 0.23.1 includes breaking API changes, which could explain the LSP4J downgrade, but no PR comments clarify this. JDT.LS 1.40.0 added Java 23 support and other features without documented regressions.
Confirm:
- Are these downgrades required to fix the annotation matching issues?
- Do the PR CI failures relate to these versions, or are they separate concerns?
- Have specific compatibility issues been identified that prevent using the newer versions?
Without this context, these changes appear to work around issues rather than fix root causes, potentially masking underlying bugs in annotation matching logic.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (1)
56-56: LGTM!The call to
queryQualificationMatchescorrectly passes themodelement as the second argument, aligning with the updated signature inSymbolProvider.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java (1)
56-56: LGTM!The call to
queryQualificationMatchescorrectly passes theeelement as the second argument, aligning with the updated signature inSymbolProvider.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (1)
201-201: The regex change is correct and intentional.The codebase contains explicit OR pattern handling in SampleDelegateCommandHandler.java (lines 81-94) that detects and processes queries with pipes in parentheses, such as
java.io.((FileWriter|FileReader|PrintStream|File|PrintWriter|RandomAccessFile))*. The regex change from\\(.*\\)to\\([^|]*\\)intentionally preserves these pipe-containing parentheses so they can be parsed by the OR pattern logic, while still removing simple parameter-style parentheses like(String)or(*). This is the correct behavior.
| // an element need not be imported if its referenced by fqn | ||
| if (!queryQualification.isEmpty() && ( | ||
| matchedElement.getElementName().equals(queryQualification) | ||
| || matchedElement.getElementName().startsWith(queryQualification + ".") | ||
| || queryQualification.matches(matchedElement.getElementName()) | ||
| )) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Guard against null matchedElement and fix backwards regex match.
Two issues:
matchedElementcould be null (no prior null check), causing NPEs at lines 211-213.- Line 213 has the match backwards:
queryQualification.matches(matchedElement.getElementName())treats the query as a string and the element name as a regex pattern, which is the opposite of the intended logic.
Apply this diff:
// an element need not be imported if its referenced by fqn
- if (!queryQualification.isEmpty() && (
- matchedElement.getElementName().equals(queryQualification)
- || matchedElement.getElementName().startsWith(queryQualification + ".")
- || queryQualification.matches(matchedElement.getElementName())
- )) {
+ if (matchedElement != null && !queryQualification.isEmpty() && (
+ matchedElement.getElementName().equals(queryQualification)
+ || matchedElement.getElementName().startsWith(queryQualification + ".")
+ || matchedElement.getElementName().matches(queryQualification)
+ )) {
return true;
}🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java
around lines 209 to 216, guard against a null matchedElement and correct the
regex match direction: first check matchedElement != null before accessing
getElementName(), and replace
queryQualification.matches(matchedElement.getElementName()) with
matchedElement.getElementName().matches(queryQualification) (or otherwise
null-safe call) so the element name is matched against the query pattern; keep
the existing equality/startsWith checks but ensure all getElementName()
invocations occur only after the null check.
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 (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (2)
224-226: String comparisons use!= ""and may NPE onlocation.Use
isEmpty()and guardlocation.- if (queryQualification != "" && location.getUri().contains(queryQualification.replaceAll("\\.", "/"))) { + if (!queryQualification.isEmpty() && location != null + && location.getUri().contains(queryQualification.replaceAll("\\.", "/"))) { return true; } ... - if (packageQueryQualification!= "" && packageDecl.getElementName().matches(packageQueryQualification)) { + if (!packageQueryQualification.isEmpty() + && packageDecl.getElementName().matches(packageQueryQualification)) { return true; }Also applies to: 231-233
243-247: Reversed/duplicated regex checks on imports.Only the import string should be matched against the query pattern.
- // import can be absolute like java.io.paths.FileReader - if (query.matches(importElement)) { - return true; - } - if (importElement.matches(query)) { - return true; - } + // import can be absolute like java.io.paths.FileReader + if (importElement.matches(query)) { + return true; + }
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (1)
209-216: Null-guardmatchedElementand fix reversed regex.Accessing
matchedElementwithout a null-check can NPE. Also the regex is backwards; the element name should match the pattern.Apply:
- // an element need not be imported if its referenced by fqn - if (!queryQualification.isEmpty() && ( - matchedElement.getElementName().equals(queryQualification) - || matchedElement.getElementName().startsWith(queryQualification + ".") - || queryQualification.matches(matchedElement.getElementName()) - )) { - return true; - } + // an element need not be imported if it is referenced by FQN + if (matchedElement != null && !queryQualification.isEmpty()) { + String name = matchedElement.getElementName(); + if (name.equals(queryQualification) + || name.startsWith(queryQualification + ".") + || name.matches(queryQualification)) { + return true; + } + }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
47-56: Critical: guardquery/unitand fix working‑copy lifecycle; also sanitize query for AST.NPE risks on
this.query.contains,unitusage, and unconditionaldiscardWorkingCopy()/close(). Pass a sanitized query to the AST visitor so annotation FQNs (no params) match.- if (this.query.contains(".")) { + if (this.query != null && this.query.contains(".")) { // First try to get compilation unit for source files - ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); + ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); + boolean isWorkingCopy = false; if (unit == null) { // If not in source, try to get class file for compiled classes IClassFile cls = (IClassFile) annotationElement.getAncestor(IJavaElement.CLASS_FILE); if (cls != null) { - unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null); + isWorkingCopy = true; } } - if (this.queryQualificationMatches(this.query.replaceAll("\\(([A-Za-z_][A-Za-z0-9_]*(\\|[A-Za-z_][A-Za-z0-9_]*)*)\\)", ".*"), annotationElement, unit, location)) { + String sanitizedQuery = this.query.replaceAll("\\(([A-Za-z_][A-Za-z0-9_]*(\\|[A-Za-z_][A-Za-z0-9_]*)*)\\)", ".*"); + if (unit != null && this.queryQualificationMatches(sanitizedQuery, annotationElement, unit, location)) { 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.ANNOTATION); + CustomASTVisitor visitor = new CustomASTVisitor(sanitizedQuery, match, QueryLocation.ANNOTATION); // Under tests, resolveConstructorBinding will return null if there are problems- unit.discardWorkingCopy(); - unit.close(); + if (unit != null && isWorkingCopy) { + unit.discardWorkingCopy(); + unit.close(); + }Also applies to: 57-63, 92-93
🧹 Nitpick comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (1)
192-197: Avoid throwing/catching NPE for nullunit.Guard
unitbefore callingmakeConsistent.- try { - unit.makeConsistent(null); + try { + if (unit != null) { + unit.makeConsistent(null); + } } catch(Exception e) { - logInfo("unable to make unit consistant, will still try as could be class file in a jar" + e); + logInfo("unable to make unit consistent, proceeding: " + e); }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
79-89: DRY the duplicated annotationQuery filter into a helper.Extract once and call in both branches.
+ private void addSymbolIfMatches(List<SymbolInformation> symbols, SymbolInformation symbol, SearchMatch match) { + if (annotationQuery != null) { + List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); + classes.add(ResolvedSourceMethod.class); + classes.add(ResolvedSourceField.class); + classes.add(ResolvedSourceType.class); + if (matchesAnnotationQuery(match, classes)) { + symbols.add(symbol); + } + } else { + symbols.add(symbol); + } + } @@ - if (visitor.symbolMatches()) { - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { - symbols.add(symbol); - } - } + if (visitor.symbolMatches()) { + addSymbolIfMatches(symbols, symbol, match); + } @@ - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { - symbols.add(symbol); - } + addSymbolIfMatches(symbols, symbol, match);Also applies to: 95-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(27-256)
⏰ 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
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
47-47: Add null check forquerybefore calling.contains().If
this.queryisnullwhen this line executes, aNullPointerExceptionwill be thrown. Add a null guard before checking if it contains a dot.Apply this diff:
- if (this.query.contains(".")) { + if (this.query != null && this.query.contains(".")) {
57-61: Add null check forunitbefore qualification matching and AST parsing.If
annotationElementhas neither aCOMPILATION_UNITnor aCLASS_FILEancestor (or ifgetWorkingCopyreturnsnull),unitwill benull. Proceeding to callqueryQualificationMatchesand subsequently creating anASTParserwith a null source will cause aNullPointerException.Apply this diff:
} - 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)) { ASTParser astParser = ASTParser.newParser(AST.getJLSLatest());
🧹 Nitpick comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
79-89: Extract duplicate annotation query matching logic.The annotation query matching block (checking
annotationQuery != null, building the class list withResolvedSourceMethod,ResolvedSourceField, andResolvedSourceType, and callingmatchesAnnotationQuery) is duplicated in both the qualified (lines 79-89) and non-qualified (lines 97-107) branches.Note: This issue was previously raised in past review comments but remains unaddressed.
Apply this diff to extract the common logic:
+ private boolean shouldAddSymbol(SearchMatch match) { + if (annotationQuery != null) { + List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); + classes.add(ResolvedSourceMethod.class); + classes.add(ResolvedSourceField.class); + classes.add(ResolvedSourceType.class); + return matchesAnnotationQuery(match, classes); + } + return true; + } + @Override public List<SymbolInformation> get(SearchMatch match) throws CoreException { List<SymbolInformation> symbols = new ArrayList<>(); try { IAnnotatable annotatable = (IAnnotatable) match.getElement(); for (IAnnotation annotation : annotatable.getAnnotations()) { IJavaElement annotationElement = annotation.getPrimaryElement(); SymbolInformation symbol = new SymbolInformation(); symbol.setName(annotation.getElementName()); symbol.setKind(convertSymbolKind(annotationElement)); symbol.setContainerName(annotation.getParent().getElementName()); Location location = getLocation(annotationElement, match); symbol.setLocation(location); if (this.query.contains(".")) { // ... [compilation unit resolution and AST parsing] cu.accept(visitor); if (visitor.symbolMatches()) { - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { + if (shouldAddSymbol(match)) { symbols.add(symbol); } } } if (unit != null && unit.isWorkingCopy()) { unit.discardWorkingCopy(); unit.close(); } } else { - if (annotationQuery != null) { - List<Class<? extends SourceRefElement>> classes = new ArrayList<>(); - classes.add(ResolvedSourceMethod.class); - classes.add(ResolvedSourceField.class); - classes.add(ResolvedSourceType.class); - if (matchesAnnotationQuery(match, classes)) { - symbols.add(symbol); - } - } else { + if (shouldAddSymbol(match)) { symbols.add(symbol); } } }Also applies to: 97-107
📜 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/symbol/AnnotationSymbolProvider.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(27-256)
⏰ 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 (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
29-32: LGTM! Query interface implementation is correct.The addition of the
WithQueryinterface, thequeryfield, and thesetQuerymethod properly enables query-based filtering for annotations.Also applies to: 126-129
92-95: Good improvement on working copy lifecycle management.The addition of
unit.isWorkingCopy()check properly ensures that only working copies created viagetWorkingCopy()(line 54) are discarded, while primary compilation units obtained directly from ancestors (line 49) are not. This resolves the lifecycle issue raised in previous reviews.
* tmp Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Working: Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Move in check for annotated classes Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * tmp Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix stuff Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Final fix Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix shouldVisit Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix condition Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Undo Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * make sure that qualification matches considers fqn usage without import and queries like (A|B|C) Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * fix a minor bug in processing regex Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * Fix regex Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix potential npe Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> --------- Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> Co-authored-by: Pranav Gaikwad <pgaikwad@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com>
* 🐛 Fix annotation matching (#149) * tmp Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Working: Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Move in check for annotated classes Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * tmp Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix stuff Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Final fix Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix shouldVisit Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix condition Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Undo Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * make sure that qualification matches considers fqn usage without import and queries like (A|B|C) Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * fix a minor bug in processing regex Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * Fix regex Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Fix potential npe Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> --------- Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> Co-authored-by: Pranav Gaikwad <pgaikwad@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> * 🐛 Fixing annotation searching by getting the FQDN from the compliation. (#163) * Fixing annotation searching by getting the FQDN from the compliation. * Falls back to trying to use the code actions to get the FQDN Signed-off-by: Shawn Hurley <shawn@hurley.page> * Couple of fixes Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> * Respect annotated feature Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> --------- Signed-off-by: Shawn Hurley <shawn@hurley.page> Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> Co-authored-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> --------- Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: Shawn Hurley <shawn@hurley.page> Co-authored-by: Juan Manuel Leflet Estrada <jleflete@redhat.com> Co-authored-by: Pranav Gaikwad <pgaikwad@redhat.com> Co-authored-by: Shawn Hurley <shawn@hurley.page>
6 participants
Fixes https://issues.redhat.com/browse/MTA-6195 (konveyor/analyzer-lsp#901)
API tests PR: 351
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores