From 7d0d7c304eabfa66055cde987a0c685eb5f04e6c Mon Sep 17 00:00:00 2001 From: Paulo Bittencourt Date: Wed, 30 Oct 2019 03:34:29 -0400 Subject: [PATCH] MM-17888 Check plugin Helpers minimum server version comments (#12663) --- Makefile | 2 +- plugin/checker/check_api.go | 50 +++++++ .../{main_test.go => check_api_test.go} | 22 +-- plugin/checker/check_helpers.go | 137 +++++++++++++++++ plugin/checker/check_helpers_test.go | 48 ++++++ plugin/checker/internal/asthelpers/helpers.go | 129 ++++++++++++++++ .../{ => internal}/test/invalid/invalid.go | 20 +++ .../{ => internal}/test/missing/missing.go | 0 plugin/checker/internal/test/valid/valid.go | 44 ++++++ plugin/checker/internal/version/comments.go | 22 +++ .../checker/internal/version/comments_test.go | 49 ++++++ plugin/checker/internal/version/version.go | 80 ++++++++++ .../checker/internal/version/version_test.go | 58 ++++++++ plugin/checker/main.go | 140 ++++++------------ plugin/checker/render.go | 29 ++++ plugin/checker/test/valid/valid.go | 12 -- plugin/helpers.go | 6 + 17 files changed, 729 insertions(+), 119 deletions(-) create mode 100644 plugin/checker/check_api.go rename plugin/checker/{main_test.go => check_api_test.go} (69%) create mode 100644 plugin/checker/check_helpers.go create mode 100644 plugin/checker/check_helpers_test.go create mode 100644 plugin/checker/internal/asthelpers/helpers.go rename plugin/checker/{ => internal}/test/invalid/invalid.go (57%) rename plugin/checker/{ => internal}/test/missing/missing.go (100%) create mode 100644 plugin/checker/internal/test/valid/valid.go create mode 100644 plugin/checker/internal/version/comments.go create mode 100644 plugin/checker/internal/version/comments_test.go create mode 100644 plugin/checker/internal/version/version.go create mode 100644 plugin/checker/internal/version/version_test.go create mode 100644 plugin/checker/render.go delete mode 100644 plugin/checker/test/valid/valid.go diff --git a/Makefile b/Makefile index 5b159471d0..950bd877e7 100644 --- a/Makefile +++ b/Makefile @@ -154,7 +154,7 @@ govet: ## Runs govet against all packages. env GO111MODULE=off $(GO) get golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow $(GO) vet $(GOFLAGS) $(ALL_PACKAGES) || exit 1 $(GO) vet -vettool=$(GOPATH)/bin/shadow $(GOFLAGS) $(ALL_PACKAGES) || exit 1 - $(GO) run $(GOFLAGS) plugin/checker/main.go + $(GO) run $(GOFLAGS) ./plugin/checker gofmt: ## Runs gofmt against all packages. @echo Running GOFMT diff --git a/plugin/checker/check_api.go b/plugin/checker/check_api.go new file mode 100644 index 0000000000..e5b9ec716f --- /dev/null +++ b/plugin/checker/check_api.go @@ -0,0 +1,50 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package main + +import ( + "fmt" + "go/ast" + "go/token" + + "github.com/mattermost/mattermost-server/plugin/checker/internal/asthelpers" + "github.com/mattermost/mattermost-server/plugin/checker/internal/version" +) + +func checkAPIVersionComments(pkgPath string) (result, error) { + pkg, err := asthelpers.GetPackage(pkgPath) + if err != nil { + return result{}, err + } + + apiInterface, err := asthelpers.FindInterface("API", pkg.Syntax) + if err != nil { + return result{}, err + } + + invalidMethods := findInvalidMethods(apiInterface.Methods.List) + return result{Errors: renderErrors(pkg.Fset, invalidMethods)}, nil +} + +func findInvalidMethods(methods []*ast.Field) []*ast.Field { + var invalid []*ast.Field + for _, m := range methods { + if !hasValidMinimumVersionComment(m.Doc.Text()) { + invalid = append(invalid, m) + } + } + return invalid +} + +func hasValidMinimumVersionComment(s string) bool { + return version.ExtractMinimumVersionFromComment(s) != "" +} + +func renderErrors(fset *token.FileSet, methods []*ast.Field) []string { + var out []string + for _, m := range methods { + out = append(out, renderWithFilePosition(fset, m.Pos(), fmt.Sprintf("missing a minimum server version comment on method %s", m.Names[0].Name))) + } + return out +} diff --git a/plugin/checker/main_test.go b/plugin/checker/check_api_test.go similarity index 69% rename from plugin/checker/main_test.go rename to plugin/checker/check_api_test.go index c05819b850..529da48947 100644 --- a/plugin/checker/main_test.go +++ b/plugin/checker/check_api_test.go @@ -10,29 +10,32 @@ import ( "github.com/stretchr/testify/assert" ) -func TestRunCheck(t *testing.T) { +func TestCheckAPIVersionComments(t *testing.T) { testCases := []struct { name, pkgPath, err string + expected result }{ { name: "valid comments", - pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/test/valid", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/valid", err: "", }, { name: "invalid comments", - pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/test/invalid", - err: "test/invalid/invalid.go:15:2: missing a minimum server version comment\n", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/invalid", + expected: result{ + Errors: []string{"internal/test/invalid/invalid.go:15:2: missing a minimum server version comment on method InvalidMethod"}, + }, }, { name: "missing API interface", - pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/test/missing", - err: "could not find API interface in package github.com/mattermost/mattermost-server/plugin/checker/test/missing", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/missing", + err: "could not find API interface", }, { name: "non-existent package path", - pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/test/does_not_exist", - err: "could not find API interface in package github.com/mattermost/mattermost-server/plugin/checker/test/does_not_exist", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/does_not_exist", + err: "could not find API interface", }, } @@ -43,7 +46,8 @@ func TestRunCheck(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err := runCheck(tc.pkgPath) + res, err := checkAPIVersionComments(tc.pkgPath) + assert.Equal(t, res, tc.expected) if tc.err != "" { assert.EqualError(t, err, tc.err) diff --git a/plugin/checker/check_helpers.go b/plugin/checker/check_helpers.go new file mode 100644 index 0000000000..8c2de7f5d1 --- /dev/null +++ b/plugin/checker/check_helpers.go @@ -0,0 +1,137 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package main + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + + "github.com/mattermost/mattermost-server/plugin/checker/internal/asthelpers" + "github.com/mattermost/mattermost-server/plugin/checker/internal/version" + + "github.com/pkg/errors" +) + +func checkHelpersVersionComments(pkgPath string) (result, error) { + pkg, err := asthelpers.GetPackage(pkgPath) + if err != nil { + return result{}, err + } + + api, apiIdent, err := asthelpers.FindInterfaceWithIdent("API", pkg.Syntax) + if err != nil { + return result{}, err + } + + apiObj := pkg.TypesInfo.ObjectOf(apiIdent) + if apiObj == nil { + return result{}, errors.New("could not find type object for API interface") + } + + helpers, err := asthelpers.FindInterface("Helpers", pkg.Syntax) + if err != nil { + return result{}, err + } + + apiVersions := mapMinimumVersionsByMethodName(api.Methods.List) + + helpersPositions := mapPositionsByMethodName(helpers.Methods.List) + helpersVersions := mapMinimumVersionsByMethodName(helpers.Methods.List) + + implMethods := asthelpers.FindReceiverMethods("HelpersImpl", pkg.Syntax) + implVersions := mapEffectiveVersionByMethod(pkg.TypesInfo, apiObj.Type(), apiVersions, implMethods) + + return validateMethods(pkg.Fset, helpersPositions, helpersVersions, implVersions), nil +} + +func validateMethods( + fset *token.FileSet, + helpersPositions map[string]token.Pos, + helpersVersions map[string]version.V, + implVersions map[string]version.V, +) result { + var res result + + for name, helperVer := range helpersVersions { + pos := helpersPositions[name] + + implVer, ok := implVersions[name] + if !ok { + res.Errors = append(res.Errors, renderWithFilePosition( + fset, + pos, + fmt.Sprintf("missing implementation for method %s", name)), + ) + continue + } + + if helperVer == "" { + res.Errors = append(res.Errors, renderWithFilePosition( + fset, + pos, + fmt.Sprintf("missing a minimum server version comment on method %s", name)), + ) + continue + } + + if helperVer == implVer { + continue + } + + if helperVer.LessThan(implVer) { + res.Errors = append(res.Errors, renderWithFilePosition( + fset, + pos, + fmt.Sprintf("documented minimum server version too low on method %s", name)), + ) + } else { + res.Warnings = append(res.Warnings, renderWithFilePosition( + fset, + pos, + fmt.Sprintf("documented minimum server version too high on method %s", name)), + ) + } + } + + return res +} + +func mapEffectiveVersionByMethod(info *types.Info, apiType types.Type, versions map[string]version.V, methods []*ast.FuncDecl) map[string]version.V { + effectiveVersions := map[string]version.V{} + for _, m := range methods { + apiMethodsCalled := asthelpers.FindMethodsCalledOnType(info, apiType, m) + effectiveVersions[m.Name.Name] = getEffectiveMinimumVersion(versions, apiMethodsCalled) + } + return effectiveVersions +} + +func mapMinimumVersionsByMethodName(methods []*ast.Field) map[string]version.V { + versions := map[string]version.V{} + for _, m := range methods { + versions[m.Names[0].Name] = version.V(version.ExtractMinimumVersionFromComment(m.Doc.Text())) + } + return versions +} + +func mapPositionsByMethodName(methods []*ast.Field) map[string]token.Pos { + pos := map[string]token.Pos{} + for _, m := range methods { + pos[m.Names[0].Name] = m.Pos() + } + return pos +} + +func getEffectiveMinimumVersion(info map[string]version.V, methods []string) version.V { + var highest version.V + for _, m := range methods { + if current, ok := info[m]; ok { + if current.GreaterThanOrEqualTo(highest) { + highest = current + } + } + } + return highest +} diff --git a/plugin/checker/check_helpers_test.go b/plugin/checker/check_helpers_test.go new file mode 100644 index 0000000000..d87d976d41 --- /dev/null +++ b/plugin/checker/check_helpers_test.go @@ -0,0 +1,48 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCheckHelpersVersionComments(t *testing.T) { + testCases := []struct { + name, pkgPath string + expected result + err string + }{ + { + name: "valid versions", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/valid", + expected: result{}, + }, + { + name: "invalid versions", + pkgPath: "github.com/mattermost/mattermost-server/plugin/checker/internal/test/invalid", + expected: result{ + Errors: []string{"internal/test/invalid/invalid.go:20:2: documented minimum server version too low on method LowerVersionMethod"}, + Warnings: []string{"internal/test/invalid/invalid.go:23:2: documented minimum server version too high on method HigherVersionMethod"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert := assert.New(t) + + res, err := checkHelpersVersionComments(tc.pkgPath) + assert.Equal(tc.expected, res) + + if tc.err != "" { + assert.EqualError(err, tc.err) + } else { + assert.NoError(err) + } + + }) + } +} diff --git a/plugin/checker/internal/asthelpers/helpers.go b/plugin/checker/internal/asthelpers/helpers.go new file mode 100644 index 0000000000..1caf72e78d --- /dev/null +++ b/plugin/checker/internal/asthelpers/helpers.go @@ -0,0 +1,129 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package asthelpers + +import ( + "go/ast" + "go/types" + + "github.com/pkg/errors" + "golang.org/x/tools/go/packages" +) + +func GetPackage(pkgPath string) (*packages.Package, error) { + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo, + } + pkgs, err := packages.Load(cfg, pkgPath) + if err != nil { + return nil, err + } + + if len(pkgs) == 0 { + return nil, errors.Errorf("could not find package %s", pkgPath) + } + return pkgs[0], nil +} + +func FindInterface(name string, files []*ast.File) (*ast.InterfaceType, error) { + iface, _, err := FindInterfaceWithIdent(name, files) + return iface, err +} + +func FindInterfaceWithIdent(name string, files []*ast.File) (*ast.InterfaceType, *ast.Ident, error) { + var ( + ident *ast.Ident + iface *ast.InterfaceType + ) + + for _, f := range files { + ast.Inspect(f, func(n ast.Node) bool { + if t, ok := n.(*ast.TypeSpec); ok { + if iface != nil { + return false + } + + if i, ok := t.Type.(*ast.InterfaceType); ok && t.Name.Name == name { + ident = t.Name + iface = i + return false + } + } + return true + }) + + if iface != nil { + return iface, ident, nil + } + } + return nil, nil, errors.Errorf("could not find %s interface", name) +} + +func FindMethodsCalledOnType(info *types.Info, typ types.Type, caller *ast.FuncDecl) []string { + var methods []string + + ast.Inspect(caller, func(n ast.Node) bool { + if s, ok := n.(*ast.SelectorExpr); ok { + + var receiver *ast.Ident + switch r := s.X.(type) { + case *ast.Ident: + // Left-hand side of the selector is an identifier, eg: + // + // a := p.API + // a.GetTeams() + // + receiver = r + case *ast.SelectorExpr: + // Left-hand side of the selector is a selector, eg: + // + // p.API.GetTeams() + // + receiver = r.Sel + } + + if receiver != nil { + obj := info.ObjectOf(receiver) + if obj != nil && types.Identical(obj.Type(), typ) { + methods = append(methods, s.Sel.Name) + } + return false + } + + } + return true + }) + + return methods +} + +func FindReceiverMethods(receiverName string, files []*ast.File) []*ast.FuncDecl { + var fns []*ast.FuncDecl + for _, f := range files { + ast.Inspect(f, func(n ast.Node) bool { + if fn, ok := n.(*ast.FuncDecl); ok { + r := extractReceiverTypeName(fn) + if r == receiverName { + fns = append(fns, fn) + } + } + return true + }) + } + return fns +} + +func extractReceiverTypeName(fn *ast.FuncDecl) string { + if fn.Recv != nil { + t := fn.Recv.List[0].Type + // Unwrap the pointer type (a star expression) + if se, ok := t.(*ast.StarExpr); ok { + t = se.X + } + if id, ok := t.(*ast.Ident); ok { + return id.Name + } + } + return "" +} diff --git a/plugin/checker/test/invalid/invalid.go b/plugin/checker/internal/test/invalid/invalid.go similarity index 57% rename from plugin/checker/test/invalid/invalid.go rename to plugin/checker/internal/test/invalid/invalid.go index dc7ec0cbbc..56f413713c 100644 --- a/plugin/checker/test/invalid/invalid.go +++ b/plugin/checker/internal/test/invalid/invalid.go @@ -14,3 +14,23 @@ type API interface { // plugin comment checker with an invalid comment. InvalidMethod() } + +type Helpers interface { + // Minimum server version: 1.1 + LowerVersionMethod() + + // Minimum server version: 1.3 + HigherVersionMethod() +} + +type HelpersImpl struct { + api API +} + +func (h *HelpersImpl) LowerVersionMethod() { + h.api.ValidMethod() +} + +func (h *HelpersImpl) HigherVersionMethod() { + h.api.ValidMethod() +} diff --git a/plugin/checker/test/missing/missing.go b/plugin/checker/internal/test/missing/missing.go similarity index 100% rename from plugin/checker/test/missing/missing.go rename to plugin/checker/internal/test/missing/missing.go diff --git a/plugin/checker/internal/test/valid/valid.go b/plugin/checker/internal/test/valid/valid.go new file mode 100644 index 0000000000..ecdb379d6b --- /dev/null +++ b/plugin/checker/internal/test/valid/valid.go @@ -0,0 +1,44 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package valid + +type API interface { + // ValidMethod is a fake method for testing the + // plugin comment checker with a valid comment. + // + // Minimum server version: 1.2.3 + ValidMethod() + + // Minimum server version: 1.5 + NewerValidMethod() +} + +type Helpers interface { + // Minimum server version: 1.2.3 + ValidHelperMethod() + + // Minimum server version: 1.5 + NewerValidHelperMethod() + + // Minimum server version: 1.5 + IndirectReferenceMethod() +} + +type HelpersImpl struct { + api API +} + +func (h *HelpersImpl) ValidHelperMethod() { + h.api.ValidMethod() +} + +func (h *HelpersImpl) NewerValidHelperMethod() { + h.api.NewerValidMethod() + h.api.ValidMethod() +} + +func (h *HelpersImpl) IndirectReferenceMethod() { + a := h.api + a.NewerValidMethod() +} diff --git a/plugin/checker/internal/version/comments.go b/plugin/checker/internal/version/comments.go new file mode 100644 index 0000000000..ac003d3070 --- /dev/null +++ b/plugin/checker/internal/version/comments.go @@ -0,0 +1,22 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package version + +import ( + "regexp" + "strings" +) + +var versionCommentRE = regexp.MustCompile(`^Minimum server version: (\d+\.\d+(?:\.\d+[\w-]*)?)$`) + +func ExtractMinimumVersionFromComment(s string) string { + lines := strings.Split(strings.TrimSpace(s), "\n") + if len(lines) > 0 { + lastLine := lines[len(lines)-1] + if m := versionCommentRE.FindStringSubmatch(lastLine); len(m) >= 1 { + return m[1] + } + } + return "" +} diff --git a/plugin/checker/internal/version/comments_test.go b/plugin/checker/internal/version/comments_test.go new file mode 100644 index 0000000000..4d71f71499 --- /dev/null +++ b/plugin/checker/internal/version/comments_test.go @@ -0,0 +1,49 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package version + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExtractVersionFromComment(t *testing.T) { + testCases := []struct { + input string + expected string + }{ + { + input: "This is a comment.\n\nMinimum server version: 1.2.3-rc1\n", + expected: "1.2.3-rc1", + }, + { + input: "This is a comment.\n\nMinimum server version: 1.2.3\n", + expected: "1.2.3", + }, + { + input: "This is a comment.\n\nMinimum server version: 1.2\n", + expected: "1.2", + }, + { + input: "This is a comment.\n\nMinimum server version: 1\n", + expected: "", + }, + { + input: "This is a comment.\n", + expected: "", + }, + { + input: "", + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("%+v", tc), func(t *testing.T) { + assert.Equal(t, tc.expected, ExtractMinimumVersionFromComment(tc.input)) + }) + } +} diff --git a/plugin/checker/internal/version/version.go b/plugin/checker/internal/version/version.go new file mode 100644 index 0000000000..997179250f --- /dev/null +++ b/plugin/checker/internal/version/version.go @@ -0,0 +1,80 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package version + +import ( + "regexp" + "strconv" + "strings" +) + +type V string + +func (v V) GreaterThanOrEqualTo(other V) bool { + return !v.LessThan(other) +} + +func (v V) LessThan(other V) bool { + leftParts, leftCount := split(v) + rightParts, rightCount := split(other) + + var length int + if leftCount < rightCount { + length = rightCount + } else { + length = leftCount + } + + for i := 0; i < length; i++ { + var left, right string + + if i < leftCount { + left = leftParts[i] + } + + if i < rightCount { + right = rightParts[i] + } + + if left == right { + continue + } + + leftInt := parseInt(left) + rightInt := parseInt(right) + + isNumericalComparison := leftInt != nil && rightInt != nil + + if isNumericalComparison { + return *leftInt < *rightInt + } + + return left < right + } + + return false +} + +func split(v V) ([]string, int) { + var chunks []string + + for _, part := range strings.Split(string(v), ".") { + chunks = append(chunks, splitNumericalChunks(part)...) + } + + return chunks, len(chunks) +} + +var numericalOrAlphaRE = regexp.MustCompile(`(\d+|\D+)`) + +func splitNumericalChunks(s string) []string { + return numericalOrAlphaRE.FindAllString(s, -1) +} + +func parseInt(s string) *int64 { + if n, err := strconv.ParseInt(s, 10, 64); err == nil { + return &n + } + return nil +} diff --git a/plugin/checker/internal/version/version_test.go b/plugin/checker/internal/version/version_test.go new file mode 100644 index 0000000000..54bdf65b5c --- /dev/null +++ b/plugin/checker/internal/version/version_test.go @@ -0,0 +1,58 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package version + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestVersionComparison(t *testing.T) { + testCases := []struct { + a, b V + }{ + { + a: V("1.2"), + b: V("1.10"), + }, + { + a: V("1.2.1"), + b: V("1.2.3"), + }, + { + a: V("1.2"), + b: V("1.2.3"), + }, + { + a: V("1.2.1"), + b: V("1.2.3"), + }, + { + a: V("1.1"), + b: V("1.2.3"), + }, + { + a: V("1.2.3"), + b: V("1.3"), + }, + { + a: V("1.2.1-rc2"), + b: V("1.2.1-rc10"), + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("%+v", tc), func(t *testing.T) { + assert.True(t, tc.a.LessThan(tc.b)) + assert.False(t, tc.b.LessThan(tc.a)) + + assert.True(t, tc.b.GreaterThanOrEqualTo(tc.a)) + assert.False(t, tc.a.GreaterThanOrEqualTo(tc.b)) + }) + } + + assert.True(t, V("1.2").GreaterThanOrEqualTo("1.2")) +} diff --git a/plugin/checker/main.go b/plugin/checker/main.go index 724bf18278..d7e956426e 100644 --- a/plugin/checker/main.go +++ b/plugin/checker/main.go @@ -4,123 +4,69 @@ package main import ( - "bytes" "fmt" "os" - "path/filepath" - "regexp" + "sort" "strings" - - "go/ast" - - "golang.org/x/tools/go/packages" - - "github.com/pkg/errors" ) const pluginPackagePath = "github.com/mattermost/mattermost-server/plugin" +type result struct { + Warnings []string + Errors []string +} + +type checkFn func(pkgPath string) (result, error) + +var checks = []checkFn{ + checkAPIVersionComments, + checkHelpersVersionComments, +} + func main() { - if err := runCheck(pluginPackagePath); err != nil { + var res result + for _, check := range checks { + res = runCheck(res, check) + } + + var msgs []string + msgs = append(msgs, res.Errors...) + msgs = append(msgs, res.Warnings...) + sort.Strings(msgs) + + if len(msgs) > 0 { fmt.Fprintln(os.Stderr, "#", pluginPackagePath) - fmt.Fprintln(os.Stderr, err) + fmt.Fprintln(os.Stderr, strings.Join(msgs, "\n")) + } + + if len(res.Errors) > 0 { os.Exit(1) } } -func runCheck(pkgPath string) error { - pkg, err := getPackage(pkgPath) +func runCheck(prev result, fn checkFn) result { + res, err := fn(pluginPackagePath) if err != nil { - return err + prev.Errors = append(prev.Errors, err.Error()) + return prev } - apiInterface := findAPIInterface(pkg.Syntax) - if apiInterface == nil { - return errors.Errorf("could not find API interface in package %s", pkgPath) + if len(res.Warnings) > 0 { + prev.Warnings = append(prev.Warnings, mapWarnings(res.Warnings)...) } - invalidMethods := findInvalidMethods(apiInterface.Methods.List) - if len(invalidMethods) > 0 { - return errors.New(renderErrorMessage(pkg, invalidMethods)) + if len(res.Errors) > 0 { + prev.Errors = append(prev.Errors, res.Errors...) } - return nil + + return prev } -func getPackage(pkgPath string) (*packages.Package, error) { - cfg := &packages.Config{ - Mode: packages.NeedName | packages.NeedTypes | packages.NeedSyntax, +func mapWarnings(ss []string) []string { + var out []string + for _, s := range ss { + out = append(out, "[warn] "+s) } - pkgs, err := packages.Load(cfg, pkgPath) - if err != nil { - return nil, err - } - - if len(pkgs) == 0 { - return nil, errors.Errorf("could not find package %s", pkgPath) - } - return pkgs[0], nil -} - -func findAPIInterface(files []*ast.File) *ast.InterfaceType { - for _, f := range files { - var iface *ast.InterfaceType - - ast.Inspect(f, func(n ast.Node) bool { - if t, ok := n.(*ast.TypeSpec); ok { - if i, ok := t.Type.(*ast.InterfaceType); ok && t.Name.Name == "API" { - iface = i - return false - } - } - return true - }) - - if iface != nil { - return iface - } - } - return nil -} - -func findInvalidMethods(methods []*ast.Field) []*ast.Field { - var invalid []*ast.Field - for _, m := range methods { - if !hasValidMinimumVersionComment(m.Doc.Text()) { - invalid = append(invalid, m) - } - } - return invalid -} - -var versionRequirementRE = regexp.MustCompile(`^Minimum server version: \d+\.\d+(\.\d+)?$`) - -func hasValidMinimumVersionComment(s string) bool { - lines := strings.Split(strings.TrimSpace(s), "\n") - if len(lines) > 0 { - lastLine := lines[len(lines)-1] - return versionRequirementRE.MatchString(lastLine) - } - return false -} - -func renderErrorMessage(pkg *packages.Package, methods []*ast.Field) string { - cwd, _ := os.Getwd() - out := &bytes.Buffer{} - - for _, m := range methods { - pos := pkg.Fset.Position(m.Pos()) - filename, err := filepath.Rel(cwd, pos.Filename) - if err != nil { - // If deriving a relative path fails for some reason, - // we prefer to still print the absolute path to the file. - filename = pos.Filename - } - fmt.Fprintf(out, - "%s:%d:%d: missing a minimum server version comment\n", - filename, - pos.Line, - pos.Column, - ) - } - return out.String() + return out } diff --git a/plugin/checker/render.go b/plugin/checker/render.go new file mode 100644 index 0000000000..885c09e986 --- /dev/null +++ b/plugin/checker/render.go @@ -0,0 +1,29 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package main + +import ( + "fmt" + "go/token" + "os" + "path/filepath" +) + +func renderWithFilePosition(fset *token.FileSet, pos token.Pos, msg string) string { + var cwd string + if d, err := os.Getwd(); err == nil { + cwd = d + } + + fpos := fset.Position(pos) + + filename, err := filepath.Rel(cwd, fpos.Filename) + if err != nil { + // If deriving a relative path fails for some reason, + // we prefer to still print the absolute path to the file. + filename = fpos.Filename + } + + return fmt.Sprintf("%s:%d:%d: %s", filename, fpos.Line, fpos.Column, msg) +} diff --git a/plugin/checker/test/valid/valid.go b/plugin/checker/test/valid/valid.go deleted file mode 100644 index d1fceafcac..0000000000 --- a/plugin/checker/test/valid/valid.go +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See License.txt for license information. - -package valid - -type API interface { - // ValidMethod is a fake method for testing the - // plugin comment checker with a valid comment. - // - // Minimum server version: 1.2.3 - ValidMethod() -} diff --git a/plugin/helpers.go b/plugin/helpers.go index 5f96bf4da1..94d66c0683 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -8,9 +8,13 @@ import "github.com/mattermost/mattermost-server/model" type Helpers interface { // EnsureBot either returns an existing bot user matching the given bot, or creates a bot user from the given bot. // Returns the id of the resulting bot. + // + // Minimum server version: 5.10 EnsureBot(bot *model.Bot) (string, error) // KVSetJSON stores a key-value pair, unique per plugin, marshalling the given value as a JSON string. + // + // Minimum server version: 5.2 KVSetJSON(key string, value interface{}) error // KVCompareAndSetJSON updates a key-value pair, unique per plugin, but only if the current value matches the given oldValue after marshalling as a JSON string. @@ -31,6 +35,8 @@ type Helpers interface { KVCompareAndDeleteJSON(key string, oldValue interface{}) (bool, error) // KVGetJSON retrieves a value based on the key, unique per plugin, unmarshalling the previously set JSON string into the given value. Returns true if the key exists. + // + // Minimum server version: 5.2 KVGetJSON(key string, value interface{}) (bool, error) // KVSetWithExpiryJSON stores a key-value pair with an expiry time, unique per plugin, marshalling the given value as a JSON string.