From f7542dc092be642d323fe22d8ec405580c53e5a8 Mon Sep 17 00:00:00 2001 From: Will Dixon Date: Mon, 30 Oct 2023 20:37:09 -0400 Subject: [PATCH] Add a strict and lax mode for each list given (#70) * add a strict and lax mode for each list given Signed-off-by: Will Dixon * Add tests to main configuration Signed-off-by: Will Dixon --------- Signed-off-by: Will Dixon --- .gitignore | 1 + README.md | 18 ++ cmd/depguard/main_test.go | 5 +- cmd/depguard/testfiles/.depguard.json | 3 +- cmd/depguard/testfiles/.depguard.toml | 3 +- cmd/depguard/testfiles/.depguard.yaml | 3 +- settings.go | 33 ++- settings_test.go | 327 +++++++++++++++++++++++--- 8 files changed, 342 insertions(+), 51 deletions(-) diff --git a/.gitignore b/.gitignore index 97cca67..e189bdb 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ *.out .idea +.null-ls*.go diff --git a/README.md b/README.md index a6c2871..2ccfa22 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ The following is an example configuration file. "$all", "!$test" ], + "listMode": "Strict", "allow": [ "$gostd", "github.com/OpenPeeDeeP" @@ -35,6 +36,7 @@ The following is an example configuration file. "files": [ "$test" ], + "listMode": "Lax", "deny": { "github.com/stretchr/testify": "Please use standard library for tests" } @@ -47,6 +49,7 @@ the linter's output. - `files` - list of file globs that will match this list of settings to compare against - `allow` - list of allowed packages - `deny` - map of packages that are not allowed where the value is a suggestion += `listMode` - the mode to use for package matching Files are matched using [Globs](https://github.com/gobwas/glob). If the files list is empty, then all files will match that list. Prefixing a file @@ -66,6 +69,21 @@ A Prefix List just means that a package will match a value, if the value is a prefix of the package. Example `github.com/OpenPeeDeeP/depguard` package will match a value of `github.com/OpenPeeDeeP` but won't match `github.com/OpenPeeDeeP/depguard/v2`. +ListMode is used to determine the package matching priority. There are three +different modes; Original, Strict, and Lax. + +Original is the original way that the package was written to use. It is not recommended +to stay with this and is only here for backwards compatibility. + +Strict, at its roots, is everything is denied unless in allowed. + +Lax, at its roots, is everything is allowed unless it is denied. + +There are cases where a package can be matched in both the allow and denied lists. +You may allow a subpackage but deny the root or vice versa. The `settings_tests.go` file +has many scenarios listed out under `TestListImportAllowed`. These tests will stay +up to date as features are added. + ### Variables There are variable replacements for each type of list (file or package). This is diff --git a/cmd/depguard/main_test.go b/cmd/depguard/main_test.go index 1067821..e7bca03 100644 --- a/cmd/depguard/main_test.go +++ b/cmd/depguard/main_test.go @@ -13,8 +13,9 @@ var testfiles embed.FS var expectedConfigStruct = &depguard.LinterSettings{ "main": &depguard.List{ - Files: []string{"$all", "!$test"}, - Allow: []string{"$gostd", "github.com/"}, + ListMode: "Strict", + Files: []string{"$all", "!$test"}, + Allow: []string{"$gostd", "github.com/"}, Deny: map[string]string{ "reflect": "Who needs reflection", "github.com/OpenPeeDeeP": "Use Something Else", diff --git a/cmd/depguard/testfiles/.depguard.json b/cmd/depguard/testfiles/.depguard.json index e083307..b990950 100644 --- a/cmd/depguard/testfiles/.depguard.json +++ b/cmd/depguard/testfiles/.depguard.json @@ -4,6 +4,7 @@ "$all", "!$test" ], + "listMode": "Strict", "allow": [ "$gostd", "github.com/" @@ -24,4 +25,4 @@ "github.com/OpenPeeDeeP/": "Use Something Else" } } -} \ No newline at end of file +} diff --git a/cmd/depguard/testfiles/.depguard.toml b/cmd/depguard/testfiles/.depguard.toml index d16363e..abd4d59 100644 --- a/cmd/depguard/testfiles/.depguard.toml +++ b/cmd/depguard/testfiles/.depguard.toml @@ -3,6 +3,7 @@ files = [ "$all", "!$test" ] +listMode = "Strict" allow = [ "$gostd", "github.com/" @@ -19,4 +20,4 @@ allow = [ "github.com/test" ] [tests.deny] -"github.com/OpenPeeDeeP/" = "Use Something Else" \ No newline at end of file +"github.com/OpenPeeDeeP/" = "Use Something Else" diff --git a/cmd/depguard/testfiles/.depguard.yaml b/cmd/depguard/testfiles/.depguard.yaml index 941a8ad..15167a2 100644 --- a/cmd/depguard/testfiles/.depguard.yaml +++ b/cmd/depguard/testfiles/.depguard.yaml @@ -2,6 +2,7 @@ main: files: - "$all" - "!$test" + listMode: Strict allow: - "$gostd" - github.com/ @@ -14,4 +15,4 @@ tests: allow: - github.com/test deny: - github.com/OpenPeeDeeP/: Use Something Else \ No newline at end of file + github.com/OpenPeeDeeP/: Use Something Else diff --git a/settings.go b/settings.go index 9314db6..311cacc 100644 --- a/settings.go +++ b/settings.go @@ -20,7 +20,9 @@ type List struct { type listMode int const ( - listModeStrict listMode = iota + listModeOriginal listMode = iota + listModeStrict + listModeLax ) type list struct { @@ -44,9 +46,13 @@ func (l *List) compile() (*list, error) { // Determine List Mode switch strings.ToLower(l.ListMode) { case "": - li.listMode = listModeStrict + li.listMode = listModeOriginal + case "original": + li.listMode = listModeOriginal case "strict": li.listMode = listModeStrict + case "lax": + li.listMode = listModeLax default: errs = append(errs, fmt.Errorf("%s is not a known list mode", l.ListMode)) } @@ -131,16 +137,25 @@ func (l *list) fileMatch(fileName string) bool { } func (l *list) importAllowed(imp string) (bool, string) { - inAllowed := len(l.allow) == 0 - if !inAllowed { - inAllowed, _ = strInPrefixList(imp, l.allow) + inAllowed, aIdx := strInPrefixList(imp, l.allow) + inDenied, dIdx := strInPrefixList(imp, l.deny) + var allowed bool + switch l.listMode { + case listModeOriginal: + inAllowed = len(l.allow) == 0 || inAllowed + allowed = inAllowed && !inDenied + case listModeStrict: + allowed = inAllowed && (!inDenied || len(l.allow[aIdx]) > len(l.deny[dIdx])) + case listModeLax: + allowed = !inDenied || (inAllowed && len(l.allow[aIdx]) > len(l.deny[dIdx])) + default: + allowed = false } - inDenied, suggIdx := strInPrefixList(imp, l.deny) sugg := "" - if inDenied && suggIdx != -1 { - sugg = l.suggestions[suggIdx] + if !allowed && inDenied && dIdx != -1 { + sugg = l.suggestions[dIdx] } - return inAllowed && !inDenied, sugg + return allowed, sugg } type LinterSettings map[string]*List diff --git a/settings_test.go b/settings_test.go index 0f6d501..b2de259 100644 --- a/settings_test.go +++ b/settings_test.go @@ -160,7 +160,7 @@ var ( }, }, { - name: "Strict Mode Default", + name: "Original Mode Default", list: &List{ Allow: []string{"os"}, Deny: map[string]string{ @@ -168,7 +168,23 @@ var ( }, }, exp: &list{ - listMode: listModeStrict, + listMode: listModeOriginal, + allow: []string{"os"}, + deny: []string{"reflect"}, + suggestions: []string{"Don't use Reflect"}, + }, + }, + { + name: "Set Original Mode", + list: &List{ + ListMode: "oRiGinal", + Allow: []string{"os"}, + Deny: map[string]string{ + "reflect": "Don't use Reflect", + }, + }, + exp: &list{ + listMode: listModeOriginal, allow: []string{"os"}, deny: []string{"reflect"}, suggestions: []string{"Don't use Reflect"}, @@ -177,7 +193,7 @@ var ( { name: "Set Strict Mode", list: &List{ - ListMode: "sTriCT", + ListMode: "sTrIct", Allow: []string{"os"}, Deny: map[string]string{ "reflect": "Don't use Reflect", @@ -190,6 +206,22 @@ var ( suggestions: []string{"Don't use Reflect"}, }, }, + { + name: "Set Lax Mode", + list: &List{ + ListMode: "lAx", + Allow: []string{"os"}, + Deny: map[string]string{ + "reflect": "Don't use Reflect", + }, + }, + exp: &list{ + listMode: listModeLax, + allow: []string{"os"}, + deny: []string{"reflect"}, + suggestions: []string{"Don't use Reflect"}, + }, + }, { name: "Unknown List Mode", list: &List{ @@ -509,13 +541,13 @@ type listImportAllowedScenario struct { type listImportAllowedScenarioInner struct { name string input string - expected bool + allowed bool suggestion string } var listImportAllowedScenarios = []*listImportAllowedScenario{ { - name: "Empty allow matches anything not in deny", + name: "Empty allow in Original matches anything not in deny", setup: &list{ deny: []string{"some/pkg/a", "some/pkg/b$"}, suggestions: []string{"because I said so", "please use newer version"}, @@ -524,86 +556,307 @@ var listImportAllowedScenarios = []*listImportAllowedScenario{ { name: "in deny", input: "some/pkg/a/bar", - expected: false, + allowed: false, suggestion: "because I said so", }, { - name: "not in deny suffixed by exact match", - input: "some/pkg/b/foo/bar", - expected: true, + name: "not in deny suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: true, }, { name: "in deny exact match", input: "some/pkg/b", - expected: false, + allowed: false, suggestion: "please use newer version", }, }, }, { - name: "Empty deny only matches what is in allow", + name: "Empty deny in Original only matches what is in allow", setup: &list{ allow: []string{"some/pkg/a", "some/pkg/b$"}, }, tests: []*listImportAllowedScenarioInner{ { - name: "in allow", - input: "some/pkg/a/bar", - expected: true, + name: "in allow", + input: "some/pkg/a/bar", + allowed: true, }, { - name: "not in allow suffixed by exact match", - input: "some/pkg/b/foo/bar", - expected: false, + name: "not in allow suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: false, }, { - name: "in allow exact match", - input: "some/pkg/b", - expected: true, + name: "in allow exact match", + input: "some/pkg/b", + allowed: true, }, }, }, { - name: "Both in Strict mode allows what is in allow and not in deny", + name: "Both in Original mode allows what is in allow and not in deny", setup: &list{ - listMode: listModeStrict, + listMode: listModeOriginal, allow: []string{"some/pkg/a/foo", "some/pkg/b", "some/pkg/c"}, - deny: []string{"some/pkg/a", "some/pkg/b/foo"}, - suggestions: []string{"because I said so", "really don't use"}, + deny: []string{"some/pkg/a", "some/pkg/b/foo", "some/pkg/d"}, + suggestions: []string{"because I said so", "really don't use", "common"}, }, tests: []*listImportAllowedScenarioInner{ { - name: "in allow but not in deny", - input: "some/pkg/c/alpha", - expected: true, + name: "in allow but not in deny", + input: "some/pkg/c/alpha", + allowed: true, }, { name: "subpackage allowed but root denied", input: "some/pkg/a/foo/bar", - expected: false, + allowed: false, suggestion: "because I said so", }, { name: "subpackage not in allowed but root denied", input: "some/pkg/a/baz", - expected: false, + allowed: false, suggestion: "because I said so", }, { name: "subpackage denied but root allowed", input: "some/pkg/b/foo/bar", - expected: false, + allowed: false, suggestion: "really don't use", }, { - name: "subpackage not denied but root allowed", - input: "some/pkg/b/baz", - expected: true, + name: "subpackage not denied but root allowed", + input: "some/pkg/b/baz", + allowed: true, }, { - name: "not in allow nor in deny", - input: "some/pkg/d/alpha", - expected: false, + name: "in deny but not in allow", + input: "some/pkg/d/baz", + allowed: false, + suggestion: "common", + }, + { + name: "not in allow nor in deny", + input: "some/pkg/e/alpha", + allowed: false, + }, + { + name: "check for out of bounds", + input: "aaa/pkg/e/alpha", + allowed: false, + }, + }, + }, + { + name: "Empty allow in Strict matches nothing", + setup: &list{ + listMode: listModeStrict, + deny: []string{"some/pkg/a", "some/pkg/b$"}, + suggestions: []string{"because I said so", "please use newer version"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in deny", + input: "some/pkg/a/bar", + allowed: false, + suggestion: "because I said so", + }, + { + name: "not in deny suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: false, + }, + { + name: "in deny exact match", + input: "some/pkg/b", + allowed: false, + suggestion: "please use newer version", + }, + }, + }, + { + name: "Empty deny in Strict only matches what is in allow", + setup: &list{ + listMode: listModeStrict, + allow: []string{"some/pkg/a", "some/pkg/b$"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in allow", + input: "some/pkg/a/bar", + allowed: true, + }, + { + name: "not in allow suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: false, + }, + { + name: "in allow exact match", + input: "some/pkg/b", + allowed: true, + }, + }, + }, + { + name: "Both in Strict mode allows what is in allow and not in deny", + setup: &list{ + listMode: listModeStrict, + allow: []string{"some/pkg/a/foo", "some/pkg/b", "some/pkg/c"}, + deny: []string{"some/pkg/a", "some/pkg/b/foo", "some/pkg/d"}, + suggestions: []string{"because I said so", "really don't use", "common"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in allow but not in deny", + input: "some/pkg/c/alpha", + allowed: true, + }, + { + name: "subpackage allowed but root denied", + input: "some/pkg/a/foo/bar", + allowed: true, + }, + { + name: "subpackage not in allowed but root denied", + input: "some/pkg/a/baz", + allowed: false, + suggestion: "because I said so", + }, + { + name: "subpackage denied but root allowed", + input: "some/pkg/b/foo/bar", + allowed: false, + suggestion: "really don't use", + }, + { + name: "subpackage not denied but root allowed", + input: "some/pkg/b/baz", + allowed: true, + }, + { + name: "in deny but not in allow", + input: "some/pkg/d/baz", + allowed: false, + suggestion: "common", + }, + { + name: "not in allow nor in deny", + input: "some/pkg/e/alpha", + allowed: false, + }, + { + name: "check for out of bounds", + input: "aaa/pkg/e/alpha", + allowed: false, + }, + }, + }, + { + name: "Empty allow in Lax matches anything not in deny", + setup: &list{ + listMode: listModeLax, + deny: []string{"some/pkg/a", "some/pkg/b$"}, + suggestions: []string{"because I said so", "please use newer version"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in deny", + input: "some/pkg/a/bar", + allowed: false, + suggestion: "because I said so", + }, + { + name: "not in deny suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: true, + }, + { + name: "in deny exact match", + input: "some/pkg/b", + allowed: false, + suggestion: "please use newer version", + }, + }, + }, + { + name: "Empty deny in Lax matches everything", + setup: &list{ + listMode: listModeLax, + allow: []string{"some/pkg/a", "some/pkg/b$"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in allow", + input: "some/pkg/a/bar", + allowed: true, + }, + { + name: "not in allow suffixed by exact match", + input: "some/pkg/b/foo/bar", + allowed: true, + }, + { + name: "in allow exact match", + input: "some/pkg/b", + allowed: true, + }, + }, + }, + { + name: "Both in Lax mode allows what is in allow and not in deny", + setup: &list{ + listMode: listModeLax, + allow: []string{"some/pkg/a/foo", "some/pkg/b", "some/pkg/c"}, + deny: []string{"some/pkg/a", "some/pkg/b/foo", "some/pkg/d"}, + suggestions: []string{"because I said so", "really don't use", "common"}, + }, + tests: []*listImportAllowedScenarioInner{ + { + name: "in allow but not in deny", + input: "some/pkg/c/alpha", + allowed: true, + }, + { + name: "subpackage allowed but root denied", + input: "some/pkg/a/foo/bar", + allowed: true, + }, + { + name: "subpackage not in allowed but root denied", + input: "some/pkg/a/baz", + allowed: false, + suggestion: "because I said so", + }, + { + name: "subpackage denied but root allowed", + input: "some/pkg/b/foo/bar", + allowed: false, + suggestion: "really don't use", + }, + { + name: "subpackage not denied but root allowed", + input: "some/pkg/b/baz", + allowed: true, + }, + { + name: "in deny but not in allow", + input: "some/pkg/d/baz", + allowed: false, + suggestion: "common", + }, + { + name: "not in allow nor in deny", + input: "some/pkg/e/alpha", + allowed: true, + }, + { + name: "check for out of bounds", + input: "aaa/pkg/e/alpha", + allowed: true, }, }, }, @@ -615,7 +868,7 @@ func TestListImportAllowed(t *testing.T) { for _, sc := range s.tests { ts.Run(sc.name, func(tst *testing.T) { act, sugg := s.setup.importAllowed(sc.input) - if act != sc.expected { + if act != sc.allowed { tst.Error("Did not return expected result") } if sugg != sc.suggestion {