From 5bc9d1dd5cf5aa744bbf54c868cf971106df8085 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Wed, 16 Oct 2024 11:06:03 +0200 Subject: [PATCH] Handle `file://` URLs in exclusion policy (#1207) This is not an ideal solution, as we should not be sending URLs to the linter in the first place. But as fixing that is a much broader issue, we'll have to settle for a hack in the meantime. Fixes #1204 Signed-off-by: Anders Eknert --- bundle/regal/config/exclusion.rego | 83 ++++++++++++++----------- bundle/regal/config/exclusion_test.rego | 32 ++++++---- internal/lsp/lint_test.go | 78 +++++++++++++++++++++++ 3 files changed, 145 insertions(+), 48 deletions(-) diff --git a/bundle/regal/config/exclusion.rego b/bundle/regal/config/exclusion.rego index aa44aac7..4d6f1f23 100644 --- a/bundle/regal/config/exclusion.rego +++ b/bundle/regal/config/exclusion.rego @@ -3,46 +3,61 @@ package regal.config import rego.v1 # METADATA -# description: determines if file should be excluded by the given rule +# description: | +# determines if file should be excluded, either because it's globally +# ignored, or because the specific rule configuration excludes it excluded_file(category, title, file) if { - _force_exclude_file(file) -} else if { - rule_config := for_rule(category, title) - ex := rule_config.ignore.files - is_array(ex) - some pattern in ex - _exclude(pattern, file) -} else := false - -_force_exclude_file(file) if { # regal ignore:external-reference some pattern in _global_ignore_patterns - _exclude(pattern, file) + _exclude(pattern, _relative_to_pattern(pattern, file)) +} else if { + some pattern in for_rule(category, title).ignore.files + _exclude(pattern, _relative_to_pattern(pattern, file)) } -_global_ignore_patterns := merged_config.ignore.files if { - not data.eval.params.ignore_files -} else := data.eval.params.ignore_files +# NOTE +# this is an awful hack which is needed when the language server +# invokes linting, as it currently will provide filenames in the +# form of URIs rather than as relative paths... and as we do not +# know the base/workspace path here, we can only try to make the +# path relative to the *pattern* providedm which is something... +# +# pattern: foo/**/bar.rego +# file: file://my/workspace/foo/baz/bar.rego +# returns: foo/baz/bar.rego +# +_relative_to_pattern(pattern, file) := relative if { + startswith(file, "file://") + + absolute := trim_suffix(trim_prefix(file, "file://"), "/") + file_parts := indexof_n(absolute, "/") -# exclude imitates Gits .gitignore pattern matching as best it can -# Ref: https://git-scm.com/docs/gitignore#_pattern_format + relative := substring( + absolute, + array.slice( + file_parts, (count(file_parts) - strings.count(pattern, "/")) - 1, + count(file_parts), + )[0] + 1, + -1, + ) +} else := file + +_global_ignore_patterns := data.eval.params.ignore_files + +_global_ignore_patterns := merged_config.ignore.files if not data.eval.params.ignore_files + +# exclude imitates .gitignore pattern matching as best it can +# ref: https://git-scm.com/docs/gitignore#_pattern_format _exclude(pattern, file) if { - patterns := _pattern_compiler(pattern) - some p in patterns + some p in _pattern_compiler(pattern) glob.match(p, ["/"], file) -} else := false +} -# pattern_compiler transforms a glob pattern into a set of glob patterns to make the -# combined set behave as Gits .gitignore -_pattern_compiler(pattern) := ps1 if { - p := _internal_slashes(pattern) - p1 := _leading_slash(p) - ps := _leading_doublestar_pattern(p1) - ps1 := {pat | - some _p, _ in ps - nps := _trailing_slash(_p) - some pat, _ in nps - } +# pattern_compiler transforms a glob pattern into a set of glob +# patterns to make the combined set behave as .gitignore +_pattern_compiler(pattern) := {pat | + some p in _leading_doublestar_pattern(trim_prefix(_internal_slashes(pattern), "/")) + some pat in _trailing_slash(p) } # Internal slashes means that the path is relative to root, @@ -73,9 +88,3 @@ _trailing_slash(pattern) := {pattern, np} if { endswith(pattern, "/") np := concat("", [pattern, "**"]) } else := {pattern} - -# If a pattern starts with a "/", the leading slash is ignored but according to -# the .gitignore rule of internal slashes, it is relative to root -_leading_slash(pattern) := substring(pattern, 1, -1) if { - startswith(pattern, "/") -} else := pattern diff --git a/bundle/regal/config/exclusion_test.rego b/bundle/regal/config/exclusion_test.rego index 67fd7fe8..d9a4d71f 100644 --- a/bundle/regal/config/exclusion_test.rego +++ b/bundle/regal/config/exclusion_test.rego @@ -64,36 +64,46 @@ rules_config_ignore_delta := {"rules": {"test": {"test-case": {"ignore": {"files config_ignore := {"ignore": {"files": ["p.rego"]}} test_excluded_file_default if { - e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params + not config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params with config.merged_config as rules_config_error - - e == false } test_excluded_file_with_ignore if { c := object.union(rules_config_error, rules_config_ignore_delta) - e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params + + config.excluded_file("test", "test-case", "p.rego") with data.eval.params as params with config.merged_config as c - e == true } test_excluded_file_config if { - e := config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore - e == true + config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore } test_excluded_file_cli_flag if { - e := config.excluded_file("test", "test-case", "p.rego") with data.eval.params as object.union( + config.excluded_file("test", "test-case", "p.rego") with data.eval.params as object.union( params, {"ignore_files": ["p.rego"]}, ) - e == true } test_excluded_file_cli_overrides_config if { - e := config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore + not config.excluded_file("test", "test-case", "p.rego") with config.merged_config as config_ignore with data.eval.params as object.union(params, {"ignore_files": [""]}) - e == false +} + +test_excluded_file_using_uri if { + conf := {"rules": {"test": {"rule": { + "level": "error", + "ignore": {"files": ["foo/**/p.rego"]}, + }}}} + + config.excluded_file("test", "rule", "file:///workspace/folder/foo/bar/p.rego") with config.merged_config as conf +} + +test_not_excluded_file_using_uri if { + conf := {"rules": {"test": {"rule": {"level": "error"}}}} + + not config.excluded_file("test", "rule", "file:///workspace/folder/foo/bar/p.rego") with config.merged_config as conf } test_trailing_slash if { diff --git a/internal/lsp/lint_test.go b/internal/lsp/lint_test.go index 9db26d8e..4c9a8254 100644 --- a/internal/lsp/lint_test.go +++ b/internal/lsp/lint_test.go @@ -1,10 +1,14 @@ package lsp import ( + "context" "reflect" "testing" + "github.com/styrainc/regal/internal/lsp/cache" "github.com/styrainc/regal/internal/lsp/types" + "github.com/styrainc/regal/internal/parse" + "github.com/styrainc/regal/pkg/config" "github.com/styrainc/regal/pkg/report" ) @@ -65,3 +69,77 @@ func TestConvertReportToDiagnostics(t *testing.T) { t.Errorf("Expected file diagnostics: %v, got: %v", expectedFileDiags, fileDiags) } } + +func TestLintWithConfigIgnoreWildcards(t *testing.T) { + t.Parallel() + + conf := &config.Config{ + Rules: map[string]config.Category{ + "idiomatic": { + "directory-package-mismatch": config.Rule{ + Level: "ignore", + }, + }, + }, + } + + contents := "package p\n\nimport rego.v1\n\ncamelCase := 1\n" + module := parse.MustParseModule(contents) + workspacePath := "/workspace" + fileURI := "file:///workspace/ignore/p.rego" + state := cache.NewCache() + + state.SetFileContents(fileURI, contents) + state.SetModule(fileURI, module) + state.SetFileDiagnostics(fileURI, []types.Diagnostic{}) + + if err := updateFileDiagnostics( + context.Background(), + state, + conf, + fileURI, + workspacePath, + []string{"prefer-snake-case"}, + ); err != nil { + t.Fatalf("Expected no error, got %v", err) + } + + diagnostics, _ := state.GetFileDiagnostics(fileURI) + if len(diagnostics) != 1 { + t.Fatalf("Expected one diagnostic item, got %v", diagnostics) + } + + if diagnostics[0].Code != "prefer-snake-case" { + t.Errorf("Expected diagnostic code to be prefer-snake-case, got %v", diagnostics[0].Code) + } + + // Clear the diagnostic and update the config with a wildcard ignore + // for any file in the ignore directory. + + state.SetFileDiagnostics(fileURI, []types.Diagnostic{}) + + conf.Rules["style"] = config.Category{ + "prefer-snake-case": config.Rule{ + Level: "error", + Ignore: &config.Ignore{ + Files: []string{"ignore/**"}, + }, + }, + } + + if err := updateFileDiagnostics( + context.Background(), + state, + conf, + fileURI, + workspacePath, + []string{"prefer-snake-case"}, + ); err != nil { + t.Fatalf("Expected no error, got %v", err) + } + + diagnostics, _ = state.GetFileDiagnostics(fileURI) + if len(diagnostics) != 0 { + t.Fatalf("Expected no diagnostics, got %v", diagnostics) + } +}