Skip to content

Commit 1b818be

Browse files
modular-magicianmodular-magician
authored
Ignored case changes in bigquery table schema mode/type (#4943) (#3405)
* 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]>
1 parent 7387850 commit 1b818be

File tree

4 files changed

+101
-36
lines changed

4 files changed

+101
-36
lines changed

‎.changelog/4943.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
bigquery: Fixed permadiff due to lowercase mode/type in `google_bigquery_table.schema`
3+
```

‎google-beta/resource_bigquery_table.go‎

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"log"
99
"sort"
1010
"strconv"
11+
"strings"
1112

1213
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
1314
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -130,14 +131,17 @@ func bigQueryTableMapKeyOverride(key string, objectA, objectB map[string]interfa
130131
valB := objectB[key]
131132
switch key {
132133
case "mode":
133-
eq := bigQueryTableModeEq(valA, valB)
134+
eq := bigQueryTableNormalizeMode(valA) == bigQueryTableNormalizeMode(valB)
134135
return eq
135136
case "description":
136137
equivalentSet := []interface{}{nil, ""}
137138
eq := valueIsInArray(valA, equivalentSet) && valueIsInArray(valB, equivalentSet)
138139
return eq
139140
case "type":
140-
return bigQueryTableTypeEq(valA, valB)
141+
if valA == nil || valB == nil {
142+
return false
143+
}
144+
return bigQueryTableTypeEq(valA.(string), valB.(string))
141145
}
142146

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

170-
func bigQueryTableTypeEq(old, new interface{}) bool {
174+
func bigQueryTableTypeEq(old, new string) bool {
175+
// Do case-insensitive comparison. https://github.com/hashicorp/terraform-provider-google/issues/9472
176+
oldUpper := strings.ToUpper(old)
177+
newUpper := strings.ToUpper(new)
178+
171179
equivalentSet1 := []interface{}{"INTEGER", "INT64"}
172180
equivalentSet2 := []interface{}{"FLOAT", "FLOAT64"}
173181
equivalentSet3 := []interface{}{"BOOLEAN", "BOOL"}
174-
eq0 := old == new
175-
eq1 := valueIsInArray(old, equivalentSet1) && valueIsInArray(new, equivalentSet1)
176-
eq2 := valueIsInArray(old, equivalentSet2) && valueIsInArray(new, equivalentSet2)
177-
eq3 := valueIsInArray(old, equivalentSet3) && valueIsInArray(new, equivalentSet3)
182+
eq0 := oldUpper == newUpper
183+
eq1 := valueIsInArray(oldUpper, equivalentSet1) && valueIsInArray(newUpper, equivalentSet1)
184+
eq2 := valueIsInArray(oldUpper, equivalentSet2) && valueIsInArray(newUpper, equivalentSet2)
185+
eq3 := valueIsInArray(oldUpper, equivalentSet3) && valueIsInArray(newUpper, equivalentSet3)
178186
eq := eq0 || eq1 || eq2 || eq3
179187
return eq
180188
}
181189

182-
func bigQueryTableModeEq(old, new interface{}) bool {
183-
equivalentSet := []interface{}{nil, "NULLABLE"}
184-
eq0 := old == new
185-
eq1 := valueIsInArray(old, equivalentSet) && valueIsInArray(new, equivalentSet)
186-
eq := eq0 || eq1
187-
return eq
190+
func bigQueryTableNormalizeMode(mode interface{}) string {
191+
if mode == nil {
192+
return "NULLABLE"
193+
}
194+
// Upper-case to get case-insensitive comparisons. https://github.com/hashicorp/terraform-provider-google/issues/9472
195+
return strings.ToUpper(mode.(string))
188196
}
189197

190-
func bigQueryTableModeIsForceNew(old, new interface{}) bool {
191-
eq := bigQueryTableModeEq(old, new)
198+
func bigQueryTableModeIsForceNew(old, new string) bool {
199+
eq := old == new
192200
reqToNull := old == "REQUIRED" && new == "NULLABLE"
193201
return !eq && !reqToNull
194202
}
@@ -250,11 +258,18 @@ func resourceBigQueryTableSchemaIsChangeable(old, new interface{}) (bool, error)
250258
return false, nil
251259
}
252260
case "type":
253-
if !bigQueryTableTypeEq(valOld, valNew) {
261+
if valOld == nil || valNew == nil {
262+
// This is invalid, so it shouldn't require a ForceNew
263+
return true, nil
264+
}
265+
if !bigQueryTableTypeEq(valOld.(string), valNew.(string)) {
254266
return false, nil
255267
}
256268
case "mode":
257-
if bigQueryTableModeIsForceNew(valOld, valNew) {
269+
if bigQueryTableModeIsForceNew(
270+
bigQueryTableNormalizeMode(valOld),
271+
bigQueryTableNormalizeMode(valNew),
272+
) {
258273
return false, nil
259274
}
260275
case "fields":

‎google-beta/resource_bigquery_table_test.go‎

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,28 +34,28 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
3434
ExpectDiffSuppress: false,
3535
},
3636
"no change": {
37-
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
38-
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
37+
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
38+
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
3939
ExpectDiffSuppress: true,
4040
},
4141
"remove key": {
42-
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
43-
New: "[{\"name\": \"someValue\", \"finalKey\" : {} }]",
42+
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"finalKey\" : {} }]",
43+
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"finalKey\" : {} }]",
4444
ExpectDiffSuppress: false,
4545
},
4646
"empty description -> default description (empty)": {
47-
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
48-
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
47+
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
48+
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\" }]",
4949
ExpectDiffSuppress: true,
5050
},
5151
"empty description -> other description": {
52-
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
53-
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"description\": \"somethingRandom\" }]",
52+
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"\" }]",
53+
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"description\": \"somethingRandom\" }]",
5454
ExpectDiffSuppress: false,
5555
},
5656
"mode NULLABLE -> other mode": {
57-
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"mode\": \"NULLABLE\" }]",
58-
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"mode\": \"somethingRandom\" }]",
57+
Old: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"mode\": \"NULLABLE\" }]",
58+
New: "[{\"name\": \"someValue\", \"type\": \"INT64\", \"anotherKey\" : \"anotherValue\", \"mode\": \"somethingRandom\" }]",
5959
ExpectDiffSuppress: false,
6060
},
6161
"mode NULLABLE -> default mode (also NULLABLE)": {
@@ -74,6 +74,23 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
7474
]`,
7575
ExpectDiffSuppress: true,
7676
},
77+
"mode & type uppercase -> lowercase": {
78+
Old: `[
79+
{
80+
"mode": "NULLABLE",
81+
"name": "PageNo",
82+
"type": "INTEGER"
83+
}
84+
]`,
85+
New: `[
86+
{
87+
"mode": "nullable",
88+
"name": "PageNo",
89+
"type": "integer"
90+
}
91+
]`,
92+
ExpectDiffSuppress: true,
93+
},
7794
"type INTEGER -> INT64": {
7895
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"INTEGER\" }]",
7996
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"INT64\" }]",
@@ -89,18 +106,32 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
89106
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"FLOAT64\" }]",
90107
ExpectDiffSuppress: true,
91108
},
92-
"type FLOAT -> default": {
109+
"type FLOAT -> other": {
93110
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"FLOAT\" }]",
94-
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
111+
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"somethingRandom\" }]",
95112
ExpectDiffSuppress: false,
96113
},
97114
"type BOOLEAN -> BOOL": {
98115
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
99116
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOL\" }]",
100117
ExpectDiffSuppress: true,
101118
},
102-
"type BOOLEAN -> default": {
119+
"type BOOLEAN -> other": {
103120
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
121+
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"somethingRandom\" }]",
122+
ExpectDiffSuppress: false,
123+
},
124+
// this is invalid but we need to make sure we don't cause a panic
125+
// if users provide an invalid schema
126+
"invalid - missing type for old": {
127+
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
128+
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
129+
ExpectDiffSuppress: false,
130+
},
131+
// this is invalid but we need to make sure we don't cause a panic
132+
// if users provide an invalid schema
133+
"invalid - missing type for new": {
134+
Old: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
104135
New: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
105136
ExpectDiffSuppress: false,
106137
},
@@ -341,6 +372,7 @@ func TestBigQueryTableSchemaDiffSuppress(t *testing.T) {
341372
}
342373

343374
for tn, tc := range cases {
375+
tn := tn
344376
tc := tc
345377
t.Run(tn, func(t *testing.T) {
346378
t.Parallel()
@@ -1037,6 +1069,22 @@ var testUnitBigQueryDataTableIsChangableTestCases = []testUnitBigQueryDataTableJ
10371069
jsonNew: "[{\"name\": \"someValue\", \"type\" : \"DATETIME\", \"mode\" : \"NULLABLE\", \"description\" : \"some new value\" }]",
10381070
changeable: false,
10391071
},
1072+
// this is invalid but we need to make sure we don't cause a panic
1073+
// if users provide an invalid schema
1074+
{
1075+
name: "typeChangeIgnoreNewMissingType",
1076+
jsonOld: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
1077+
jsonNew: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
1078+
changeable: true,
1079+
},
1080+
// this is invalid but we need to make sure we don't cause a panic
1081+
// if users provide an invalid schema
1082+
{
1083+
name: "typeChangeIgnoreOldMissingType",
1084+
jsonOld: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\" }]",
1085+
jsonNew: "[{\"name\": \"someValue\", \"anotherKey\" : \"anotherValue\", \"type\": \"BOOLEAN\" }]",
1086+
changeable: true,
1087+
},
10401088
{
10411089
name: "typeModeReqToNull",
10421090
jsonOld: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"mode\" : \"REQUIRED\", \"description\" : \"someVal\" }]",
@@ -1050,10 +1098,10 @@ var testUnitBigQueryDataTableIsChangableTestCases = []testUnitBigQueryDataTableJ
10501098
changeable: false,
10511099
},
10521100
{
1053-
name: "typeModeOmission",
1101+
name: "modeToDefaultNullable",
10541102
jsonOld: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"mode\" : \"REQUIRED\", \"description\" : \"someVal\" }]",
10551103
jsonNew: "[{\"name\": \"someValue\", \"type\" : \"BOOLEAN\", \"description\" : \"some new value\" }]",
1056-
changeable: false,
1104+
changeable: true,
10571105
},
10581106
{
10591107
name: "orderOfArrayChangesAndDescriptionChanges",
@@ -1822,8 +1870,8 @@ resource "google_bigquery_table" "test" {
18221870
{
18231871
description = "Time snapshot was taken, in Epoch milliseconds. Same across all rows and all tables in the snapshot, and uniquely defines a particular snapshot."
18241872
name = "snapshot_timestamp"
1825-
mode = "NULLABLE"
1826-
type = "INTEGER"
1873+
mode = "nullable"
1874+
type = "integer"
18271875
},
18281876
{
18291877
description = "Timestamp of dataset creation"

‎google-beta/resource_dataproc_cluster_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ import (
1313
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
1414
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
1515

16-
"google.golang.org/api/googleapi"
17-
1816
dataproc "google.golang.org/api/dataproc/v1beta2"
17+
"google.golang.org/api/googleapi"
1918
)
2019

2120
func TestDataprocExtractInitTimeout(t *testing.T) {

0 commit comments

Comments
 (0)