Skip to content

Commit

Permalink
lsp: Support file ignore config (#620)
Browse files Browse the repository at this point in the history
Fixes: #589

This PR makes the filtering logic support a new rootDir prefix. When
set, this will be stripped from file paths before testing their ignore
status. I have tried to add tests to explain the new functionality.

Signed-off-by: Charlie Egan <charlie@styra.com>
  • Loading branch information
charlieegan3 authored Apr 4, 2024
1 parent 155d8f5 commit 9081abf
Show file tree
Hide file tree
Showing 7 changed files with 216 additions and 13 deletions.
8 changes: 5 additions & 3 deletions internal/lsp/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func updateParse(cache *Cache, uri string) (bool, error) {
return false, nil
}

func updateFileDiagnostics(ctx context.Context, cache *Cache, regalConfig *config.Config, uri string) error {
func updateFileDiagnostics(ctx context.Context, cache *Cache, regalConfig *config.Config, uri, rootDir string) error {
module, ok := cache.GetModule(uri)
if !ok {
// then there must have been a parse error
Expand All @@ -114,7 +114,9 @@ func updateFileDiagnostics(ctx context.Context, cache *Cache, regalConfig *confi

input := rules.NewInput(map[string]string{uri: contents}, map[string]*ast.Module{uri: module})

regalInstance := linter.NewLinter().WithInputModules(&input)
regalInstance := linter.NewLinter().
WithInputModules(&input).
WithRootDir(rootDir)

if regalConfig != nil {
regalInstance = regalInstance.WithUserConfig(*regalConfig)
Expand Down Expand Up @@ -186,7 +188,7 @@ func updateAllDiagnostics(ctx context.Context, cache *Cache, regalConfig *config

input := rules.NewInput(files, modules)

regalInstance := linter.NewLinter().WithInputModules(&input)
regalInstance := linter.NewLinter().WithInputModules(&input).WithRootDir(detachedURI)

if regalConfig != nil {
regalInstance = regalInstance.WithUserConfig(*regalConfig)
Expand Down
2 changes: 1 addition & 1 deletion internal/lsp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func (l *LanguageServer) StartDiagnosticsWorker(ctx context.Context) {
}

// otherwise, lint the file and send the diagnostics
err = updateFileDiagnostics(ctx, l.cache, l.loadedConfig, evt.URI)
err = updateFileDiagnostics(ctx, l.cache, l.loadedConfig, evt.URI, l.clientRootURI)
if err != nil {
l.logError(fmt.Errorf("failed to update file diagnostics: %w", err))
}
Expand Down
41 changes: 40 additions & 1 deletion internal/lsp/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ rules:
// workspace diagnostics are run, this test validates that the correct diagnostics are sent to the client in this
// scenario.
//
//nolint:gocognit,maintidx
//nolint:gocognit,maintidx,gocyclo
func TestLanguageServerMultipleFiles(t *testing.T) {
t.Parallel()

Expand All @@ -321,6 +321,7 @@ func TestLanguageServerMultipleFiles(t *testing.T) {
tempDir := t.TempDir()
authzRegoURI := fileURIScheme + tempDir + "/authz.rego"
adminsRegoURI := fileURIScheme + tempDir + "/admins.rego"
ignoredRegoURI := fileURIScheme + tempDir + "/ignored/foo.rego"

files := map[string]string{
"authz.rego": `package authz
Expand All @@ -339,6 +340,25 @@ import rego.v1
users = {"alice", "bob"}
`,
"ignored/foo.rego": `package ignored
foo = 1
`,
".regal/config.yaml": `
ignore:
files:
- ignored/*.rego
`,
}

err = os.MkdirAll(filepath.Join(tempDir, ".regal"), 0o755)
if err != nil {
t.Fatalf("failed to create .regal directory: %s", err)
}

err = os.MkdirAll(filepath.Join(tempDir, "ignored"), 0o755)
if err != nil {
t.Fatalf("failed to create ignored directory: %s", err)
}

for f, fc := range files {
Expand All @@ -359,6 +379,7 @@ users = {"alice", "bob"}

authzFileMessages := make(chan FileDiagnostics, 1)
adminsFileMessages := make(chan FileDiagnostics, 1)
ignoredFileMessages := make(chan FileDiagnostics, 1)
testHandler := func(_ context.Context, _ *jsonrpc2.Conn, req *jsonrpc2.Request) (result any, err error) {
if req.Method == "textDocument/publishDiagnostics" {
var requestData FileDiagnostics
Expand All @@ -376,6 +397,10 @@ users = {"alice", "bob"}
adminsFileMessages <- requestData
}

if requestData.URI == ignoredRegoURI {
ignoredFileMessages <- requestData
}

return struct{}{}, nil
}

Expand Down Expand Up @@ -459,6 +484,20 @@ users = {"alice", "bob"}
t.Fatalf("timed out waiting for admins.rego diagnostics to be sent")
}

select {
case requestData := <-ignoredFileMessages:
if requestData.URI != ignoredRegoURI {
t.Fatalf("expected diagnostics to be sent for ignored/foo.rego, got %s", requestData.URI)
}

// this file is ignored, so there should be no diagnostics
if len(requestData.Items) != 0 {
t.Fatalf("expected 0 diagnostics, got %d, %v", len(requestData.Items), requestData)
}
case <-time.After(3 * time.Second):
t.Fatalf("timed out waiting for ignored/foo.rego diagnostics to be sent")
}

// 3. Client sends textDocument/didChange notification with new contents for main.rego
// no response to the call is expected
err = connClient.Call(ctx, "textDocument/didChange", TextDocumentDidChangeParams{
Expand Down
21 changes: 15 additions & 6 deletions pkg/config/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@ import (
"github.com/open-policy-agent/opa/bundle"
)

func FilterIgnoredPaths(paths, ignore []string, checkFileExists bool) ([]string, error) {
func FilterIgnoredPaths(paths, ignore []string, checkFileExists bool, rootDir string) ([]string, error) {
// if set, rootDir is normalized to end with a platform appropriate separator
if rootDir != "" && !strings.HasSuffix(rootDir, string(filepath.Separator)) {
rootDir += string(filepath.Separator)
}

if checkFileExists {
filtered := make([]string, 0, len(paths))

Expand All @@ -29,14 +34,14 @@ func FilterIgnoredPaths(paths, ignore []string, checkFileExists bool) ([]string,
return nil, fmt.Errorf("failed to filter paths:\n%w", err)
}

return filterPaths(filtered, ignore)
return filterPaths(filtered, ignore, rootDir)
}

if len(ignore) == 0 {
return paths, nil
}

return filterPaths(paths, ignore)
return filterPaths(paths, ignore, rootDir)
}

func walkPaths(paths []string, filter func(path string, info os.DirEntry, err error) error) error {
Expand All @@ -60,7 +65,7 @@ func walkPaths(paths []string, filter func(path string, info os.DirEntry, err er
return errs
}

func filterPaths(policyPaths []string, ignore []string) ([]string, error) {
func filterPaths(policyPaths []string, ignore []string, rootDir string) ([]string, error) {
filtered := make([]string, 0, len(policyPaths))

outer:
Expand All @@ -70,7 +75,7 @@ outer:
continue
}

excluded, err := excludeFile(pattern, f)
excluded, err := excludeFile(pattern, f, rootDir)
if err != nil {
return nil, fmt.Errorf("failed to check for exclusion using pattern %s: %w", pattern, err)
}
Expand All @@ -88,9 +93,13 @@ outer:

// excludeFile imitates the pattern matching of .gitignore files
// See `exclusion.rego` for details on the implementation.
func excludeFile(pattern string, filename string) (bool, error) {
func excludeFile(pattern, filename, rootDir string) (bool, error) {
n := len(pattern)

if rootDir != "" {
filename = strings.TrimPrefix(filename, rootDir)
}

// Internal slashes means path is relative to root, otherwise it can
// appear anywhere in the directory (--> **/)
if !strings.Contains(pattern[:n-1], "/") {
Expand Down
113 changes: 113 additions & 0 deletions pkg/config/filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package config

import (
"sort"
"testing"
)

func TestFilterIgnoredPaths(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
paths []string
ignore []string
checkFileExists bool
rootDir string
expected []string
}{
"no paths": {
paths: []string{},
ignore: []string{},
expected: []string{},
},
"no ignore": {
paths: []string{"foo/bar.rego"},
ignore: []string{},
expected: []string{"foo/bar.rego"},
},
"explicit ignore": {
paths: []string{"foo/bar.rego", "foo/baz.rego"},
ignore: []string{"foo/bar.rego"},
expected: []string{"foo/baz.rego"},
},
"wildcard ignore": {
paths: []string{"foo/bar.rego", "foo/baz.rego", "bar/foo.rego"},
ignore: []string{"foo/*"},
expected: []string{"bar/foo.rego"},
},
"wildcard ignore, with ext": {
paths: []string{"foo/bar.rego", "foo/baz.rego", "bar/foo.rego"},
ignore: []string{"foo/*.rego"},
expected: []string{"bar/foo.rego"},
},
"double wildcard ignore": {
paths: []string{"foo/bar/baz/bax.rego", "foo/baz/bar/bax.rego", "bar/foo.rego"},
ignore: []string{"foo/bar/**"},
expected: []string{"bar/foo.rego", "foo/baz/bar/bax.rego"},
},
"rootDir, explicit ignore": {
paths: []string{"wow/foo/bar.rego", "wow/foo/baz.rego"},
ignore: []string{"foo/bar.rego"},
expected: []string{"wow/foo/baz.rego"},
rootDir: "wow/",
},
"rootDir, no slash, explicit ignore": {
paths: []string{"wow/foo/bar.rego", "wow/foo/baz.rego"},
ignore: []string{"foo/bar.rego"},
expected: []string{"wow/foo/baz.rego"},
rootDir: "wow",
},
"rootDir, wildcard ignore, with ext": {
paths: []string{"wow/foo/bar.rego", "wow/foo/baz.rego", "wow/bar/foo.rego"},
ignore: []string{"foo/*.rego"},
expected: []string{"wow/bar/foo.rego"},
rootDir: "wow/",
},
"rootDir, double wildcard ignore": {
paths: []string{
"wow/foo/bar/baz/bax.rego",
"wow/foo/baz/bar/bax.rego",
"wow/bar/foo.rego",
},
ignore: []string{"foo/bar/**"},
expected: []string{"wow/bar/foo.rego", "wow/foo/baz/bar/bax.rego"},
rootDir: "wow",
},
"rootDir URI": {
paths: []string{
"file:///wow/foo/bar.rego",
"file:///wow/foo/baz.rego",
"file:///wow/bar/foo.rego",
},
ignore: []string{"foo/*.rego"},
expected: []string{"file:///wow/bar/foo.rego"},
rootDir: "file:///wow",
},
}

for name, tc := range testCases {
tc := tc

t.Run(name, func(t *testing.T) {
t.Parallel()

filtered, err := FilterIgnoredPaths(tc.paths, tc.ignore, tc.checkFileExists, tc.rootDir)
if err != nil {
t.Fatal(err)
}

if len(filtered) != len(tc.expected) {
t.Fatalf("expected %d paths, got %d", len(tc.expected), len(filtered))
}

sort.Strings(filtered)
sort.Strings(tc.expected)

for i, path := range filtered {
if path != tc.expected[i] {
t.Errorf("expected path %s, got %s", tc.expected[i], path)
}
}
})
}
}
19 changes: 17 additions & 2 deletions pkg/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
type Linter struct {
inputPaths []string
inputModules *rules.Input
rootDir string
ruleBundles []*bundle.Bundle
userConfig *config.Config
combinedConfig *config.Config
Expand Down Expand Up @@ -201,6 +202,15 @@ func (l Linter) WithProfiling(enabled bool) Linter {
return l
}

// WithRootDir sets the root directory for the linter.
// A door directory or prefix can be use to resolve relative paths
// referenced in the linter configuration with absolute file paths or URIs.
func (l Linter) WithRootDir(rootDir string) Linter {
l.rootDir = rootDir

return l
}

// Lint runs the linter on provided policies.
func (l Linter) Lint(ctx context.Context) (report.Report, error) {
l.startTimer(regalmetrics.RegalLint)
Expand Down Expand Up @@ -239,7 +249,7 @@ func (l Linter) Lint(ctx context.Context) (report.Report, error) {

l.startTimer(regalmetrics.RegalFilterIgnoredFiles)

filtered, err := config.FilterIgnoredPaths(l.inputPaths, ignore, true)
filtered, err := config.FilterIgnoredPaths(l.inputPaths, ignore, true, l.rootDir)
if err != nil {
return report.Report{}, fmt.Errorf("errors encountered when reading files to lint: %w", err)
}
Expand All @@ -259,7 +269,12 @@ func (l Linter) Lint(ctx context.Context) (report.Report, error) {
if l.inputModules != nil {
l.startTimer(regalmetrics.RegalFilterIgnoredModules)

filteredPaths, err := config.FilterIgnoredPaths(l.inputModules.FileNames, ignore, false)
filteredPaths, err := config.FilterIgnoredPaths(
l.inputModules.FileNames,
ignore,
false,
l.rootDir,
)
if err != nil {
return report.Report{}, fmt.Errorf("failed to filter paths: %w", err)
}
Expand Down
25 changes: 25 additions & 0 deletions pkg/linter/linter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ or := 1
expViolations []string
expLevels []string
ignoreFilesFlag []string
rootDir string
}{
{
name: "baseline",
Expand Down Expand Up @@ -231,6 +232,28 @@ or := 1
filename: "p.rego",
expViolations: []string{},
},
{
name: "user config global ignore files with rootDir",
userConfig: &config.Config{
Ignore: config.Ignore{
Files: []string{"foo/*"},
},
},
filename: "file:///wow/foo/p.rego",
expViolations: []string{},
rootDir: "file:///wow",
},
{
name: "user config global ignore files with rootDir, not ignored",
userConfig: &config.Config{
Ignore: config.Ignore{
Files: []string{"bar/*"},
},
},
filename: "file:///wow/foo/p.rego",
expViolations: []string{"opa-fmt", "top-level-iteration", "rule-shadows-builtin"},
rootDir: "file:///wow",
},
{
name: "CLI flag ignore files",
filename: "p.rego",
Expand All @@ -246,6 +269,8 @@ or := 1

linter := NewLinter()

linter = linter.WithRootDir(tt.rootDir)

if tt.userConfig != nil {
linter = linter.WithUserConfig(*tt.userConfig)
}
Expand Down

0 comments on commit 9081abf

Please sign in to comment.