From be886e06e508f44d3f8ada66a337fb3c6d17cb13 Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Sat, 27 Sep 2025 13:57:00 +0200 Subject: [PATCH 1/3] [Fix] Correctly handling tags update in `databricks_sql_endpoint` When updating tags of SQL Warehouse completely removing it, the reflect functionality incorrectly handled the case and tried to send an empty JSON object. This is fixed in current PR. Resolves #5051 --- NEXT_CHANGELOG.md | 1 + common/reflect_resource.go | 37 ++++++- common/reflect_resource_test.go | 178 ++++++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+), 5 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 2bd63916c6..85df3165a8 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -9,6 +9,7 @@ ### Bug Fixes * Allow update `managed_services_customer_managed_key_id` in `databricks_mws_workspaces` ([#5034](https://github.com/databricks/terraform-provider-databricks/pull/5034)) +* Correctly handling tags update in `databricks_sql_endpoint` ([#5060](https://github.com/databricks/terraform-provider-databricks/pull/5060)) ### Documentation diff --git a/common/reflect_resource.go b/common/reflect_resource.go index ca0d3b7d88..7466ba5320 100644 --- a/common/reflect_resource.go +++ b/common/reflect_resource.go @@ -688,6 +688,21 @@ func isValueNilOrEmpty(valueField *reflect.Value, fieldPath string) bool { return false } +// isStructEmpty checks if a struct has all zero values +func isStructEmpty(v reflect.Value) bool { + if v.Kind() != reflect.Struct { + return false + } + + for i := 0; i < v.NumField(); i++ { + field := v.Field(i) + if !field.IsZero() { + return false + } + } + return true +} + // StructToData reads result using schema onto resource data func StructToData(result any, s map[string]*schema.Schema, d *schema.ResourceData) error { aliases := getAliasesMapFromStruct(result) @@ -922,10 +937,9 @@ func readListFromData(path []string, d attributeGetter, return nil case reflect.Slice: k := valueField.Type().Elem().Kind() - newSlice := reflect.MakeSlice(valueField.Type(), len(rawList), len(rawList)) - valueField.Set(newSlice) + var validItems []reflect.Value + for i, elem := range rawList { - item := newSlice.Index(i) switch k { case reflect.Struct: // here we rely on Terraform SDK to perform validation, so we don't to it twice @@ -937,14 +951,27 @@ func readListFromData(path []string, d attributeGetter, if err != nil { return err } - item.Set(ve) + // Only add non-empty structs to avoid sending empty objects to the API + if !isStructEmpty(ve) { + validItems = append(validItems, ve) + } default: - err := setPrimitiveValueOfKind(fieldPath, k, item, elem) + // For non-struct types, create a temporary item to check if it's valid + tempItem := reflect.New(valueField.Type().Elem()).Elem() + err := setPrimitiveValueOfKind(fieldPath, k, tempItem, elem) if err != nil { return err } + validItems = append(validItems, tempItem) } } + + // Create slice with only valid items + newSlice := reflect.MakeSlice(valueField.Type(), len(validItems), len(validItems)) + for i, item := range validItems { + newSlice.Index(i).Set(item) + } + valueField.Set(newSlice) default: return fmt.Errorf("%s[%v] unknown collection field", fieldPath, rawList) } diff --git a/common/reflect_resource_test.go b/common/reflect_resource_test.go index 593bc8c735..41edb9b2d1 100644 --- a/common/reflect_resource_test.go +++ b/common/reflect_resource_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "strconv" "testing" "github.com/databricks/databricks-sdk-go/service/sql" @@ -752,6 +753,183 @@ func TestReadListFromData(t *testing.T) { assert.EqualError(t, err, "[[1]] unknown collection field") } +func TestIsStructEmpty(t *testing.T) { + // Test with empty struct + type EmptyStruct struct{} + emptyStruct := reflect.ValueOf(EmptyStruct{}) + assert.True(t, isStructEmpty(emptyStruct)) + + // Test with struct containing zero values + type ZeroStruct struct { + StringField string + IntField int + BoolField bool + } + zeroStruct := reflect.ValueOf(ZeroStruct{}) + assert.True(t, isStructEmpty(zeroStruct)) + + // Test with struct containing non-zero values + type NonZeroStruct struct { + StringField string + IntField int + BoolField bool + } + nonZeroStruct := reflect.ValueOf(NonZeroStruct{ + StringField: "test", + IntField: 42, + BoolField: true, + }) + assert.False(t, isStructEmpty(nonZeroStruct)) + + // Test with struct containing some zero and some non-zero values + type MixedStruct struct { + StringField string + IntField int + BoolField bool + } + mixedStruct := reflect.ValueOf(MixedStruct{ + StringField: "test", + IntField: 0, // zero value + BoolField: false, // zero value + }) + assert.False(t, isStructEmpty(mixedStruct)) + + // Test with non-struct value + nonStruct := reflect.ValueOf("string") + assert.False(t, isStructEmpty(nonStruct)) +} + +func TestReadListFromDataWithEmptyStructs(t *testing.T) { + // Define a test struct similar to EndpointTagPair + type TestTag struct { + Key string `json:"key,omitempty"` + Value string `json:"value,omitempty"` + } + + // Create schema for the test + schema := &schema.Schema{ + Type: schema.TypeSet, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "key": { + Type: schema.TypeString, + Optional: true, + }, + "value": { + Type: schema.TypeString, + Optional: true, + }, + }, + }, + } + + // Test case 1: Empty list should result in empty slice + t.Run("EmptyList", func(t *testing.T) { + var result []TestTag + valueField := reflect.ValueOf(&result).Elem() + + err := readListFromData([]string{"tags"}, &mockAttributeGetter{}, []any{}, &valueField, schema, map[string]map[string]string{}, strconv.Itoa) + assert.NoError(t, err) + assert.Len(t, result, 0) + }) + + // Test case 2: List with empty structs should be filtered out + t.Run("EmptyStructsFiltered", func(t *testing.T) { + var result []TestTag + valueField := reflect.ValueOf(&result).Elem() + + // Create mock data with empty structs + rawList := []any{ + map[string]any{"key": "", "value": ""}, // empty struct + map[string]any{"key": "", "value": ""}, // another empty struct + } + + mockGetter := &mockAttributeGetter{ + data: map[string]any{ + "tags.0.key": "", + "tags.0.value": "", + "tags.1.key": "", + "tags.1.value": "", + }, + } + + err := readListFromData([]string{"tags"}, mockGetter, rawList, &valueField, schema, map[string]map[string]string{}, strconv.Itoa) + assert.NoError(t, err) + assert.Len(t, result, 0) // Should be empty because all structs are empty + }) + + // Test case 3: List with mixed empty and non-empty structs + t.Run("MixedStructs", func(t *testing.T) { + var result []TestTag + valueField := reflect.ValueOf(&result).Elem() + + // Create mock data with one empty and one non-empty struct + rawList := []any{ + map[string]any{"key": "", "value": ""}, // empty struct + map[string]any{"key": "test", "value": "value"}, // non-empty struct + } + + mockGetter := &mockAttributeGetter{ + data: map[string]any{ + "tags.0.key": "", + "tags.0.value": "", + "tags.1.key": "test", + "tags.1.value": "value", + }, + } + + err := readListFromData([]string{"tags"}, mockGetter, rawList, &valueField, schema, map[string]map[string]string{}, strconv.Itoa) + assert.NoError(t, err) + assert.Len(t, result, 1) // Should contain only the non-empty struct + assert.Equal(t, "test", result[0].Key) + assert.Equal(t, "value", result[0].Value) + }) + + // Test case 4: List with only non-empty structs + t.Run("NonEmptyStructs", func(t *testing.T) { + var result []TestTag + valueField := reflect.ValueOf(&result).Elem() + + // Create mock data with non-empty structs + rawList := []any{ + map[string]any{"key": "key1", "value": "value1"}, + map[string]any{"key": "key2", "value": "value2"}, + } + + mockGetter := &mockAttributeGetter{ + data: map[string]any{ + "tags.0.key": "key1", + "tags.0.value": "value1", + "tags.1.key": "key2", + "tags.1.value": "value2", + }, + } + + err := readListFromData([]string{"tags"}, mockGetter, rawList, &valueField, schema, map[string]map[string]string{}, strconv.Itoa) + assert.NoError(t, err) + assert.Len(t, result, 2) // Should contain both structs + assert.Equal(t, "key1", result[0].Key) + assert.Equal(t, "value1", result[0].Value) + assert.Equal(t, "key2", result[1].Key) + assert.Equal(t, "value2", result[1].Value) + }) +} + +// mockAttributeGetter implements the attributeGetter interface for testing +type mockAttributeGetter struct { + data map[string]any +} + +func (m *mockAttributeGetter) GetOk(key string) (any, bool) { + value, ok := m.data[key] + return value, ok +} + +func (m *mockAttributeGetter) GetOkExists(key string) (any, bool) { + value, ok := m.data[key] + return value, ok +} + func TestReadReflectValueFromDataCornerCases(t *testing.T) { type Nonsense struct { New float64 `json:"new,omitempty"` From 3daabd79ba216430b54514f9ac7be50de733b43a Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Mon, 29 Sep 2025 09:05:41 +0200 Subject: [PATCH 2/3] Address Copilot review comments - Fix misleading comment about validation checking for non-struct types - Replace direct strconv.Itoa function parameter with proper lambda function - Ensure code clarity and maintainability --- common/reflect_resource.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/common/reflect_resource.go b/common/reflect_resource.go index 7466ba5320..af6d1da7f1 100644 --- a/common/reflect_resource.go +++ b/common/reflect_resource.go @@ -853,7 +853,9 @@ func readReflectValueFromData(path []string, d attributeGetter, case schema.TypeList: // here we rely on Terraform SDK to perform validation, so we don't to it twice rawList := raw.([]any) - return readListFromData(path, d, rawList, valueField, fieldSchema, aliases, strconv.Itoa) + return readListFromData(path, d, rawList, valueField, fieldSchema, aliases, func(i int) string { + return strconv.Itoa(i) + }) default: return fmt.Errorf("%s[%v] unsupported field type", fieldPath, raw) } @@ -956,7 +958,7 @@ func readListFromData(path []string, d attributeGetter, validItems = append(validItems, ve) } default: - // For non-struct types, create a temporary item to check if it's valid + // For non-struct types, add all primitive values tempItem := reflect.New(valueField.Type().Elem()).Elem() err := setPrimitiveValueOfKind(fieldPath, k, tempItem, elem) if err != nil { From 9b0a0f8bee9e4cc4e169650dfb25ee5c4b3f6dfd Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Mon, 29 Sep 2025 09:16:14 +0200 Subject: [PATCH 3/3] Update common/reflect_resource.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- common/reflect_resource.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/reflect_resource.go b/common/reflect_resource.go index af6d1da7f1..7df4dcbf1a 100644 --- a/common/reflect_resource.go +++ b/common/reflect_resource.go @@ -853,9 +853,7 @@ func readReflectValueFromData(path []string, d attributeGetter, case schema.TypeList: // here we rely on Terraform SDK to perform validation, so we don't to it twice rawList := raw.([]any) - return readListFromData(path, d, rawList, valueField, fieldSchema, aliases, func(i int) string { - return strconv.Itoa(i) - }) + return readListFromData(path, d, rawList, valueField, fieldSchema, aliases, strconv.Itoa) default: return fmt.Errorf("%s[%v] unsupported field type", fieldPath, raw) }