From 0e995bc01a215e58d30736f542714276f4aaf4b4 Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Fri, 15 Apr 2022 17:29:52 -0400 Subject: [PATCH] New experimental check: Avoid shadowing (#149) * New experimental check: Avoid shadowing From https://github.community/t/order-of-owners-in-codeowners-file/143073/2 This is something that has bitten us. We initially put a "*" at the end of the CODEOWNERS files to catch unassigned files However, this entry should've been put at the beginning, only the last matching pattern is considered --- README.md | 7 +- internal/check/avoid_shadowing.go | 86 +++++++++++++++++ internal/check/avoid_shadowing_test.go | 94 +++++++++++++++++++ internal/load/load.go | 4 + tests/integration/integration_test.go | 23 ++++- .../avoid-shadowing.golden.txt | 6 ++ .../avoid-shadowing.golden.txt | 4 + .../gh-codeowners/avoid-shadowing.golden.txt | 4 + 8 files changed, 221 insertions(+), 7 deletions(-) create mode 100644 internal/check/avoid_shadowing.go create mode 100644 internal/check/avoid_shadowing_test.go create mode 100644 tests/integration/testdata/TestCheckFailures/avoid-shadowing.golden.txt create mode 100644 tests/integration/testdata/TestCheckSuccess/offline_checks/GitHubCODEOWNERS/avoid-shadowing.golden.txt create mode 100644 tests/integration/testdata/TestCheckSuccess/offline_checks/gh-codeowners/avoid-shadowing.golden.txt diff --git a/README.md b/README.md index 92c8412..ccb1117 100644 --- a/README.md +++ b/README.md @@ -105,9 +105,10 @@ The following checks are enabled by default: The experimental checks are disabled by default: -| Name | Description | -|----------|---------------------------------------------------------------------------------------------------------------------------------------------| -| notowned | **[Not Owned File Checker]**

Reports if a given repository contain files that do not have specified owners in CODEOWNERS file. | +| Name | Description | +|-----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------| +| notowned | **[Not Owned File Checker]**

Reports if a given repository contain files that do not have specified owners in CODEOWNERS file. | +| avoid-shadowing | **[Avoid Shadowing Checker]**

Reports if entries go from least specific to most specific. Otherwise, earlier entries are completely ignored. | To enable experimental check set `EXPERIMENTAL_CHECKS=notowned` environment variable. diff --git a/internal/check/avoid_shadowing.go b/internal/check/avoid_shadowing.go new file mode 100644 index 0000000..f3e85d5 --- /dev/null +++ b/internal/check/avoid_shadowing.go @@ -0,0 +1,86 @@ +package check + +import ( + "context" + "fmt" + "regexp" + "strings" + + "github.com/mszostok/codeowners-validator/internal/ctxutil" + "github.com/mszostok/codeowners-validator/pkg/codeowners" + "github.com/pkg/errors" +) + +type AvoidShadowing struct{} + +func NewAvoidShadowing() *AvoidShadowing { + return &AvoidShadowing{} +} + +func (c *AvoidShadowing) Check(ctx context.Context, in Input) (output Output, err error) { + var bldr OutputBuilder + + previousEntries := []codeowners.Entry{} + for _, entry := range in.CodeownersEntries { + if ctxutil.ShouldExit(ctx) { + return Output{}, ctx.Err() + } + re, err := wildCardToRegexp(endWithSlash(entry.Pattern)) + if err != nil { + return Output{}, errors.Wrapf(err, "while compiling pattern %s into a regexp", entry.Pattern) + } + shadowed := []codeowners.Entry{} + for _, previous := range previousEntries { + if re.MatchString(endWithSlash(previous.Pattern)) { + shadowed = append(shadowed, previous) + } + } + if len(shadowed) > 0 { + msg := fmt.Sprintf("Pattern %q shadows the following patterns:\n%s\nEntries should go from least-specific to most-specific.", entry.Pattern, c.listFormatFunc(shadowed)) + bldr.ReportIssue(msg, WithEntry(entry)) + } + previousEntries = append(previousEntries, entry) + } + + return bldr.Output(), nil +} + +// listFormatFunc is a basic formatter that outputs a bullet point list of the pattern. +func (c *AvoidShadowing) listFormatFunc(es []codeowners.Entry) string { + points := make([]string, len(es)) + for i, err := range es { + points[i] = fmt.Sprintf(" * %d: %q", err.LineNo, err.Pattern) + } + + return strings.Join(points, "\n") +} + +// Name returns human readable name of the validator +func (AvoidShadowing) Name() string { + return "[Experimental] Avoid Shadowing Checker" +} + +// endWithSlash adds a trailing slash to a string if it doesn't already end with one. +// This is useful when matching CODEOWNERS pattern because the trailing slash is optional. +func endWithSlash(s string) string { + if !strings.HasSuffix(s, "/") { + return s + "/" + } + return s +} + +// wildCardToRegexp converts a wildcard pattern to a regular expression pattern. +func wildCardToRegexp(pattern string) (*regexp.Regexp, error) { + var result strings.Builder + for i, literal := range strings.Split(pattern, "*") { + // Replace * with .* + if i > 0 { + result.WriteString(".*") + } + + // Quote any regular expression meta characters in the + // literal text. + result.WriteString(regexp.QuoteMeta(literal)) + } + return regexp.Compile("^" + result.String() + "$") +} diff --git a/internal/check/avoid_shadowing_test.go b/internal/check/avoid_shadowing_test.go new file mode 100644 index 0000000..830e300 --- /dev/null +++ b/internal/check/avoid_shadowing_test.go @@ -0,0 +1,94 @@ +package check_test + +import ( + "context" + "testing" + + "github.com/mszostok/codeowners-validator/internal/check" + "github.com/mszostok/codeowners-validator/internal/ptr" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAvoidShadowing(t *testing.T) { + tests := map[string]struct { + codeownersInput string + expectedIssues []check.Issue + }{ + "Should report info about shadowed entries": { + codeownersInput: ` + /build/logs/ @doctocat + /script @mszostok + + # Shadows + * @s1 + /s*/ @s2 + /s* @s3 + /b* @s4 + /b*/logs @s5 + + # OK + /b*/other @o1 + /script/* @o2 + `, + expectedIssues: []check.Issue{ + { + Severity: check.Error, + LineNo: ptr.Uint64Ptr(6), + Message: `Pattern "*" shadows the following patterns: + * 2: "/build/logs/" + * 3: "/script" +Entries should go from least-specific to most-specific.`, + }, + { + Severity: check.Error, + LineNo: ptr.Uint64Ptr(7), + Message: `Pattern "/s*/" shadows the following patterns: + * 3: "/script" +Entries should go from least-specific to most-specific.`, + }, + { + Severity: check.Error, + LineNo: ptr.Uint64Ptr(8), + Message: `Pattern "/s*" shadows the following patterns: + * 3: "/script" + * 7: "/s*/" +Entries should go from least-specific to most-specific.`, + }, + { + Severity: check.Error, + LineNo: ptr.Uint64Ptr(9), + Message: `Pattern "/b*" shadows the following patterns: + * 2: "/build/logs/" +Entries should go from least-specific to most-specific.`, + }, + { + Severity: check.Error, + LineNo: ptr.Uint64Ptr(10), + Message: `Pattern "/b*/logs" shadows the following patterns: + * 2: "/build/logs/" +Entries should go from least-specific to most-specific.`, + }, + }, + }, + "Should not report any issues with correct CODEOWNERS file": { + codeownersInput: FixtureValidCODEOWNERS, + expectedIssues: nil, + }, + } + + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + sut := check.NewAvoidShadowing() + + // when + out, err := sut.Check(context.TODO(), LoadInput(tc.codeownersInput)) + + // then + require.NoError(t, err) + assert.ElementsMatch(t, tc.expectedIssues, out.Issues) + }) + } +} diff --git a/internal/load/load.go b/internal/load/load.go index 02326de..6fcecf6 100644 --- a/internal/load/load.go +++ b/internal/load/load.go @@ -77,6 +77,10 @@ func loadExperimentalChecks(experimentalChecks []string) ([]check.Checker, error checks = append(checks, check.NewNotOwnedFile(cfg.NotOwnedChecker)) } + if contains(experimentalChecks, "avoid-shadowing") { + checks = append(checks, check.NewAvoidShadowing()) + } + return checks, nil } diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index c30e67b..1c56d54 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -43,9 +43,6 @@ var repositories = []struct { // This test is based on golden file. // If the `-test.update-golden` flag is set then the actual content is written // to the golden file. -// -// To update golden file, run: -// UPDATE_GOLDEN=true make test-integration func TestCheckSuccess(t *testing.T) { type ( Envs map[string]string @@ -57,6 +54,8 @@ func TestCheckSuccess(t *testing.T) { } ) + // To update golden file, run: + // TEST=TestCheckSuccess/offline_checks UPDATE_GOLDEN=true make test-integration t.Run("offline checks", func(t *testing.T) { for _, repoTC := range repositories { // given @@ -75,6 +74,13 @@ func TestCheckSuccess(t *testing.T) { "CHECKS": "duppatterns", }, }, + { + name: "avoid-shadowing", + envs: Envs{ + "CHECKS": "disable-all", + "EXPERIMENTAL_CHECKS": "avoid-shadowing", + }, + }, { name: "notowned", envs: Envs{ @@ -117,6 +123,8 @@ func TestCheckSuccess(t *testing.T) { } }) + // To update golden file, run: + // GITHUB_TOKEN= TEST=TestCheckSuccess/online_checks UPDATE_GOLDEN=true make test-integration t.Run("online checks", func(t *testing.T) { tests := testCase{ { @@ -179,7 +187,7 @@ func TestCheckSuccess(t *testing.T) { // to the golden file. // // To update golden file, run: -// UPDATE_GOLDEN=true make test-integration +// TEST=TestCheckFailures UPDATE_GOLDEN=true make test-integration func TestCheckFailures(t *testing.T) { type Envs map[string]string tests := []struct { @@ -207,6 +215,13 @@ func TestCheckFailures(t *testing.T) { "CHECKS": "duppatterns", }, }, + { + name: "avoid-shadowing", + envs: Envs{ + "CHECKS": "disable-all", + "EXPERIMENTAL_CHECKS": "avoid-shadowing", + }, + }, { name: "notowned", envs: Envs{ diff --git a/tests/integration/testdata/TestCheckFailures/avoid-shadowing.golden.txt b/tests/integration/testdata/TestCheckFailures/avoid-shadowing.golden.txt new file mode 100644 index 0000000..cd43a0c --- /dev/null +++ b/tests/integration/testdata/TestCheckFailures/avoid-shadowing.golden.txt @@ -0,0 +1,6 @@ +==> Executing [Experimental] Avoid Shadowing Checker () + [err] line 11: Pattern "/some/awesome/dir" shadows the following patterns: + * 10: "/some/awesome/dir" +Entries should go from least-specific to most-specific. + +1 check(s) executed, 1 failure(s) diff --git a/tests/integration/testdata/TestCheckSuccess/offline_checks/GitHubCODEOWNERS/avoid-shadowing.golden.txt b/tests/integration/testdata/TestCheckSuccess/offline_checks/GitHubCODEOWNERS/avoid-shadowing.golden.txt new file mode 100644 index 0000000..0308e14 --- /dev/null +++ b/tests/integration/testdata/TestCheckSuccess/offline_checks/GitHubCODEOWNERS/avoid-shadowing.golden.txt @@ -0,0 +1,4 @@ +==> Executing [Experimental] Avoid Shadowing Checker () + Check OK + +1 check(s) executed, no failure(s) diff --git a/tests/integration/testdata/TestCheckSuccess/offline_checks/gh-codeowners/avoid-shadowing.golden.txt b/tests/integration/testdata/TestCheckSuccess/offline_checks/gh-codeowners/avoid-shadowing.golden.txt new file mode 100644 index 0000000..0308e14 --- /dev/null +++ b/tests/integration/testdata/TestCheckSuccess/offline_checks/gh-codeowners/avoid-shadowing.golden.txt @@ -0,0 +1,4 @@ +==> Executing [Experimental] Avoid Shadowing Checker () + Check OK + +1 check(s) executed, no failure(s)