From 7acde4a20c5204aaa2953ddcab3880794b77ec5c Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 8 Jan 2025 12:16:09 +0000 Subject: [PATCH 1/5] fixer: Prep for 1.0 tests Signed-off-by: Charlie Egan --- e2e/cli_test.go | 2 - pkg/fixer/fixer_test.go | 86 ++++++++++++++++++++++++----------------- pkg/linter/linter.go | 2 +- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/e2e/cli_test.go b/e2e/cli_test.go index 1df4b1c1..82e27e7b 100644 --- a/e2e/cli_test.go +++ b/e2e/cli_test.go @@ -288,8 +288,6 @@ func TestLintV0WithRegoV1ImportViolations(t *testing.T) { t.Errorf("expected stderr %q, got %q", exp, act) } - //fmt.Println(stdout.String()) - var rep report.Report if err = json.Unmarshal(stdout.Bytes(), &rep); err != nil { t.Fatalf("expected JSON response, got %v", stdout.String()) diff --git a/pkg/fixer/fixer_test.go b/pkg/fixer/fixer_test.go index a58f59e5..21e66150 100644 --- a/pkg/fixer/fixer_test.go +++ b/pkg/fixer/fixer_test.go @@ -3,12 +3,15 @@ package fixer import ( "bytes" "context" + "os" "slices" "testing" "github.com/open-policy-agent/opa/v1/ast" "github.com/open-policy-agent/opa/v1/format" + "github.com/open-policy-agent/opa/v1/topdown" + "github.com/styrainc/regal/pkg/config" "github.com/styrainc/regal/pkg/fixer/fileprovider" "github.com/styrainc/regal/pkg/fixer/fixes" "github.com/styrainc/regal/pkg/linter" @@ -20,11 +23,7 @@ func TestFixer(t *testing.T) { policies := map[string][]byte{ "test/main.rego": []byte(`package test -allow { -true #no space -} - -deny = true +allow = true #no space `), } @@ -37,10 +36,22 @@ deny = true l := linter.NewLinter(). WithEnableAll(true). - WithInputModules(&input) + WithInputModules(&input). + WithPrintHook(topdown.NewPrintHook(os.Stderr)). + WithUserConfig(config.Config{ + Capabilities: &config.Capabilities{ + Features: []string{"rego_v1_import"}, + }, + }) f := NewFixer() - f.RegisterFixes(fixes.NewDefaultFixes()...) + f.RegisterFixes( + []fixes.Fix{ + &fixes.UseAssignmentOperator{}, + &fixes.NoWhitespaceComment{}, + &fixes.DirectoryPackageMismatch{}, + }..., + ) fixReport, err := f.Fix(context.Background(), &l, memfp) if err != nil { @@ -48,24 +59,17 @@ deny = true } expectedFileFixedViolations := map[string][]string{ - // use-assignment-operator is not expected since use-rego-v1 also addresses this in this example - "test/main.rego": {"no-whitespace-comment", "use-rego-v1"}, + "test/main.rego": {"no-whitespace-comment", "use-assignment-operator"}, } expectedFileContents := map[string][]byte{ "test/main.rego": []byte(`package test -import rego.v1 - -allow := true - -# no space - -deny := true +allow := true # no space `), } if got, exp := fixReport.TotalFixes(), uint(2); got != exp { - t.Fatalf("expected %d fixed files, got %d", exp, got) + t.Fatalf("expected a total of %d fixes, got %d", exp, got) } fpFiles, err := memfp.List() @@ -95,19 +99,21 @@ deny := true // check that the fixed violations are correct fxs := fixReport.FixesForFile(file) + var fixes []string + for _, fx := range fxs { + fixes = append(fixes, fx.Title) + } + expectedFixes, ok := expectedFileFixedViolations[file] if !ok { - t.Fatalf("unexpected file waas fixed %s", file) + t.Fatalf("unexpected file was fixed %s", file) } - if len(fxs) != len(expectedFixes) { - t.Fatalf("unexpected number of fixes for %s:\ngot: %v\nexpected: %v", file, fxs, expectedFixes) - } + slices.Sort(expectedFixes) + slices.Sort(fixes) - for _, fx := range fxs { - if !slices.Contains(expectedFixes, fx.Title) { - t.Fatalf("expected fixes to contain %s:\ngot: %v", fx.Title, expectedFixes) - } + if !slices.Equal(expectedFixes, fixes) { + t.Fatalf("unexpected fixes for %s:\ngot: %v\nexpected: %v", file, fixes, expectedFixes) } } } @@ -135,7 +141,12 @@ deny = true l := linter.NewLinter(). WithEnableAll(true). - WithInputModules(&input) + WithInputModules(&input). + WithUserConfig(config.Config{ + Capabilities: &config.Capabilities{ + Features: []string{"rego_v1_import"}, + }, + }) f := NewFixer() // No fixes are registered here, we are only testing the functionality of @@ -173,7 +184,7 @@ deny := true } if got, exp := fixReport.TotalFixes(), uint(1); got != exp { - t.Fatalf("expected %d fixed files, got %d", exp, got) + t.Fatalf("expected %d fixes, got %d", exp, got) } fpFiles, err := memfp.List() @@ -200,22 +211,25 @@ deny := true string(expectedContent)) } - // check that the fixed violations are correct fxs := fixReport.FixesForFile(file) + // check that the fixed violations are correct + var fixes []string + for _, fx := range fxs { + fixes = append(fixes, fx.Title) + } + expectedFixes, ok := expectedFileFixedViolations[file] if !ok { - t.Fatalf("unexpected file waas fixed %s", file) + t.Fatalf("unexpected file was fixed %s", file) } - if len(fxs) != len(expectedFixes) { - t.Fatalf("unexpected number of fixes for %s:\ngot: %v\nexpected: %v", file, fxs, expectedFixes) - } + slices.Sort(expectedFixes) + slices.Sort(fixes) - for _, fx := range fxs { - if !slices.Contains(expectedFixes, fx.Title) { - t.Fatalf("expected fixes to contain %s:\ngot: %v", fx.Title, expectedFixes) - } + if !slices.Equal(expectedFixes, fixes) { + t.Fatalf("unexpected fixes for %s:\ngot: %v\nexpected: %v", file, fixes, expectedFixes) } + } } diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index cabae830..3be7cd3f 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -463,7 +463,7 @@ func (l Linter) DetermineEnabledRules(ctx context.Context) ([]string, error) { queryStr := `[rule | data.regal.rules[cat][rule] - count([true | data.regal.rules[cat][rule].notices]) == 0 + object.get(data.regal.rules[cat][rule], "notices", set()) == set() data.regal.config.for_rule(cat, rule).level != "ignore" ]` From a8fa1e309e1d30064f43ec2376fc23f519e53832 Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 8 Jan 2025 14:02:26 +0000 Subject: [PATCH 2/5] fixer/fmt: Add tests for other rego versions Signed-off-by: Charlie Egan --- pkg/fixer/fixes/fmt_test.go | 71 +++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/pkg/fixer/fixes/fmt_test.go b/pkg/fixer/fixes/fmt_test.go index 4922a79f..0816342d 100644 --- a/pkg/fixer/fixes/fmt_test.go +++ b/pkg/fixer/fixes/fmt_test.go @@ -34,6 +34,68 @@ func TestFmt(t *testing.T) { fmt: &Fmt{}, fixExpected: true, }, + "rego version unknown, ambigous syntax": { + fc: &FixCandidate{ + Filename: "test.rego", + Contents: []byte("package testutil\nallow := true"), + RegoVersion: ast.RegoUndefined, + }, + fmt: &Fmt{}, + fixExpected: true, + contentAfterFix: []byte(`package testutil + +allow := true +`), + }, + "rego version unknown, v0 syntax": { + fc: &FixCandidate{ + Filename: "test.rego", + Contents: []byte("package testutil\nallow[msg] { msg := 1}"), + RegoVersion: ast.RegoUndefined, + }, + fmt: &Fmt{}, + fixExpected: true, + contentAfterFix: []byte(`package testutil + +import rego.v1 + +allow contains msg if msg := 1 +`), + }, + "rego version unknown, v0v1 compat syntax": { + fc: &FixCandidate{ + Filename: "test.rego", + Contents: []byte(`package testutil +import rego.v1 +allow contains msg if msg :=1 + `), + RegoVersion: ast.RegoUndefined, + }, + fmt: &Fmt{}, + fixExpected: true, + contentAfterFix: []byte(`package testutil + +import rego.v1 + +allow contains msg if msg := 1 +`), + }, + "rego version unknown, v1 syntax": { + fc: &FixCandidate{ + Filename: "test.rego", + Contents: []byte(`package testutil + +allow contains msg if msg :=1 + `), + RegoVersion: ast.RegoUndefined, + }, + fmt: &Fmt{}, + fixExpected: true, + contentAfterFix: []byte(`package testutil + +allow contains msg if msg := 1 +`), + }, "rego v1 (rego version 0)": { fc: &FixCandidate{ Filename: "test.rego", @@ -58,6 +120,15 @@ allow := true }, fixExpected: false, }, + "rego v1, version known": { + fc: &FixCandidate{Filename: "test.rego", Contents: []byte("package testutil\n\nallow := true\n")}, + fmt: &Fmt{ + OPAFmtOpts: format.Opts{ + RegoVersion: ast.RegoV1, + }, + }, + fixExpected: false, + }, } for testName, tc := range testCases { From d80b1849b1d8d946ac1775f0d3690eefc574c0a5 Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 8 Jan 2025 14:54:44 +0000 Subject: [PATCH 3/5] fixer: Update fixer e2e test Signed-off-by: Charlie Egan --- e2e/cli_test.go | 79 ++++++++++++++++++++++++++++------------ internal/lsp/server.go | 9 +---- pkg/fixer/fixer_test.go | 1 - pkg/fixer/fixes/fixes.go | 13 ++----- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/e2e/cli_test.go b/e2e/cli_test.go index 82e27e7b..f2e92012 100644 --- a/e2e/cli_test.go +++ b/e2e/cli_test.go @@ -834,11 +834,17 @@ func TestFix(t *testing.T) { td := t.TempDir() initialState := map[string]string{ - ".regal/config.yaml": "", // needed to find the root in the right place + ".regal/config.yaml": ` +project: + rego-version: 1 + roots: + - path: v0 + rego-version: 0 + - path: v1 + rego-version: 0 +`, "foo/main.rego": `package wow -import rego.v1 - #comment allow if { @@ -847,14 +853,12 @@ allow if { `, "foo/main_test.rego": `package wow_test -test_allow { +test_allow if { true } `, "foo/foo.rego": `package foo -import rego.v1 - # present and correct allow if { @@ -862,13 +866,21 @@ allow if { } `, "bar/main.rego": `package wow["foo-bar"].baz - -import rego.v1 `, "bar/main_test.rego": `package wow["foo-bar"].baz_test -test_allow { +test_allow if { true } +`, + "v0/main.rego": `package v0 + +#comment +allow { true } +`, + "v1/main.rego": `package v1 + +#comment +allow if { true } `, "unrelated.txt": `foobar`, } @@ -878,29 +890,46 @@ test_allow { } // --force is required to make the changes when there is no git repo - err := regal(&stdout, &stderr)("fix", "--force", filepath.Join(td, "foo"), filepath.Join(td, "bar")) + err := regal(&stdout, &stderr)( + "fix", + "--force", + filepath.Join(td, "foo"), + filepath.Join(td, "bar"), + filepath.Join(td, "v0"), + filepath.Join(td, "v1"), + ) // 0 exit status is expected as all violations should have been fixed expectExitCode(t, err, 0, &stdout, &stderr) - exp := fmt.Sprintf(`8 fixes applied: -In project root: %s + exp := fmt.Sprintf(`12 fixes applied: +In project root: %[1]s bar/main.rego -> wow/foo-bar/baz/main.rego: - directory-package-mismatch bar/main_test.rego -> wow/foo-bar/baz/main_test.rego: - directory-package-mismatch -- use-rego-v1 +- opa-fmt foo/main.rego -> wow/main.rego: - directory-package-mismatch -- use-rego-v1 +- opa-fmt - no-whitespace-comment foo/main_test.rego -> wow/main_test.rego: - directory-package-mismatch -- use-rego-v1 +- opa-fmt + + +In project root: %[1]s/v0 +main.rego: +- opa-fmt +- no-whitespace-comment +In project root: %[1]s/v1 +main.rego: +- opa-fmt +- no-whitespace-comment `, td) if act := stdout.String(); exp != act { @@ -914,8 +943,6 @@ foo/main_test.rego -> wow/main_test.rego: expectedState := map[string]string{ "foo/foo.rego": `package foo -import rego.v1 - # present and correct allow if { @@ -923,28 +950,32 @@ allow if { } `, "wow/foo-bar/baz/main.rego": `package wow["foo-bar"].baz - -import rego.v1 `, "wow/foo-bar/baz/main_test.rego": `package wow["foo-bar"].baz_test -import rego.v1 - test_allow := true `, "wow/main.rego": `package wow -import rego.v1 - # comment allow := true `, "wow/main_test.rego": `package wow_test +test_allow := true +`, + "v0/main.rego": `package v0 + import rego.v1 -test_allow := true +# comment +allow := true +`, + "v1/main.rego": `package v1 + +# comment +allow := true `, "unrelated.txt": `foobar`, } diff --git a/internal/lsp/server.go b/internal/lsp/server.go index ab798479..ba2139ca 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -2165,14 +2165,7 @@ func (l *LanguageServer) handleTextDocumentFormatting( f := fixer.NewFixer() f.RegisterFixes(fixes.NewDefaultFormatterFixes()...) - f.RegisterMandatoryFixes( - &fixes.Fmt{ - NameOverride: "use-rego-v1", - OPAFmtOpts: format.Opts{ - RegoVersion: ast.RegoV0CompatV1, - }, - }, - ) + f.RegisterMandatoryFixes([]fixes.Fix{&fixes.Fmt{}}...) if roots, err := config.GetPotentialRoots( l.workspacePath(), diff --git a/pkg/fixer/fixer_test.go b/pkg/fixer/fixer_test.go index 21e66150..7d2a70f7 100644 --- a/pkg/fixer/fixer_test.go +++ b/pkg/fixer/fixer_test.go @@ -230,6 +230,5 @@ deny := true if !slices.Equal(expectedFixes, fixes) { t.Fatalf("unexpected fixes for %s:\ngot: %v\nexpected: %v", file, fixes, expectedFixes) } - } } diff --git a/pkg/fixer/fixes/fixes.go b/pkg/fixer/fixes/fixes.go index b6f6b5ce..f09430a1 100644 --- a/pkg/fixer/fixes/fixes.go +++ b/pkg/fixer/fixes/fixes.go @@ -2,7 +2,6 @@ package fixes import ( "github.com/open-policy-agent/opa/v1/ast" - "github.com/open-policy-agent/opa/v1/format" "github.com/styrainc/regal/internal/lsp/clients" "github.com/styrainc/regal/pkg/config" @@ -14,10 +13,9 @@ func NewDefaultFixes() []Fix { return []Fix{ &Fmt{}, &Fmt{ + // this effectively maps the fix for violations from the + // use-rego-v1 rule to just format the file. NameOverride: "use-rego-v1", - OPAFmtOpts: format.Opts{ - RegoVersion: ast.RegoV0CompatV1, - }, }, &UseAssignmentOperator{}, &NoWhitespaceComment{}, @@ -29,12 +27,7 @@ func NewDefaultFixes() []Fix { // Notably, this does not include fixers that move files around. func NewDefaultFormatterFixes() []Fix { return []Fix{ - &Fmt{ - NameOverride: "use-rego-v1", - OPAFmtOpts: format.Opts{ - RegoVersion: ast.RegoV0CompatV1, - }, - }, + &Fmt{}, &UseAssignmentOperator{}, &NoWhitespaceComment{}, } From 4262a4aaca9cc821205214e8589217f51f8d3850 Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 8 Jan 2025 14:58:48 +0000 Subject: [PATCH 4/5] Correct formatting of launch.json --- .vscode/launch.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index bd6d1c89..fc5243a3 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -26,4 +26,5 @@ ] } ] -} \ No newline at end of file +} + From a5c1b188fc41c39cde6ca1895affcceca3988dfc Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 8 Jan 2025 15:18:41 +0000 Subject: [PATCH 5/5] fixer: Return formatting to Fixer test Signed-off-by: Charlie Egan --- .vscode/launch.json | 1 - pkg/fixer/fixer_test.go | 32 +++++++++++++------------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index fc5243a3..0044af33 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -27,4 +27,3 @@ } ] } - diff --git a/pkg/fixer/fixer_test.go b/pkg/fixer/fixer_test.go index 7d2a70f7..bd9f1681 100644 --- a/pkg/fixer/fixer_test.go +++ b/pkg/fixer/fixer_test.go @@ -3,13 +3,11 @@ package fixer import ( "bytes" "context" - "os" "slices" "testing" "github.com/open-policy-agent/opa/v1/ast" "github.com/open-policy-agent/opa/v1/format" - "github.com/open-policy-agent/opa/v1/topdown" "github.com/styrainc/regal/pkg/config" "github.com/styrainc/regal/pkg/fixer/fileprovider" @@ -23,7 +21,10 @@ func TestFixer(t *testing.T) { policies := map[string][]byte{ "test/main.rego": []byte(`package test -allow = true #no space +allow if { +true #no space +} +deny = true `), } @@ -36,22 +37,10 @@ allow = true #no space l := linter.NewLinter(). WithEnableAll(true). - WithInputModules(&input). - WithPrintHook(topdown.NewPrintHook(os.Stderr)). - WithUserConfig(config.Config{ - Capabilities: &config.Capabilities{ - Features: []string{"rego_v1_import"}, - }, - }) + WithInputModules(&input) f := NewFixer() - f.RegisterFixes( - []fixes.Fix{ - &fixes.UseAssignmentOperator{}, - &fixes.NoWhitespaceComment{}, - &fixes.DirectoryPackageMismatch{}, - }..., - ) + f.RegisterFixes(fixes.NewDefaultFixes()...) fixReport, err := f.Fix(context.Background(), &l, memfp) if err != nil { @@ -59,12 +48,17 @@ allow = true #no space } expectedFileFixedViolations := map[string][]string{ - "test/main.rego": {"no-whitespace-comment", "use-assignment-operator"}, + // use-assigment-operator is correct in formatting so does not appear. + "test/main.rego": {"no-whitespace-comment", "opa-fmt"}, } expectedFileContents := map[string][]byte{ "test/main.rego": []byte(`package test -allow := true # no space +allow := true + +# no space + +deny := true `), }