Skip to content

Commit

Permalink
Ignored case changes in bigquery table schema mode/type (hashicorp#4943)
Browse files Browse the repository at this point in the history 
* Ignored case changes in bigquery table schema mode/type

BigQuery table schema normalizes the mode and type to uppercase, which causes a permadiff for the end user if they set it in lowercase.

Also removed tests for setting type to nil, because a field type being nil is invalid.

Resolved hashicorp/terraform-provider-google#9472

* Handle invalid input from users without a panic

Signed-off-by: Modular Magician <[email protected]>
  • Loading branch information
@modular-magician
modular-magician committed Jul 2, 2021
1 parent 7387850 commit a25ffd0
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 36 deletions.
3 changes: 3 additions & 0 deletions .changelog/4943.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
bigquery: Fixed permadiff due to lowercase mode/type in `google_bigquery_table.schema`
```
49 changes: 32 additions & 17 deletions google-beta/resource_bigquery_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"log"
"sort"
"strconv"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -130,14 +131,17 @@ func bigQueryTableMapKeyOverride(key string, objectA, objectB map[string]interfa
valB := objectB[key]
switch key {
case "mode":
eq := bigQueryTableModeEq(valA, valB)
eq := bigQueryTableNormalizeMode(valA) == bigQueryTableNormalizeMode(valB)
return eq
case "description":
equivalentSet := []interface{}{nil, ""}
eq := valueIsInArray(valA, equivalentSet) && valueIsInArray(valB, equivalentSet)
return eq
case "type":
return bigQueryTableTypeEq(valA, valB)
if valA == nil || valB == nil {
return false
}
return bigQueryTableTypeEq(valA.(string), valB.(string))
}

// otherwise rely on default behavior
Expand Down Expand Up @@ -167,28 +171,32 @@ func bigQueryTableSchemaDiffSuppress(name, old, new string, _ *schema.ResourceDa
return eq
}

func bigQueryTableTypeEq(old, new interface{}) bool {
func bigQueryTableTypeEq(old, new string) bool {
// Do case-insensitive comparison. https://github.com/hashicorp/terraform-provider-google/issues/9472
oldUpper := strings.ToUpper(old)
newUpper := strings.ToUpper(new)

equivalentSet1 := []interface{}{"INTEGER", "INT64"}
equivalentSet2 := []interface{}{"FLOAT", "FLOAT64"}
equivalentSet3 := []interface{}{"BOOLEAN", "BOOL"}
eq0 := old == new
eq1 := valueIsInArray(old, equivalentSet1) && valueIsInArray(new, equivalentSet1)
eq2 := valueIsInArray(old, equivalentSet2) && valueIsInArray(new, equivalentSet2)
eq3 := valueIsInArray(old, equivalentSet3) && valueIsInArray(new, equivalentSet3)
eq0 := oldUpper == newUpper
eq1 := valueIsInArray(oldUpper, equivalentSet1) && valueIsInArray(newUpper, equivalentSet1)
eq2 := valueIsInArray(oldUpper, equivalentSet2) && valueIsInArray(newUpper, equivalentSet2)
eq3 := valueIsInArray(oldUpper, equivalentSet3) && valueIsInArray(newUpper, equivalentSet3)
eq := eq0 || eq1 || eq2 || eq3
return eq
}

func bigQueryTableModeEq(old, new interface{}) bool {
equivalentSet := []interface{}{nil, "NULLABLE"}
eq0 := old == new
eq1 := valueIsInArray(old, equivalentSet) && valueIsInArray(new, equivalentSet)
eq := eq0 || eq1
return eq
func bigQueryTableNormalizeMode(mode interface{}) string {
if mode == nil {
return "NULLABLE"
}
// Upper-case to get case-insensitive comparisons. https://github.com/hashicorp/terraform-provider-google/issues/9472
return strings.ToUpper(mode.(string))
}

func bigQueryTableModeIsForceNew(old, new interface{}) bool {
eq := bigQueryTableModeEq(old, new)
func bigQueryTableModeIsForceNew(old, new string) bool {
eq := old == new
reqToNull := old == "REQUIRED" && new == "NULLABLE"
return !eq && !reqToNull
}
Expand Down Expand Up @@ -250,11 +258,18 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}) (bool, error)
return false, nil
}
case "type":
if !bigQueryTableTypeEq(valOld, valNew) {
if valOld == nil || valNew == nil {
// This is invalid, so it shouldn't require a ForceNew
return true, nil
}
if !bigQueryTableTypeEq(valOld.(string), valNew.(string)) {
return false, nil
}
case "mode":
if bigQueryTableModeIsForceNew(valOld, valNew) {
if bigQueryTableModeIsForceNew(
bigQueryTableNormalizeMode(valOld),
bigQueryTableNormalizeMode(valNew),
) {
return false, nil
}
case "fields":
Expand Down
82 changes: 65 additions & 17 deletions google-beta/resource_bigquery_table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,28 +34,28 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
ExpectDiffSuppress: false,
},
"no change": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
ExpectDiffSuppress: true,
},
"remove key": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
New: "[{\"name\": \"someValue\", \"finalKey\" : {} }]",
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"finalKey\" : {} }]",
ExpectDiffSuppress: false,
},
"empty description -> default description (empty)": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\" }]",
ExpectDiffSuppress: true,
},
"empty description -> other description": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"somethingRandom\" }]",
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"somethingRandom\" }]",
ExpectDiffSuppress: false,
},
"mode NULLABLE -> other mode": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"mode\": \"NULLABLE\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"mode\": \"somethingRandom\" }]",
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"mode\": \"NULLABLE\" }]",
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"mode\": \"somethingRandom\" }]",
ExpectDiffSuppress: false,
},
"mode NULLABLE -> default mode (also NULLABLE)": {
Expand All @@ -74,6 +74,23 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
]`,
ExpectDiffSuppress: true,
},
"mode & type uppercase -> lowercase": {
Old: `[
{
"mode": "NULLABLE",
"name": "PageNo",
"type": "INTEGER"
}
]`,
New: `[
{
"mode": "nullable",
"name": "PageNo",
"type": "integer"
}
]`,
ExpectDiffSuppress: true,
},
"type INTEGER -> INT64": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"INTEGER\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"INT64\" }]",
Expand All @@ -89,18 +106,32 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"FLOAT64\" }]",
ExpectDiffSuppress: true,
},
"type FLOAT -> default": {
"type FLOAT -> other": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"FLOAT\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"somethingRandom\" }]",
ExpectDiffSuppress: false,
},
"type BOOLEAN -> BOOL": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOL\" }]",
ExpectDiffSuppress: true,
},
"type BOOLEAN -> default": {
"type BOOLEAN -> other": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"somethingRandom\" }]",
ExpectDiffSuppress: false,
},
// this is invalid but we need to make sure we don't cause a panic
// if users provide an invalid schema
"invalid - missing type for old": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
ExpectDiffSuppress: false,
},
// this is invalid but we need to make sure we don't cause a panic
// if users provide an invalid schema
"invalid - missing type for new": {
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
ExpectDiffSuppress: false,
},
Expand Down Expand Up @@ -341,6 +372,7 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
}

