🐛 Fixing annotation searching by getting the FQDN from the compliation. #163
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Falls back to trying to use the code actions to get the FQDN Signed-off-by: Shawn Hurley <[email protected]>
|
Warning Rate limit exceeded@jmle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a fast-path in AnnotationSymbolProvider to resolve an annotation element's fully-qualified type name (FQDN) via compilation unit or codeSelect, match it against the query (exact or regex), and short-circuit when matched; falls back to existing AST-based resolution if FQDN resolution fails. Changes
Sequence Diagram(s)sequenceDiagram
participant Query as Query
participant Provider as AnnotationSymbolProvider
participant CU as CompilationUnit
participant CodeSel as codeSelect (IType)
participant AST as AST-based resolver
Query->>Provider: request annotation symbol for element
Provider->>CU: inspect compilation unit for type binding
alt CU yields FQDN
CU-->>Provider: FQDN
else CU unresolved
Provider->>CodeSel: codeSelect to find IType
CodeSel-->>Provider: IType / FQDN or null
end
Provider->>Provider: match FQDN (exact or regex)?
alt match
Provider--xQuery: return symbol (close working copy)
else no match
Provider->>AST: fallback to AST-based resolution
AST-->>Provider: resolved symbol(s)
Provider-->>Query: return symbol(s)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 (6)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (6)
62-71: Minor: avoid Arrays.asList + var; tighten loop and break earlyUse a direct for‑each over the array and explicit types for readability and earlier exit.
- var elements = unit.codeSelect(match.getOffset(), match.getLength()); - for (IJavaElement e: Arrays.asList(elements)) { - if (e instanceof IType) { - var newT = (IType) e; - if (newT.isResolved()) { - fqdn = newT.getFullyQualifiedName('.'); - logInfo("FQDN from code select: " + fqdn); - } - } - } + IJavaElement[] elements = unit.codeSelect(match.getOffset(), match.getLength()); + for (IJavaElement e : elements) { + if (e instanceof IType) { + IType newT = (IType) e; + if (newT.isResolved()) { + fqdn = newT.getFullyQualifiedName('.'); + logInfo("FQDN from code select: " + fqdn); + break; + } + } + }
141-144: Return empty list instead of null on failureReturning null forces callers to null‑check; prefer an empty list.
- return null; + return List.of();
49-58: Optional: ensure cleanup on all paths with try/finallyTo make leaks impossible, wrap the working‑copy usage in try/finally and remove per‑branch closes.
- if (this.query.contains(".")) { + if (this.query.contains(".")) { + ICompilationUnit unit = null; + try { // First try to get compilation unit for source files - ICompilationUnit unit = (ICompilationUnit) annotationElement.getAncestor(IJavaElement.COMPILATION_UNIT); + 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); } } + // ... existing logic ... + } finally { + if (unit != null && unit.isWorkingCopy()) { + unit.discardWorkingCopy(); + unit.close(); + } + }
51-58: Verify source/target level supportsvarLocal variable type inference requires Java 10+. Ensure the module’s source/target is >= 10 or replace with explicit types.
87-121: Resilience: enable recovery on AST parsingWhen falling back to AST, enabling bindings/statements recovery improves robustness with broken code (common in LSP workspaces).
- astParser.setResolveBindings(true); + astParser.setResolveBindings(true); + astParser.setBindingsRecovery(true); + astParser.setStatementsRecovery(true);
31-35: Follow‑up: regression test for issue #162Please add an integration test covering the demo’s
OrderManagementAppInitializer.javacase to prevent future regressions (FQDN resolution via codeSelect path).Also applies to: 147-159
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
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/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
Comment on lines
59
to
75
| IType t = unit.getType(annotationElement.getElementName()); | ||
| String fqdn = ""; | ||
| if (!t.isResolved()) { | ||
| var elements = unit.codeSelect(match.getOffset(), match.getLength()); | ||
| for (IJavaElement e: Arrays.asList(elements)) { | ||
| if (e instanceof IType) { | ||
| var newT = (IType) e; | ||
| if (newT.isResolved()) { | ||
| fqdn = newT.getFullyQualifiedName('.'); | ||
| logInfo("FQDN from code select: " + fqdn); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| fqdn = t.getFullyQualifiedName('.'); | ||
| logInfo("resolved type: " + fqdn); | ||
| } |
There was a problem hiding this comment.
Guard against null unit/handle existence; prefer codeSelect-first for FQDN
unit can still be null (no CU/class file), and unit.getType(simpleName) rarely resolves for annotations defined elsewhere. Also call sites on a non‑existent handle can mislead. Guard unit, use exists(), and favor codeSelect first.
- IType t = unit.getType(annotationElement.getElementName());
- String fqdn = "";
- if (!t.isResolved()) {
- var elements = unit.codeSelect(match.getOffset(), match.getLength());
- for (IJavaElement e: Arrays.asList(elements)) {
- if (e instanceof IType) {
- var newT = (IType) e;
- if (newT.isResolved()) {
- fqdn = newT.getFullyQualifiedName('.');
- logInfo("FQDN from code select: " + fqdn);
- }
- }
- }
- } else {
- fqdn = t.getFullyQualifiedName('.');
- logInfo("resolved type: " + fqdn);
- }
+ String fqdn = "";
+ IType t = null;
+ if (unit != null) {
+ // Prefer codeSelect: the annotation type is usually external to this CU.
+ IJavaElement[] sel = unit.codeSelect(match.getOffset(), match.getLength());
+ for (IJavaElement e : sel) {
+ if (e.getElementType() == IJavaElement.TYPE) {
+ IType newT = (IType) e;
+ if (newT.isResolved()) {
+ fqdn = newT.getFullyQualifiedName('.');
+ logInfo("FQDN from code select: " + fqdn);
+ break;
+ }
+ }
+ }
+ // Fallback: try CU-local handle only if present and resolved.
+ if (fqdn.isEmpty()) {
+ t = unit.getType(annotationElement.getElementName());
+ if (t != null && t.exists() && t.isResolved()) {
+ fqdn = t.getFullyQualifiedName('.');
+ logInfo("resolved type: " + fqdn);
+ }
+ }
+ }🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
around lines 59 to 75, guard against a null or non-existing ICompilationUnit
before calling methods on it, prefer using codeSelect first to resolve the
annotation FQDN (since unit.getType(simpleName) often fails for annotations
defined elsewhere), check unit.exists() and skip codeSelect/getType if the unit
is null or doesn't exist, iterate codeSelect results and return the first
resolved IType FQDN if present, and only if codeSelect yields nothing attempt
unit.getType(simpleName) with a null/exists check and use its
getFullyQualifiedName('.') as a fallback; add concise logs for each resolution
path and ensure no method is invoked on a null/non-existing unit.
There was a problem hiding this comment.
@shawn-hurley this has a point, we should guard against nil just in case
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java
Outdated
Show resolved
Hide resolved
jmle
requested changes
Oct 22, 2025
Comment on lines
59
to
75
| IType t = unit.getType(annotationElement.getElementName()); | ||
| String fqdn = ""; | ||
| if (!t.isResolved()) { | ||
| var elements = unit.codeSelect(match.getOffset(), match.getLength()); | ||
| for (IJavaElement e: Arrays.asList(elements)) { | ||
| if (e instanceof IType) { | ||
| var newT = (IType) e; | ||
| if (newT.isResolved()) { | ||
| fqdn = newT.getFullyQualifiedName('.'); | ||
| logInfo("FQDN from code select: " + fqdn); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| fqdn = t.getFullyQualifiedName('.'); | ||
| logInfo("resolved type: " + fqdn); | ||
| } |
There was a problem hiding this comment.
@shawn-hurley this has a point, we should guard against nil just in case
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.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: 0
♻️ Duplicate comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
89-96: Guard AST fallback when unit is null/non‑existent; pass this.query to visitor.Prevents NPE/IAE on setSource and aligns the query used by the visitor.
- if (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()); - astParser.setSource(unit); - astParser.setResolveBindings(true); - CompilationUnit cu = (CompilationUnit) astParser.createAST(null); - CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.ANNOTATION); + String normalizedForAst = this.query.replaceAll("\\(([A-Za-z_][A-Za-z0-9_]*(\\|[A-Za-z_][A-Za-z0-9_]*)*)\\)", ".*"); + if (this.queryQualificationMatches(normalizedForAst, annotationElement, unit, location)) { + if (unit != null && unit.exists()) { + ASTParser astParser = ASTParser.newParser(AST.getJLSLatest()); + astParser.setSource(unit); + astParser.setResolveBindings(true); + CompilationUnit cu = (CompilationUnit) astParser.createAST(null); + CustomASTVisitor visitor = new CustomASTVisitor(this.query, match, QueryLocation.ANNOTATION); + } else { + logInfo("Skipping AST fallback: no compilation unit available"); + }
59-85: Fix FQDN path: guard unit, prefer codeSelect, correct regex direction, normalize wildcards, and honor annotationQuery before early return.
- Avoid NPEs: check unit.exists().
- Prefer codeSelect over unit.getType(simpleName).
- Use fqdn.equals(query) or fqdn.matches(normalizedQuery); current query.matches(fqdn) is inverted.
- Normalize '' to '.' (same rule as CustomASTVisitor) for consistency.
- Apply matchesAnnotationQuery before adding/returning; current early return skips element filtering.
Apply:
- if (unit != null) { - IType t = unit.getType(annotationElement.getElementName()); - String fqdn = ""; - if (!t.isResolved()) { - var elements = unit.codeSelect(match.getOffset(), match.getLength()); - for (IJavaElement e: Arrays.asList(elements)) { - if (e instanceof IType) { - var newT = (IType) e; - if (newT.isResolved()) { - fqdn = newT.getFullyQualifiedName('.'); - logInfo("FQDN from code select: " + fqdn); - } - } - } - } else { - fqdn = t.getFullyQualifiedName('.'); - logInfo("resolved type: " + fqdn); - } - if (query.matches(fqdn) || fqdn.matches(query)) { - if (unit.isWorkingCopy()) { - unit.discardWorkingCopy(); - unit.close(); - } - symbols.add(symbol); - return symbols; - } - } + if (unit != null && unit.exists()) { + String fqdn = ""; + try { + IJavaElement[] sel = unit.codeSelect(match.getOffset(), match.getLength()); + for (IJavaElement e : sel) { + if (e.getElementType() == IJavaElement.TYPE) { + IType newT = (IType) e; + if (newT.isResolved()) { + fqdn = newT.getFullyQualifiedName('.'); + logInfo("FQDN from code select: " + fqdn); + break; + } + } + } + } catch (Exception ignore) { + // fall through to getType fallback + } + if (fqdn.isEmpty()) { + IType t = unit.getType(annotationElement.getElementName()); + if (t != null && t.exists() && t.isResolved()) { + fqdn = t.getFullyQualifiedName('.'); + logInfo("resolved type: " + fqdn); + } + } + // Normalize wildcards like in CustomASTVisitor + String normalized = this.query.replaceAll("(?<!\\.)\\*", ".*"); + if (!fqdn.isEmpty() && (fqdn.equals(this.query) || fqdn.matches(normalized))) { + // Respect element/value filters when present + 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)) { + // no early return; fall back to AST if needed + } else { + symbols.add(symbol); + if (unit.isWorkingCopy()) { + unit.discardWorkingCopy(); + unit.close(); + } + return symbols; + } + } else { + symbols.add(symbol); + if (unit.isWorkingCopy()) { + unit.discardWorkingCopy(); + unit.close(); + } + return symbols; + } + } + }
🧹 Nitpick comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
64-72: Avoid Arrays.asList on arrays in tight loops.Iterate the array directly to reduce allocation.
- var elements = unit.codeSelect(match.getOffset(), match.getLength()); - for (IJavaElement e: Arrays.asList(elements)) { + IJavaElement[] elements = unit.codeSelect(match.getOffset(), match.getLength()); + for (IJavaElement e : elements) {
143-146: Return empty list on error instead of null.Prevents NPEs in callers expecting a collection.
- return null; + return new ArrayList<>();
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java (2)
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/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 <[email protected]>
jmle
approved these changes
Oct 22, 2025
pranavgaikwad
approved these changes
Oct 22, 2025
jmle
added a commit
to jmle/java-analyzer-bundle
that referenced
this pull request
Oct 22, 2025
…n. (konveyor#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 <[email protected]> * Couple of fixes Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Respect annotated feature Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> --------- Signed-off-by: Shawn Hurley <[email protected]> Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]>
jmle
added a commit
that referenced
this pull request
Oct 22, 2025
* 🐛 Fix annotation matching (#149) * tmp Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Working: Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Move in check for annotated classes Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * tmp Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Fix stuff Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Final fix Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Fix shouldVisit Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Fix condition Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Undo Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * make sure that qualification matches considers fqn usage without import and queries like (A|B|C) Signed-off-by: Pranav Gaikwad <[email protected]> * fix a minor bug in processing regex Signed-off-by: Pranav Gaikwad <[email protected]> * Fix regex Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Fix potential npe Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Pranav Gaikwad <[email protected]> Co-authored-by: Pranav Gaikwad <[email protected]> Signed-off-by: Cherry Picker <[email protected]> * 🐛 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 <[email protected]> * Couple of fixes Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> * Respect annotated feature Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> --------- Signed-off-by: Shawn Hurley <[email protected]> Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Pranav Gaikwad <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Signed-off-by: Shawn Hurley <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Pranav Gaikwad <[email protected]> Co-authored-by: Shawn Hurley <[email protected]>
This was referenced Oct 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #162
Summary by CodeRabbit