fix: handle glob patterns when expanded by the shell #583
Conversation
aaguiarz
changed the title
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces single-string test file handling with multi-pattern expansion: adds expandTestFilePatterns, switches the --tests flag to a StringArray, merges flag patterns with positional args, validates and expands globs/literals, emits precise errors for missing/invalid/no-match cases, adds unit tests and an example YAML, and updates .gitignore to ignore Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "fga model test CLI"
participant Expander as "expandTestFilePatterns"
participant FS as "Filesystem"
Note over CLI,Expander: New flow merges flags + positional args
User->>CLI: fga model test --tests *.fga.yaml [positional...]
CLI->>Expander: patterns (flags) + posArgs
alt patterns empty
Expander-->>CLI: error "no test files specified"
else
loop each input
Expander->>FS: check existence or Glob(pattern)
FS-->>Expander: matched files / not found / glob error
end
alt invalid glob
Expander-->>CLI: error "invalid glob pattern"
else no matches
Expander-->>CLI: error "no matching test files found"
else matches found
Expander-->>CLI: []files
CLI->>CLI: run tests on files
CLI-->>User: test results
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 0
🧹 Nitpick comments (5)
.gitignore (1)
38-38: Consider a more comprehensive ignore pattern.The current rule
.DS_Storeonly ignores the file at the repository root. macOS can create.DS_Storefiles in any directory.Apply this diff for better coverage:
-/tests/fixtures/identifiers -.DS_Store +/tests/fixtures/identifiers +**/.DS_Storecmd/model/test_test.go (2)
156-177: Consider reusing existing utilities and simplifying helper logic.The helper functions duplicate functionality available in
internal/slices.Contains(as shown in the code snippets). Additionally,containsSubstringat line 167 has redundant logic—checking if the string ends with the substring OR contains it anywhere, when thecontainscall already handles both cases.For
containsPath, consider using the existing utility:+import "github.com/openfga/cli/internal/slices" + -func containsPath(files []string, pathSubstring string) bool { - for _, file := range files { - if containsSubstring(file, pathSubstring) { - return true - } - } - return false -} - -func containsSubstring(s, substr string) bool { - return len(s) >= len(substr) && (s[len(s)-len(substr):] == substr || contains(s, substr)) -} - -func contains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} +func containsPath(files []string, pathSubstring string) bool { + for _, file := range files { + if strings.Contains(file, pathSubstring) { + return true + } + } + + return false +}Or even simpler, use the standard library's
strings.Containsdirectly in test assertions.
156-177: Address linter feedback.The linter flags minor style issues: missing periods in comments and missing blank lines before returns.
Apply this diff:
-// containsPath checks if any file in the slice contains the given path substring +// containsPath checks if any file in the slice contains the given path substring. func containsPath(files []string, pathSubstring string) bool { for _, file := range files { if containsSubstring(file, pathSubstring) { return true } } + return false } func containsSubstring(s, substr string) bool { return len(s) >= len(substr) && (s[len(s)-len(substr):] == substr || contains(s, substr)) } func contains(s, substr string) bool { for i := 0; i <= len(s)-len(substr); i++ { if s[i:i+len(substr)] == substr { return true } } + return false }cmd/model/test.go (2)
185-185: Fix line length violation.The flag usage description exceeds the 120-character limit (143 characters).
Apply this diff to break the line:
- modelTestCmd.Flags().StringArray("tests", []string{}, "Path or glob of YAML test files. Can be specified multiple times or use glob patterns") + modelTestCmd.Flags().StringArray("tests", []string{}, + "Path or glob of YAML test files. Can be specified multiple times or use glob patterns")
150-150: Optional: Address remaining linter feedback.The linter flags a few minor style issues:
- Dynamic error messages (lines 150, 171, 176): For user-facing CLI errors, the current approach is acceptable, but you could wrap static sentinel errors for consistency with project conventions if preferred
- Missing whitespace before range loop (line 154): Add a blank line before the loop
- Missing blank line before continue (line 158): Add a blank line before continue
These are low-priority style improvements that can be deferred if not aligned with project conventions.
Also applies to: 154-154, 158-158, 171-171, 176-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)cmd/model/test.go(3 hunks)cmd/model/test_test.go(1 hunks)example/subdir/simple.fga.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/model/test_test.go (1)
internal/slices/contains.go (1)
Contains(22-30)
🪛 GitHub Check: Lints
cmd/model/test_test.go
[failure] 176-176:
return with no blank line before (nlreturn)
[failure] 163-163:
return with no blank line before (nlreturn)
[failure] 156-156:
Comment should end in a period (godot)
cmd/model/test.go
[failure] 154-154:
missing whitespace above this line (no shared variables above range) (wsl_v5)
[failure] 158-158:
continue with no blank line before (nlreturn)
[failure] 185-185:
The line is 143 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 147-147:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 176-176:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("no test files found")" (err113)
[failure] 171-171:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("test file %s does not exist", pattern)" (err113)
[failure] 150-150:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("no test files specified")" (err113)
⏰ 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: Tests
🔇 Additional comments (5)
example/subdir/simple.fga.yaml (1)
1-23: LGTM!The test file structure is well-formed and provides a clear, minimal example for testing subdirectory glob patterns. The model, tuples, and test assertions are consistent with each other.
cmd/model/test_test.go (1)
10-154: LGTM!The test coverage is comprehensive and validates all key scenarios for pattern expansion including single files, multiple files, glob patterns, shell-expanded inputs, error cases, and edge cases. This provides strong confidence in the new functionality.
cmd/model/test.go (3)
39-39: LGTM!The changes correctly implement the fix for handling shell-expanded glob patterns:
- Allowing zero positional arguments (line 39) handles the case when all files come from the flag
- Switching to
StringArray(line 42) enables multiple--testsarguments- Delegating to
expandTestFilePatterns(line 58) centralizes the pattern expansion logicAlso applies to: 42-42, 57-61
138-180: LGTM!The
expandTestFilePatternsfunction correctly handles the core requirements:
- Combines flag patterns and positional arguments to support shell expansion (lines 143-147)
- Validates file existence before attempting glob expansion (lines 156-159)
- Expands glob patterns when files don't exist as literals (lines 162-172)
- Provides clear error messages for common failure modes
147-147: Linter warning is a false positive.The
appendAssignwarning at line 147 can be safely ignored. The code intentionally creates a new sliceallPatternsby combiningpatternsandposArgs, rather than mutatingpatternsin place. This is the correct approach.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/model/test.go (1)
145-191: Consider deduplicating file paths to avoid processing the same file multiple times.The function doesn't deduplicate paths, so if a file is matched by multiple patterns or appears in both flags and positional args, it will be processed multiple times. While this might be intentional in some cases, it's more likely to be unintended behavior that could confuse users.
Apply this diff to add deduplication:
func expandTestFilePatterns(patterns []string, posArgs []string) ([]string, error) { // Combine flag values and positional args // This handles shell expansion: when the shell expands ./example/*.fga.yaml to // ./example/file1.yaml ./example/file2.yaml, the first file goes to the --tests flag // and the rest end up as positional arguments allPatterns := make([]string, 0, len(patterns)+len(posArgs)) allPatterns = append(allPatterns, patterns...) allPatterns = append(allPatterns, posArgs...) if len(allPatterns) == 0 { return nil, errNoTestFilesSpecified } - fileNames := []string{} + // Use a map to track seen files and maintain order with a slice + seen := make(map[string]bool) + fileNames := []string{} for _, pattern := range allPatterns { // First, check if it's a literal file that exists if _, err := os.Stat(pattern); err == nil { - fileNames = append(fileNames, pattern) + if !seen[pattern] { + seen[pattern] = true + fileNames = append(fileNames, pattern) + } continue } // Otherwise, try to expand it as a glob pattern matches, err := filepath.Glob(pattern) if err != nil { return nil, fmt.Errorf("invalid tests pattern %s due to %w", pattern, err) } if len(matches) > 0 { - fileNames = append(fileNames, matches...) + for _, match := range matches { + if !seen[match] { + seen[match] = true + fileNames = append(fileNames, match) + } + } } else { // If glob didn't match and file doesn't exist, report error return nil, fmt.Errorf("%w: %s", errTestFileDoesNotExist, pattern) } } if len(fileNames) == 0 { return nil, errNoTestFilesFound } return fileNames, nil }cmd/model/test_test.go (1)
157-166: Optional: Consider more precise path matching.The
anyContainshelper uses substring matching, which could theoretically cause false positives if one filename is a substring of another (e.g., "file.yaml" would match "myfile.yaml"). However, given the distinctive test paths used in this test suite, this is unlikely to be an issue in practice.If you want more precision, you could match against the basename or use exact matching:
-// anyContains checks if any string in the slice contains the given substring. -func anyContains(slice []string, substr string) bool { +// anyContains checks if any string in the slice has a basename that contains the given substring. +func anyContains(slice []string, substr string) bool { for _, s := range slice { - if strings.Contains(s, substr) { + if strings.Contains(filepath.Base(s), substr) { return true } } return false }However, the current implementation is adequate for these tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/model/test.go(4 hunks)cmd/model/test_test.go(1 hunks)
🔇 Additional comments (4)
cmd/model/test.go (3)
34-38: LGTM!The sentinel error declarations follow Go best practices for error signaling and provide clear, specific error messages for different failure scenarios.
46-68: LGTM!The changes correctly handle the transition to accepting multiple file patterns:
- Allowing zero positional args makes sense since patterns can come from the
--testsflag- Using
GetStringArrayaligns with the new flag type- Proper error handling is in place
196-197: LGTM!The flag definition correctly uses
StringArrayto accept multiple patterns, and the description clearly communicates the new capability.cmd/model/test_test.go (1)
11-155: Excellent test coverage!The test suite comprehensively covers the key scenarios:
- Single and multiple file handling
- Glob pattern expansion
- Shell expansion simulation (flags vs positional args)
- Mixed glob and literal inputs
- Error conditions (non-existent files, no matches, invalid globs)
The tests effectively validate the intended behavior of handling both quoted glob patterns (CLI expands) and shell-expanded arguments.
|
I know I approved and merged this, but the more I think about it, the less I like it. This currently confuses the args of the e.g. when we say: This means:
When we do: in a directory with 4 files: This means:
Given that - I would rather either:
The problem (3) suffers is the same as this PR - I'm not sure how well the command args play with the flag args - as in: fga model test --store-id=abcde --debug ./*.fga.yamlWhat happens here? A user's expectation I think will be that it runs the tests on all the fga.yaml files in the directory - but not sure that's what will happen (prob they'll be taken as the args to the verbose flag?). We'll need to look into it. |
Description
What problem is being solved?
Fixes #582
How is it being solved?
Accepting multiple
--fileparameters so it properly handles wildcards expanded by the shell.What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit