New experimental check: Avoid shadowing (#149)

* New experimental check: Avoid shadowing
From https:munity/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

@@ -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]** <br /><br /> Reports if a given repository contain files that do not have specified owners in CODEOWNERS file. |
+| Name | Description |
+|-----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------|
+| notowned | **[Not Owned File Checker]** <br /><br /> Reports if a given repository contain files that do not have specified owners in CODEOWNERS file. |
+| avoid-shadowing | **[Avoid Shadowing Checker]** <br /><br /> 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.
 

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() + "$")
+}

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)
+ })
+ }
+}

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
 }
 

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=<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{

tests/integration/testdata/TestCheckFailures/avoid-shadowing.golden.txt

@@ -0,0 +1,6 @@
+==> Executing [Experimental] Avoid Shadowing Checker (<duration>)
+ [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)

tests/integration/testdata/TestCheckSuccess/offline_checks/GitHubCODEOWNERS/avoid-shadowing.golden.txt

@@ -0,0 +1,4 @@
+==> Executing [Experimental] Avoid Shadowing Checker (<duration>)
+ Check OK
+
+1 check(s) executed, no failure(s)

tests/integration/testdata/TestCheckSuccess/offline_checks/gh-codeowners/avoid-shadowing.golden.txt

@@ -0,0 +1,4 @@
+==> Executing [Experimental] Avoid Shadowing Checker (<duration>)
+ Check OK
+
+1 check(s) executed, no failure(s)