feat(filter): add depth control for dependency traversal #5268
Conversation
sofianedjerbi
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@sofianedjerbi is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds numeric depth syntax for graph traversal: lexer recognizes ellipses and numeric tokens adjacent to them; parser records per-direction depth into GraphExpression; evaluator enforces and propagates depth limits during traversal; docs and tests updated to exercise new syntax and behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Lexer
participant Parser
participant AST as GraphExpression
participant Evaluator
participant Traversal as GraphTraversal
rect rgb(240,248,255)
Lexer->>Parser: Tokenize input (IDENT / NUMBER / ELLIPSIS / PATH ...)
end
rect rgb(245,255,240)
Parser->>Parser: Detect depth tokens (N... / ...N), paths, carets
Parser->>AST: Construct GraphExpression (IncludeDependents/IncludeDependencies, DependentDepth, DependencyDepth)
Parser-->>Evaluator: Return parsed Expression
end
rect rgb(255,250,240)
Evaluator->>AST: Inspect depth fields
alt Dependency traversal requested
Evaluator->>Traversal: traverseDependencies(maxDepth=DependencyDepth||Max, originalDepth)
else Dependent traversal requested
Evaluator->>Traversal: traverseDependents(maxDepth=DependentDepth||Max, originalDepth)
end
Traversal->>Traversal: Recurse with (currentDepth-1, originalDepth) until limit or unlimited
Traversal-->>Evaluator: Return visited nodes
Evaluator-->>Parser: Return filtered result set
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-08-19T16:05:54.723ZApplied to files:
🧬 Code graph analysis (2)internal/filter/parser_test.go (2)
internal/filter/parser.go (2)
🔇 Additional comments (19)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/filter/evaluator.go (1)
313-320: Warning messages reference wrong depth value.The warning messages in both
traverseDependencies(line 316-317) andtraverseDependents(line 350-351) format the message withMaxTraversalDepthconstant instead of the actualmaxDepthparameter being used. When users specify a custom depth limit (e.g.,foo..2), the warning will incorrectly report 1000000 instead of 2.🔎 Proposed fix
if maxDepth <= 0 { if l != nil { l.Warnf( "Maximum dependency traversal depth (%d) reached for component %s during filtering. Some dependencies may have been excluded from results.", - MaxTraversalDepth, + maxDepth, c.Path(), ) }Apply the same fix to
traverseDependents(line 350-351):if maxDepth <= 0 { if l != nil { l.Warnf( "Maximum dependent traversal depth (%d) reached for component %s during filtering. Some dependents may have been excluded from results.", - MaxTraversalDepth, + maxDepth, c.Path(), ) }Also applies to: 348-355
🧹 Nitpick comments (1)
internal/filter/ast.go (1)
220-233: Consider extendingNewGraphExpressionto accept depth parameters.The constructor initializes the struct without the new
DependentDepthandDependencyDepthfields. While the zero-value default (unlimited traversal) is correct, callers now need to modify the struct directly after construction to set depth limits.For API consistency and discoverability, consider adding depth parameters to the constructor or creating an alternative constructor:
🔎 Possible constructor signature
func NewGraphExpressionWithDepth( target Expression, includeDependents bool, includeDependencies bool, excludeTarget bool, dependentDepth int, dependencyDepth int, ) *GraphExpressionAlternatively, keep the current constructor for backward compatibility and have the parser set the fields directly as it currently does.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/filter/ast.go(3 hunks)internal/filter/doc.go(1 hunks)internal/filter/evaluator.go(1 hunks)internal/filter/evaluator_test.go(1 hunks)internal/filter/lexer.go(5 hunks)internal/filter/lexer_test.go(1 hunks)internal/filter/parser.go(6 hunks)internal/filter/parser_test.go(2 hunks)internal/filter/token.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/filter/doc.gointernal/filter/parser_test.gointernal/filter/ast.gointernal/filter/evaluator_test.gointernal/filter/lexer_test.gointernal/filter/token.gointernal/filter/evaluator.gointernal/filter/lexer.gointernal/filter/parser.go
🧠 Learnings (1)
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/filter/ast.go
🧬 Code graph analysis (5)
internal/filter/parser_test.go (2)
internal/filter/ast.go (2)
GraphExpression(211-218)AttributeExpression(63-69)internal/filter/evaluator.go (1)
MaxTraversalDepth(26-26)
internal/filter/evaluator_test.go (4)
internal/component/unit.go (1)
NewUnit(46-54)internal/component/component.go (1)
Component(35-59)internal/filter/ast.go (2)
GraphExpression(211-218)AttributeExpression(63-69)internal/filter/evaluator.go (1)
Evaluate(40-61)
internal/filter/lexer_test.go (1)
internal/filter/token.go (6)
Token(73-77)ELLIPSIS(31-31)EOF(11-11)ELLIPSIS_DEPTH(32-32)PATH(17-17)IDENT(14-14)
internal/filter/lexer.go (1)
internal/filter/token.go (2)
NewToken(80-86)ELLIPSIS_DEPTH(32-32)
internal/filter/parser.go (2)
internal/filter/token.go (14)
ELLIPSIS(31-31)ELLIPSIS_DEPTH(32-32)PIPE(21-21)EQUAL(22-22)RBRACE(26-26)RBRACKET(28-28)CARET(33-33)EOF(11-11)ILLEGAL(8-8)IDENT(14-14)PATH(17-17)BANG(20-20)LBRACE(25-25)LBRACKET(27-27)internal/filter/evaluator.go (1)
MaxTraversalDepth(26-26)
🔇 Additional comments (17)
internal/filter/lexer_test.go (1)
130-196: Comprehensive test coverage for new ellipsis depth tokens.The new test cases thoroughly cover the
ELLIPSISandELLIPSIS_DEPTHtoken types including:
- Basic ellipsis and depth-annotated ellipsis tokens
- Edge cases like parent directory paths (
../foo) not being confused with depth syntax- Token boundary detection (
foo..1correctly splits into IDENT + ELLIPSIS_DEPTH)- Precedence rules (
...takes precedence over..N)The tests are well-structured and follow the existing patterns in the file.
internal/filter/doc.go (1)
73-92: Clear and comprehensive documentation for depth-limited traversal.The documentation effectively explains:
- Basic graph traversal operators (
...)- New depth-limited syntax (
..N)- Combined usage patterns (
..1foo..2)- Default unlimited traversal when depth is 0 or unspecified
The examples are practical and align with the implementation.
internal/filter/ast.go (1)
236-262: String() method correctly renders depth-limited expressions.The logic properly handles both depth-limited (
..N) and unlimited (...) forms for both dependent and dependency directions. The conditional rendering ensures round-trip consistency between parsing and string representation.internal/filter/token.go (2)
31-33: LGTM!The new
ELLIPSIS_DEPTHtoken is properly positioned with the other graph operators and follows the existing naming conventions.
63-64: Appropriate string representation for debugging.Using
"..N"as the string representation is clear for debugging purposes, while the actual depth value (e.g.,"..1","..25") is preserved inToken.Literal.internal/filter/parser_test.go (3)
632-704: Thorough test coverage for depth-limited graph expressions.The new test cases comprehensively cover:
- Single-direction depth limits (prefix and postfix)
- Bidirectional depth limits with different values
- Depth combined with caret (exclude target)
- Multi-digit depth values
The tests properly assert both
DependentDepthandDependencyDepthfields are populated correctly.
705-732: Good edge case coverage for depth overflow handling.The tests verify two important edge cases:
- Very large depth clamped to max (line 706-718): Values exceeding
MaxTraversalDepthare clamped rather than causing errors- Integer overflow falls back to unlimited (line 719-732): Values that overflow
intparsing result inDependentDepth=0(unlimited traversal)The overflow-to-unlimited fallback is a reasonable choice for robustness, though it may be worth documenting this behavior so users understand that extremely large values like
..99999999999999999999999don't error but instead behave as unlimited.Please confirm the overflow fallback behavior (using unlimited traversal when depth parsing fails) is the intended design. If a user typos a very large number, they might not realize they're getting unlimited traversal instead of an error.
748-753: Assertions updated to verify new depth fields.The assertions for
DependentDepthandDependencyDepthare properly added to validate the parsing of depth-limited expressions.internal/filter/evaluator_test.go (1)
978-1078: LGTM! Comprehensive test coverage for depth-limited graph traversal.The test suite thoroughly validates the new depth-control feature across both dependency and dependent directions, as well as the unlimited-depth case. The linear graph structure (a → b → c → d) provides clear, predictable traversal paths for verification.
internal/filter/lexer.go (3)
76-88: LGTM! Depth ellipsis token recognition is correctly implemented.The logic properly distinguishes between
...(unlimited) and..N(depth-limited) patterns, reading all consecutive digits and emitting the appropriate ELLIPSIS_DEPTH token.
183-190: LGTM! Consistent ellipsis boundary detection across all reading methods.The updates to
readIdentifier,readAttributeValue, andreadPathconsistently stop at both...and..Nboundaries, ensuring proper token separation.Also applies to: 208-215, 232-239
298-301: LGTM! Simple and correct digit detection helper.internal/filter/evaluator.go (1)
255-258: LGTM! Depth computation logic is correct.The evaluator properly uses
DependencyDepthandDependentDepthwhen specified, falling back toMaxTraversalDepthfor unlimited traversal (depth = 0).Also applies to: 268-271
internal/filter/parser.go (4)
75-86: LGTM! Prefix ellipsis depth parsing is correct.The parser properly handles both unlimited (
...) and depth-limited (..N) dependent traversal prefixes, extracting the depth value and storing it for GraphExpression construction.
136-147: LGTM! Postfix ellipsis depth parsing is correct.The parser properly handles both unlimited (
...) and depth-limited (..N) dependency traversal postfixes, extracting the depth value and storing it for GraphExpression construction.
150-159: LGTM! Depth fields correctly propagated to GraphExpression.The parsed depth values are properly set in the GraphExpression, enabling depth-controlled traversal in the evaluator.
175-192: LGTM! Robust depth extraction with appropriate bounds checking.The
parseEllipsisDepthfunction correctly:
- Validates minimum literal length
- Parses the numeric portion after ".."
- Returns 0 (unlimited) for invalid input or negative values
- Clamps excessively large values to MaxTraversalDepth
Edge cases are well-handled.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/filter/parser.go (1)
194-207: Inconsistent behavior between large and overflowing depth values.The function clamps values greater than
MaxTraversalDepthtoMaxTraversalDepth, but returns 0 (unlimited) for values that causestrconv.Atoito overflow. This creates inconsistent behavior:
999999999...foo→ depth clamped toMaxTraversalDepth99999999999999999999999...foo→ depth = 0 (unlimited)Consider either clamping overflow to
MaxTraversalDepthor returning an error. One approach:🔎 Suggested fix for consistent clamping
func parseDepth(literal string) int { depth, err := strconv.Atoi(literal) - if err != nil || depth < 0 { - return 0 + if err != nil { + // Overflow or invalid - clamp to max + return MaxTraversalDepth + } + + if depth < 0 { + return 0 } if depth > MaxTraversalDepth { return MaxTraversalDepth } return depth }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/filter/ast.go(4 hunks)internal/filter/complex_test.go(1 hunks)internal/filter/doc.go(1 hunks)internal/filter/lexer.go(5 hunks)internal/filter/lexer_test.go(1 hunks)internal/filter/parser.go(5 hunks)internal/filter/parser_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/filter/doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/filter/ast.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/filter/complex_test.gointernal/filter/lexer.gointernal/filter/parser_test.gointernal/filter/lexer_test.gointernal/filter/parser.go
🧬 Code graph analysis (3)
internal/filter/complex_test.go (3)
internal/filter/lexer.go (1)
NewLexer(18-23)internal/filter/parser.go (1)
NewParser(30-41)internal/discovery/discovery.go (1)
String(325-327)
internal/filter/parser_test.go (2)
internal/filter/ast.go (3)
GraphExpression(211-218)AttributeExpression(63-69)PathExpression(31-36)internal/filter/evaluator.go (1)
MaxTraversalDepth(26-26)
internal/filter/lexer_test.go (1)
internal/filter/token.go (5)
Token(70-74)ELLIPSIS(31-31)EOF(11-11)IDENT(14-14)PATH(17-17)
🔇 Additional comments (10)
internal/filter/lexer.go (1)
170-175: Ellipsis boundary detection is consistent across token readers.The logic correctly stops at
...(three consecutive dots) inreadIdentifier,readAttributeValue, andreadPath, while allowing..patterns (like..1orfoo..bar) to be captured as identifiers. This aligns with the PR objective where..Nsyntax is handled at the parser level, not the lexer level.Also applies to: 192-197, 213-218
internal/filter/complex_test.go (2)
79-83: Clarify intended behavior for parentheses in expressions.The test expects
1...(foo | bar)to parse as1...name=(foo | name=bar), treating(fooas the name value. Is this the intended behavior? Parentheses are not typically valid in filter names, and users might expect grouping semantics. Consider whether this should be an error case or if documentation should clarify this limitation.
10-102: Good test coverage for complex depth expressions.The test suite covers a comprehensive set of scenarios including intersections, negations, mixed unlimited/limited depth, paths, braces, attributes, and carets. The parallel test execution and use of table-driven tests follows Go best practices.
internal/filter/lexer_test.go (1)
130-226: Comprehensive lexer tests for depth and ellipsis tokenization.The new test cases thoroughly cover the tokenization rules for depth syntax:
- Ellipsis (
...) correctly produces ELLIPSIS token- Double dots with digits (
..1,..25) remain as IDENT- Parent directory paths (
../foo) correctly produce PATH- Mixed patterns (
foo..bar,foo..1) stay as single IDENT- Depth syntax patterns (
1...foo,foo...1,1...foo...2) correctly split into separate tokensThis provides good regression coverage for the new feature.
internal/filter/parser_test.go (3)
749-762: Verify overflow behavior is intentional.The test expects that an overflowing depth value (
99999999999999999999999) results inDependentDepth: 0(unlimited). This silent fallback could mask user input errors. Consider whether this should instead:
- Return an error for unparseable depth values
- Clamp to
MaxTraversalDepthlike other large values- Document this as intentional "unlimited" fallback behavior
662-873: Thorough test coverage for depth-limited graph expressions.The tests comprehensively cover:
- Basic depth prefixes and postfixes
- Multi-digit depths and clamping to
MaxTraversalDepth- Numeric directory disambiguation with escape hatches (
{1},name=1)- Alphanumeric identifiers correctly distinguished from depth values
This provides strong regression protection for the new depth feature.
1026-1042: Good error case coverage for incomplete depth expressions.The new error cases properly verify that malformed depth expressions (e.g.,
1...without target,1...with trailing space,1......2with no target between) are rejected with errors.internal/filter/parser.go (3)
75-88: Prefix depth parsing logic is correct.The logic properly handles both forms:
N...foo- numeric prefix followed by ellipsis...foo- simple ellipsis prefixThe
isPurelyNumericcheck ensures alphanumeric identifiers likefoo1...aren't mistakenly treated as depth prefixes.
179-192: LGTM - Simple and efficient numeric check.The
isPurelyNumericfunction correctly handles the empty string case and efficiently checks for ASCII digits without allocations. This is appropriate for the use case of detecting depth prefixes.
138-162: Postfix depth parsing and GraphExpression construction look correct.The logic properly:
- Detects postfix ellipsis and sets
includeDependencies- Optionally parses trailing numeric depth
- Constructs
GraphExpressionwith bothDependentDepthandDependencyDepthThe conditional wrapping in
GraphExpressiononly occurs when graph operators are present, preserving backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/filter/lexer.go (1)
170-175: Consider extracting the ellipsis boundary check into a helper function.The same ellipsis detection logic is repeated in
readIdentifier,readAttributeValue, andreadPath. Extracting this into a helper likeisLookingAtEllipsis()would reduce duplication and centralize the logic.🔎 Proposed refactor
+// isLookingAtEllipsis returns true if the lexer is positioned at the start of "...". +func (l *Lexer) isLookingAtEllipsis() bool { + return l.ch == '.' && l.peekChar() == '.' && + l.readPosition+1 < len(l.input) && l.input[l.readPosition+1] == '.' +} + // readIdentifier reads an identifier from the input. // Identifiers can contain letters, numbers, underscores, hyphens, dots, and other non-special chars. // This includes hidden files starting with a dot like .gitignore // Trailing whitespace is trimmed. func (l *Lexer) readIdentifier() string { position := l.position for isIdentifierChar(l.ch) { - // stop at ellipsis (...) - if l.ch == '.' && l.peekChar() == '.' { - if l.readPosition+1 < len(l.input) && l.input[l.readPosition+1] == '.' { - break - } + if l.isLookingAtEllipsis() { + break } l.readChar() }Apply the same simplification to
readAttributeValueandreadPath.Also applies to: 192-197, 213-218
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/filter/lexer.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/filter/lexer.go
🧠 Learnings (1)
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/filter/lexer.go
🔇 Additional comments (2)
internal/filter/lexer.go (2)
63-75: LGTM!The ellipsis detection correctly identifies "..." sequences and emits an ELLIPSIS token. The boundary check and fallthrough logic for ".." patterns (parent directory) are properly handled.
260-274: LGTM!The helper functions
isSpecialChar,isPathSeparator,isIdentifierChar,isAttributeValueChar, andisPathCharcorrectly define the character classification logic for the lexer. The previously flagged unusedisDigitfunction has been removed.
|
Failed on lint |
Fixed in 1102530 |
Adds ..N syntax to limit dependency/dependent traversal depth. For example, foo..1 only includes direct dependencies.
Move depth numbers outside the ellipsis for clearer parsing.
This avoids ambiguity with numeric directory names like "1" or "123".
Before: ..1foo..2 (is ..11 depth 11 or depth 1 + dir "1"?)
After: 1...foo...2 (unambiguous, ellipsis is always ...)
For numeric dirs, use braces or explicit attributes: {1}...1 or name=1...1
- Change complex_test.go package to filter_test (testpackage rule) - Add blank lines before statements in parser.go and evaluator_test.go
sofianedjerbi
force-pushed
the
feature/filter-depth-control
branch
from
1102530 to
f7dde26
Compare
|
Just updated PR to fix CI |
2 participants
Description
Fixes #5267.
Adds depth-limited traversal syntax by placing the depth number outside the ellipsis:
Existing
...syntax stays unchanged (unlimited traversal).Numeric directory disambiguation
Purely numeric tokens adjacent to
...are interpreted as depths. For numeric directory names, use escape hatches:TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added depth-limited graph traversal syntax for filter expressions:
N...target...N.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.