From f493ff4d938219175d70b8b0181f3c9f2951a36d Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Sun, 6 Dec 2020 17:05:13 -0500 Subject: [PATCH] New Check: R019: check for `(*schema.ResourceData).HasChanges()` receiver method usage with many arguments (#217) Reference: https://github.com/bflad/tfproviderlint/issues/212 --- CHANGELOG.md | 1 + README.md | 1 + passes/R019/R019.go | 62 +++++++++++++++++++ passes/R019/R019_test.go | 26 ++++++++ passes/R019/README.md | 28 +++++++++ passes/R019/testdata/src/a/alias.go | 24 +++++++ passes/R019/testdata/src/a/alias_v2.go | 24 +++++++ passes/R019/testdata/src/a/main.go | 24 +++++++ passes/R019/testdata/src/a/main_v2.go | 24 +++++++ passes/R019/testdata/src/a/outside_package.go | 11 ++++ passes/R019/testdata/src/a/schema/schema.go | 7 +++ passes/R019/testdata/src/a/vendor | 1 + passes/R019/testdata/src/threshold/main.go | 17 +++++ passes/R019/testdata/src/threshold/vendor | 1 + passes/checks.go | 2 + .../resourcedatahaschangecallexpr.go | 14 +++++ .../resourcedatahaschangecallexpr_test.go | 15 +++++ .../resourcedatahaschangescallexpr.go | 14 +++++ .../resourcedatahaschangescallexpr_test.go | 15 +++++ 19 files changed, 311 insertions(+) create mode 100644 passes/R019/R019.go create mode 100644 passes/R019/R019_test.go create mode 100644 passes/R019/README.md create mode 100644 passes/R019/testdata/src/a/alias.go create mode 100644 passes/R019/testdata/src/a/alias_v2.go create mode 100644 passes/R019/testdata/src/a/main.go create mode 100644 passes/R019/testdata/src/a/main_v2.go create mode 100644 passes/R019/testdata/src/a/outside_package.go create mode 100644 passes/R019/testdata/src/a/schema/schema.go create mode 120000 passes/R019/testdata/src/a/vendor create mode 100644 passes/R019/testdata/src/threshold/main.go create mode 120000 passes/R019/testdata/src/threshold/vendor create mode 100644 passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr.go create mode 100644 passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr_test.go create mode 100644 passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr.go create mode 100644 passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 261c4657..adf38814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ FEATURES +* **New Check:** `R019`: check for `(*schema.ResourceData).HasChanges()` receiver method usage with many arguments * **New Check:** `V009`: check for `validation.StringMatch()` calls with empty message argument * **New Check:** `V010`: check for `validation.StringDoesNotMatch()` calls with empty message argument diff --git a/README.md b/README.md index ae9c3830..be87b20a 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,7 @@ Standard lint checks are enabled by default in the `tfproviderlint` tool. Opt-in | [R016](passes/R016) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.PrefixedUniqueId()` value | AST | | [R017](passes/R017) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `time.Now()` value | AST | | [R018](passes/R018) | check for `time.Sleep()` function usage | AST | +| [R019](passes/R019) | check for `(*schema.ResourceData).HasChanges()` receiver method usage with many arguments | AST | ### Standard Schema Checks diff --git a/passes/R019/R019.go b/passes/R019/R019.go new file mode 100644 index 00000000..82f8732f --- /dev/null +++ b/passes/R019/R019.go @@ -0,0 +1,62 @@ +package R019 + +import ( + "flag" + "go/ast" + + "golang.org/x/tools/go/analysis" + + "github.com/bflad/tfproviderlint/passes/commentignore" + "github.com/bflad/tfproviderlint/passes/helper/schema/resourcedatahaschangescallexpr" +) + +const Doc = `check for (*schema.ResourceData).HasChanges() calls with many arguments + +The R019 analyzer reports when there are a large number of arguments being +passed to (*schema.ResourceData).HasChanges(), which it may be preferable to +use (*schema.ResourceData).HasChangesExcept() instead. + +Optional parameters: + -threshold=5 Number of arguments for reporting +` + +const analyzerName = "R019" + +var ( + threshold int +) + +var Analyzer = &analysis.Analyzer{ + Name: analyzerName, + Doc: Doc, + Flags: parseFlags(), + Requires: []*analysis.Analyzer{ + commentignore.Analyzer, + resourcedatahaschangescallexpr.Analyzer, + }, + Run: run, +} + +func parseFlags() flag.FlagSet { + var flags = flag.NewFlagSet(analyzerName, flag.ExitOnError) + flags.IntVar(&threshold, "threshold", 5, "Number of arguments for reporting") + return *flags +} + +func run(pass *analysis.Pass) (interface{}, error) { + ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer) + callExprs := pass.ResultOf[resourcedatahaschangescallexpr.Analyzer].([]*ast.CallExpr) + for _, callExpr := range callExprs { + if ignorer.ShouldIgnore(analyzerName, callExpr) { + continue + } + + if len(callExpr.Args) < threshold { + continue + } + + pass.Reportf(callExpr.Pos(), "%s: d.HasChanges() has many arguments, consider d.HasChangesExcept()", analyzerName) + } + + return nil, nil +} diff --git a/passes/R019/R019_test.go b/passes/R019/R019_test.go new file mode 100644 index 00000000..67d5fd65 --- /dev/null +++ b/passes/R019/R019_test.go @@ -0,0 +1,26 @@ +package R019_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/bflad/tfproviderlint/passes/R019" +) + +func TestR019(t *testing.T) { + testdata := analysistest.TestData() + analyzer := R019.Analyzer + analysistest.Run(t, testdata, analyzer, "a") +} + +func TestR019Threshold(t *testing.T) { + testdata := analysistest.TestData() + analyzer := R019.Analyzer + + if err := analyzer.Flags.Set("threshold", "3"); err != nil { + t.Fatalf("error setting threshold flag: %s", err) + } + + analysistest.Run(t, testdata, analyzer, "threshold") +} diff --git a/passes/R019/README.md b/passes/R019/README.md new file mode 100644 index 00000000..c47870ab --- /dev/null +++ b/passes/R019/README.md @@ -0,0 +1,28 @@ +# R019 + +The R019 analyzer reports when there are a large number of arguments being passed to [`(*schema.ResourceData).HasChanges()`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema#ResourceData.HasChanges), which it may be preferable to use [`(*schema.ResourceData).HasChangesExcept()`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema#ResourceData.HasChangesExcept) instead. + +## Optional Arguments + +- `-threshold=5` Number of arguments before reporting + +## Flagged Code + +```go +d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") +``` + +## Passing Code + +```go +d.HasChangesExcept("metadata_attr") +``` + +## Ignoring Reports + +Singular reports can be ignored by adding the a `//lintignore:R019` Go code comment at the end of the offending line or on the line immediately proceding, e.g. + +```go +//lintignore:R019 +d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") +``` diff --git a/passes/R019/testdata/src/a/alias.go b/passes/R019/testdata/src/a/alias.go new file mode 100644 index 00000000..e5a7b591 --- /dev/null +++ b/passes/R019/testdata/src/a/alias.go @@ -0,0 +1,24 @@ +package a + +import ( + s "github.com/hashicorp/terraform-plugin-sdk/helper/schema" +) + +func falias() { + var d s.ResourceData + + // Comment ignored + + //lintignore:R019 + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019 + + // Failing + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)" + + // Passing + + d.HasChanges("attr1", "attr2", "attr3", "attr4") +} diff --git a/passes/R019/testdata/src/a/alias_v2.go b/passes/R019/testdata/src/a/alias_v2.go new file mode 100644 index 00000000..172aa2a1 --- /dev/null +++ b/passes/R019/testdata/src/a/alias_v2.go @@ -0,0 +1,24 @@ +package a + +import ( + s "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func falias_v2() { + var d s.ResourceData + + // Comment ignored + + //lintignore:R019 + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019 + + // Failing + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)" + + // Passing + + d.HasChanges("attr1", "attr2", "attr3", "attr4") +} diff --git a/passes/R019/testdata/src/a/main.go b/passes/R019/testdata/src/a/main.go new file mode 100644 index 00000000..471eb83b --- /dev/null +++ b/passes/R019/testdata/src/a/main.go @@ -0,0 +1,24 @@ +package a + +import ( + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" +) + +func f() { + var d schema.ResourceData + + // Comment ignored + + //lintignore:R019 + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019 + + // Failing + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)" + + // Passing + + d.HasChanges("attr1", "attr2", "attr3", "attr4") +} diff --git a/passes/R019/testdata/src/a/main_v2.go b/passes/R019/testdata/src/a/main_v2.go new file mode 100644 index 00000000..e28a5fa9 --- /dev/null +++ b/passes/R019/testdata/src/a/main_v2.go @@ -0,0 +1,24 @@ +package a + +import ( + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func f_v2() { + var d schema.ResourceData + + // Comment ignored + + //lintignore:R019 + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019 + + // Failing + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)" + + // Passing + + d.HasChanges("attr1", "attr2", "attr3", "attr4") +} diff --git a/passes/R019/testdata/src/a/outside_package.go b/passes/R019/testdata/src/a/outside_package.go new file mode 100644 index 00000000..d3f001e3 --- /dev/null +++ b/passes/R019/testdata/src/a/outside_package.go @@ -0,0 +1,11 @@ +package a + +import ( + "a/schema" +) + +func foutside() { + var d schema.ResourceData + + d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") +} diff --git a/passes/R019/testdata/src/a/schema/schema.go b/passes/R019/testdata/src/a/schema/schema.go new file mode 100644 index 00000000..7bd746ab --- /dev/null +++ b/passes/R019/testdata/src/a/schema/schema.go @@ -0,0 +1,7 @@ +package schema + +type ResourceData struct{} + +func (d *ResourceData) HasChanges(keys ...string) bool { + return false +} diff --git a/passes/R019/testdata/src/a/vendor b/passes/R019/testdata/src/a/vendor new file mode 120000 index 00000000..776800d2 --- /dev/null +++ b/passes/R019/testdata/src/a/vendor @@ -0,0 +1 @@ +../../../../../vendor \ No newline at end of file diff --git a/passes/R019/testdata/src/threshold/main.go b/passes/R019/testdata/src/threshold/main.go new file mode 100644 index 00000000..00b5b0ff --- /dev/null +++ b/passes/R019/testdata/src/threshold/main.go @@ -0,0 +1,17 @@ +package a + +import ( + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" +) + +func f() { + var d schema.ResourceData + + // Failing with lower threshold + + d.HasChanges("attr1", "attr2", "attr3") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)" + + // Passing with lower threshold + + d.HasChanges("attr1", "attr2") +} diff --git a/passes/R019/testdata/src/threshold/vendor b/passes/R019/testdata/src/threshold/vendor new file mode 120000 index 00000000..776800d2 --- /dev/null +++ b/passes/R019/testdata/src/threshold/vendor @@ -0,0 +1 @@ +../../../../../vendor \ No newline at end of file diff --git a/passes/checks.go b/passes/checks.go index b780c873..92cd064a 100644 --- a/passes/checks.go +++ b/passes/checks.go @@ -28,6 +28,7 @@ import ( "github.com/bflad/tfproviderlint/passes/R016" "github.com/bflad/tfproviderlint/passes/R017" "github.com/bflad/tfproviderlint/passes/R018" + "github.com/bflad/tfproviderlint/passes/R019" "github.com/bflad/tfproviderlint/passes/S001" "github.com/bflad/tfproviderlint/passes/S002" "github.com/bflad/tfproviderlint/passes/S003" @@ -109,6 +110,7 @@ var AllChecks = []*analysis.Analyzer{ R016.Analyzer, R017.Analyzer, R018.Analyzer, + R019.Analyzer, S001.Analyzer, S002.Analyzer, S003.Analyzer, diff --git a/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr.go b/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr.go new file mode 100644 index 00000000..9b6ef32f --- /dev/null +++ b/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr.go @@ -0,0 +1,14 @@ +package resourcedatahaschangecallexpr + +import ( + "github.com/bflad/tfproviderlint/helper/analysisutils" + "github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema" +) + +var Analyzer = analysisutils.ReceiverMethodCallExprAnalyzer( + "resourcedatahaschangecallexpr", + schema.IsReceiverMethod, + schema.PackagePath, + schema.TypeNameResourceData, + "HasChange", +) diff --git a/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr_test.go b/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr_test.go new file mode 100644 index 00000000..432262cf --- /dev/null +++ b/passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr_test.go @@ -0,0 +1,15 @@ +package resourcedatahaschangecallexpr + +import ( + "testing" + + "golang.org/x/tools/go/analysis" +) + +func TestValidateAnalyzer(t *testing.T) { + err := analysis.Validate([]*analysis.Analyzer{Analyzer}) + + if err != nil { + t.Fatal(err) + } +} diff --git a/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr.go b/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr.go new file mode 100644 index 00000000..b31b46d1 --- /dev/null +++ b/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr.go @@ -0,0 +1,14 @@ +package resourcedatahaschangescallexpr + +import ( + "github.com/bflad/tfproviderlint/helper/analysisutils" + "github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema" +) + +var Analyzer = analysisutils.ReceiverMethodCallExprAnalyzer( + "resourcedatahaschangescallexpr", + schema.IsReceiverMethod, + schema.PackagePath, + schema.TypeNameResourceData, + "HasChanges", +) diff --git a/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr_test.go b/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr_test.go new file mode 100644 index 00000000..874b0afa --- /dev/null +++ b/passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr_test.go @@ -0,0 +1,15 @@ +package resourcedatahaschangescallexpr + +import ( + "testing" + + "golang.org/x/tools/go/analysis" +) + +func TestValidateAnalyzer(t *testing.T) { + err := analysis.Validate([]*analysis.Analyzer{Analyzer}) + + if err != nil { + t.Fatal(err) + } +}