for tn, tc := range cases {
tn := tn
tc := tc
t.Run(tn, func(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -1037,6 +1069,22 @@ var testUnitBigQueryDataTableIsChangableTestCases = []testUnitBigQueryDataTableJ
jsonNew: "[{\"name\": \"someValue\", \"type\" : \"DATETIME\", \"mode\" : \"NULLABLE\", \"description\" : \"some new value\" }]",
changeable: false,
},
// this is invalid but we need to make sure we don't cause a panic
// if users provide an invalid schema
{
name: "typeChangeIgnoreNewMissingType",
jsonOld: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
jsonNew: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
changeable: true,
},
// this is invalid but we need to make sure we don't cause a panic
// if users provide an invalid schema
{
name: "typeChangeIgnoreOldMissingType",
jsonOld: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
jsonNew: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
changeable: true,
},
{
name: "typeModeReqToNull",
jsonOld: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"mode\" : \"REQUIRED\", \"description\" : \"someVal\" }]",
Expand All @@ -1050,10 +1098,10 @@ var testUnitBigQueryDataTableIsChangableTestCases = []testUnitBigQueryDataTableJ
changeable: false,
},
{
name: "typeModeOmission",
name: "modeToDefaultNullable",
jsonOld: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"mode\" : \"REQUIRED\", \"description\" : \"someVal\" }]",
jsonNew: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"description\" : \"some new value\" }]",
changeable: false,
changeable: true,
},
{
name: "orderOfArrayChangesAndDescriptionChanges",
Expand Down Expand Up @@ -1822,8 +1870,8 @@ resource "google_bigquery_table" "test" {
{
description = "Time snapshot was taken, in Epoch milliseconds. Same across all rows and all tables in the snapshot, and uniquely defines a particular snapshot."
name = "snapshot_timestamp"
mode = "NULLABLE"
type = "INTEGER"
mode = "nullable"
type = "integer"
},
{
description = "Timestamp of dataset creation"
Expand Down
3 changes: 1 addition & 2 deletions google-beta/resource_dataproc_cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"

"google.golang.org/api/googleapi"

dataproc "google.golang.org/api/dataproc/v1beta2"
"google.golang.org/api/googleapi"
)

func TestDataprocExtractInitTimeout(t *testing.T) {
Expand Down

0 comments on commit a25ffd0

Please sign in to comment.