diff --git a/catalog/resource_sql_table.go b/catalog/resource_sql_table.go index e770c37c95..922d2d0cd2 100644 --- a/catalog/resource_sql_table.go +++ b/catalog/resource_sql_table.go @@ -41,16 +41,44 @@ type SqlTableInfo struct { StorageCredentialName string `json:"storage_credential_name,omitempty" tf:"force_new"` ViewDefinition string `json:"view_definition,omitempty"` Comment string `json:"comment,omitempty"` - Properties map[string]string `json:"properties,omitempty" tf:"computed"` + Properties map[string]string `json:"properties,omitempty"` Options map[string]string `json:"options,omitempty" tf:"force_new"` - ClusterID string `json:"cluster_id,omitempty" tf:"computed"` - WarehouseID string `json:"warehouse_id,omitempty"` - Owner string `json:"owner,omitempty" tf:"computed"` + // EffectiveProperties includes both properties and options. Options are prefixed with `option.`. + EffectiveProperties map[string]string `json:"effective_properties" tf:"computed"` + ClusterID string `json:"cluster_id,omitempty" tf:"computed"` + WarehouseID string `json:"warehouse_id,omitempty"` + Owner string `json:"owner,omitempty" tf:"computed"` exec common.CommandExecutor sqlExec sql.StatementExecutionInterface } +func (ti SqlTableInfo) CustomizeSchema(s *common.CustomizableSchema) *common.CustomizableSchema { + + caseInsensitiveFields := []string{"name", "catalog_name", "schema_name"} + for _, field := range caseInsensitiveFields { + s.SchemaPath(field).SetCustomSuppressDiff(common.EqualFoldDiffSuppress) + } + s.SchemaPath("data_source_format").SetCustomSuppressDiff(func(k, old, new string, d *schema.ResourceData) bool { + if new == "" { + return true + } + return strings.EqualFold(strings.ToLower(old), strings.ToLower(new)) + }) + s.SchemaPath("storage_location").SetCustomSuppressDiff(ucDirectoryPathSlashAndEmptySuppressDiff) + s.SchemaPath("view_definition").SetCustomSuppressDiff(common.SuppressDiffWhitespaceChange) + + s.SchemaPath("cluster_id").SetConflictsWith([]string{"warehouse_id"}) + s.SchemaPath("warehouse_id").SetConflictsWith([]string{"cluster_id"}) + + s.SchemaPath("partitions").SetConflictsWith([]string{"cluster_keys"}) + s.SchemaPath("cluster_keys").SetConflictsWith([]string{"partitions"}) + s.SchemaPath("column", "type").SetCustomSuppressDiff(func(k, old, new string, d *schema.ResourceData) bool { + return getColumnType(old) == getColumnType(new) + }) + return s +} + type SqlTablesAPI struct { client *common.DatabricksClient context context.Context @@ -62,6 +90,9 @@ func NewSqlTablesAPI(ctx context.Context, m any) SqlTablesAPI { func (a SqlTablesAPI) getTable(name string) (ti SqlTableInfo, err error) { err = a.client.Get(a.context, "/unity-catalog/tables/"+name, nil, &ti) + // Copy returned properties & options to read-only attributes + ti.EffectiveProperties = ti.Properties + ti.Properties = nil return } @@ -77,54 +108,6 @@ func parseComment(s string) string { return strings.ReplaceAll(strings.ReplaceAll(s, `\'`, `'`), `'`, `\'`) } -// These properties are added automatically -// If we do not customize the diff using these then terraform will constantly try to remove them -// `properties` is essentially a "partially" computed field -// This needs to be replaced with something a bit more robust in the future -func sqlTableIsManagedProperty(key string) bool { - managedProps := map[string]bool{ - // Property set if the table uses `cluster_keys`. - "clusteringColumns": true, - - "delta.lastCommitTimestamp": true, - "delta.lastUpdateVersion": true, - "delta.minReaderVersion": true, - "delta.minWriterVersion": true, - "delta.columnMapping.maxColumnId": true, - "delta.enableDeletionVectors": true, - "delta.enableRowTracking": true, - "delta.feature.clustering": true, - "delta.feature.changeDataFeed": true, - "delta.feature.deletionVectors": true, - "delta.feature.domainMetadata": true, - "delta.feature.liquid": true, - "delta.feature.rowTracking": true, - "delta.feature.v2Checkpoint": true, - "delta.feature.timestampNtz": true, - "delta.liquid.clusteringColumns": true, - "delta.rowTracking.materializedRowCommitVersionColumnName": true, - "delta.rowTracking.materializedRowIdColumnName": true, - "delta.checkpoint.writeStatsAsJson": true, - "delta.checkpoint.writeStatsAsStruct": true, - "delta.checkpointPolicy": true, - "view.catalogAndNamespace.numParts": true, - "view.catalogAndNamespace.part.0": true, - "view.catalogAndNamespace.part.1": true, - "view.query.out.col.0": true, - "view.query.out.numCols": true, - "view.referredTempFunctionsNames": true, - "view.referredTempViewNames": true, - "view.sqlConfig.spark.sql.hive.convertCTAS": true, - "view.sqlConfig.spark.sql.legacy.createHiveTableByDefault": true, - "view.sqlConfig.spark.sql.parquet.compression.codec": true, - "view.sqlConfig.spark.sql.session.timeZone": true, - "view.sqlConfig.spark.sql.sources.commitProtocolClass": true, - "view.sqlConfig.spark.sql.sources.default": true, - "view.sqlConfig.spark.sql.streaming.stopTimeout": true, - } - return managedProps[key] -} - func (ti *SqlTableInfo) initCluster(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) (err error) { defaultClusterName := "terraform-sql-table" clustersAPI := clusters.NewClustersAPI(ctx, c) @@ -212,9 +195,7 @@ func (ti *SqlTableInfo) serializeColumnInfos() string { func (ti *SqlTableInfo) serializeProperties() string { propsMap := make([]string, 0, len(ti.Properties)) for key, value := range ti.Properties { - if !sqlTableIsManagedProperty(key) { - propsMap = append(propsMap, fmt.Sprintf("'%s'='%s'", key, value)) - } + propsMap = append(propsMap, fmt.Sprintf("'%s'='%s'", key, value)) } return strings.Join(propsMap[:], ", ") // 'foo'='bar', 'this'='that' } @@ -222,9 +203,7 @@ func (ti *SqlTableInfo) serializeProperties() string { func (ti *SqlTableInfo) serializeOptions() string { optionsMap := make([]string, 0, len(ti.Options)) for key, value := range ti.Options { - if !sqlTableIsManagedProperty(key) { - optionsMap = append(optionsMap, fmt.Sprintf("'%s'='%s'", key, value)) - } + optionsMap = append(optionsMap, fmt.Sprintf("'%s'='%s'", key, value)) } return strings.Join(optionsMap[:], ", ") // 'foo'='bar', 'this'='that' } @@ -549,31 +528,7 @@ func assertNoColumnMembershipAndFieldValueUpdate(oldCols []interface{}, newColum } func ResourceSqlTable() common.Resource { - tableSchema := common.StructToSchema(SqlTableInfo{}, - func(s map[string]*schema.Schema) map[string]*schema.Schema { - caseInsensitiveFields := []string{"name", "catalog_name", "schema_name"} - for _, field := range caseInsensitiveFields { - s[field].DiffSuppressFunc = common.EqualFoldDiffSuppress - } - s["data_source_format"].DiffSuppressFunc = func(k, old, new string, d *schema.ResourceData) bool { - if new == "" { - return true - } - return strings.EqualFold(strings.ToLower(old), strings.ToLower(new)) - } - s["storage_location"].DiffSuppressFunc = ucDirectoryPathSlashAndEmptySuppressDiff - s["view_definition"].DiffSuppressFunc = common.SuppressDiffWhitespaceChange - - s["cluster_id"].ConflictsWith = []string{"warehouse_id"} - s["warehouse_id"].ConflictsWith = []string{"cluster_id"} - - s["partitions"].ConflictsWith = []string{"cluster_keys"} - s["cluster_keys"].ConflictsWith = []string{"partitions"} - common.MustSchemaPath(s, "column", "type").DiffSuppressFunc = func(k, old, new string, d *schema.ResourceData) bool { - return getColumnType(old) == getColumnType(new) - } - return s - }) + tableSchema := common.StructToSchema(SqlTableInfo{}, nil) return common.Resource{ Schema: tableSchema, CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error { @@ -585,25 +540,30 @@ func ResourceSqlTable() common.Resource { return err } } - if d.HasChange("properties") { - old, new := d.GetChange("properties") - oldProps := old.(map[string]any) - newProps := new.(map[string]any) - old, _ = d.GetChange("options") - options := old.(map[string]any) - for key := range oldProps { - if _, ok := newProps[key]; !ok { - //options also gets exposed as properties - if _, ok := options[key]; ok { - newProps[key] = oldProps[key] - } - //some options are exposed as option.[...] properties - if sqlTableIsManagedProperty(key) || strings.HasPrefix(key, "option.") { - newProps[key] = oldProps[key] - } - } + // Compute the new effective property/options. + // If the user changed a property or option, the resource will already be considered changed. + // If the user specified a property but the value of that property has changed, that will appear + // as a change in the effective property/option. To cause a diff to be detected, we need to + // reset the effective property/option to the requested value. + userSpecifiedProperties := d.Get("properties").(map[string]interface{}) + userSpecifiedOptions := d.Get("options").(map[string]interface{}) + effectiveProperties := d.Get("effective_properties").(map[string]interface{}) + diff := make(map[string]interface{}) + for k, userSpecifiedValue := range userSpecifiedProperties { + if effectiveValue, ok := effectiveProperties[k]; !ok || effectiveValue != userSpecifiedValue { + diff[k] = userSpecifiedValue + } + } + for k, userSpecifiedValue := range userSpecifiedOptions { + if effectiveValue, ok := effectiveProperties["option."+k]; !ok || effectiveValue != userSpecifiedValue { + diff["option."+k] = userSpecifiedValue + } + } + if len(diff) > 0 { + for k, v := range diff { + effectiveProperties[k] = v } - d.SetNew("properties", newProps) + d.SetNew("effective_properties", effectiveProperties) } // No support yet for changing the COMMENT on a VIEW // Once added this can be removed diff --git a/catalog/resource_sql_table_test.go b/catalog/resource_sql_table_test.go index f1d51d47b9..795a1d3f31 100644 --- a/catalog/resource_sql_table_test.go +++ b/catalog/resource_sql_table_test.go @@ -1,6 +1,7 @@ package catalog import ( + "context" "fmt" "strconv" "testing" @@ -11,6 +12,7 @@ import ( "github.com/databricks/terraform-provider-databricks/clusters" "github.com/databricks/terraform-provider-databricks/common" "github.com/databricks/terraform-provider-databricks/qa" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/stretchr/testify/assert" "golang.org/x/exp/slices" ) @@ -1332,6 +1334,196 @@ func TestResourceSqlTableCreateTable_ExistingSQLWarehouse(t *testing.T) { assert.NoError(t, err) } +func TestResourceSqlTableCreateTable_OnlyManagedProperties(t *testing.T) { + qa.ResourceFixture{ + CommandMock: func(commandStr string) common.CommandResults { + return common.CommandResults{ + ResultType: "", + Data: nil, + } + }, + HCL: ` + name = "bar" + catalog_name = "main" + schema_name = "foo" + table_type = "MANAGED" + data_source_format = "DELTA" + warehouse_id = "existingwarehouse" + + column { + name = "id" + type = "int" + } + properties = { + "delta.enableDeletionVectors" = "false" + } + `, + Fixtures: []qa.HTTPFixture{ + { + Method: "POST", + Resource: "/api/2.0/sql/statements/", + ExpectedRequest: sql.ExecuteStatementRequest{ + Statement: "CREATE TABLE `main`.`foo`.`bar` (`id` int)\nUSING DELTA\nTBLPROPERTIES ('delta.enableDeletionVectors'='false');", + WaitTimeout: "50s", + WarehouseId: "existingwarehouse", + OnWaitTimeout: sql.ExecuteStatementRequestOnWaitTimeoutCancel, + }, + Response: sql.StatementResponse{ + StatementId: "statement1", + Status: &sql.StatementStatus{ + State: "SUCCEEDED", + }, + }, + }, + { + Method: "GET", + Resource: "/api/2.1/unity-catalog/tables/main.foo.bar", + Response: SqlTableInfo{ + Name: "bar", + CatalogName: "main", + SchemaName: "foo", + TableType: "EXTERNAL", + DataSourceFormat: "DELTA", + StorageLocation: "s3://ext-main/foo/bar1", + StorageCredentialName: "somecred", + Comment: "terraform managed", + Properties: map[string]string{ + "one": "two", + "three": "four", + "delta.enableDeletionVectors": "false", + }, + }, + }, + }, + Create: true, + Resource: ResourceSqlTable(), + }.ApplyNoError(t) +} + +func TestResourceSqlTable_Diff_ExistingResource(t *testing.T) { + testCases := []struct { + name string + hcl string + instanceState map[string]string + expectedDiff map[string]*terraform.ResourceAttrDiff + }{ + { + "existing resource with no properties or options", + "", + map[string]string{ + "effective_properties.%": "0", + }, + nil, + }, + { + "existing resource with computed properties and options", + "", + map[string]string{ + "effective_properties.%": "2", + "effective_properties.computedprop": "computedvalue", + "effective_properties.option.computedprop": "computedvalue", + }, + nil, + }, + { + "existing resource with property and option", + `properties = { + "myprop" = "myval" + } + options = { + "myopt" = "myval" + }`, + map[string]string{ + "properties.%": "1", + "properties.myprop": "myval", + "options.%": "1", + "options.myopt": "myval", + "effective_properties.%": "2", + "effective_properties.myprop": "myval", + "effective_properties.option.myopt": "myval", + }, + nil, + }, + { + "existing resource with property and option and computed property and computed option", + `properties = { + "myprop" = "myval" + } + options = { + "myopt" = "myval" + }`, + map[string]string{ + "properties.%": "1", + "properties.myprop": "myval", + "options.%": "1", + "options.myopt": "myval", + "effective_properties.%": "4", + "effective_properties.myprop": "myval", + "effective_properties.computedprop": "computedval", + "effective_properties.option.myopt": "myval", + "effective_properties.option.computedopt": "computedval", + }, + nil, + }, + { + "existing resource with conflicting property and option", + `properties = { + "myprop" = "myval" + } + options = { + "myopt" = "myval" + } + `, + map[string]string{ + "properties.%": "1", + "properties.myprop": "myval", + "options.%": "1", + "options.myopt": "myval", + "effective_properties.%": "2", + "effective_properties.myprop": "otherval", + "effective_properties.option.myopt": "otherval", + }, + map[string]*terraform.ResourceAttrDiff{ + "effective_properties.myprop": {New: "myval", Old: "otherval"}, + "effective_properties.option.myopt": {New: "myval", Old: "otherval"}, + }, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + instanceState := testCase.instanceState + if instanceState == nil { + instanceState = make(map[string]string) + } + instanceState["catalog_name"] = "main" + instanceState["schema_name"] = "default" + instanceState["table_type"] = "MANAGED" + instanceState["name"] = "mytable" + instanceState["cluster_id"] = "test-1234" + instanceState["column.#"] = "0" + instanceState["owner"] = "testuser" + + expectedDiff := testCase.expectedDiff + if expectedDiff == nil { + expectedDiff = make(map[string]*terraform.ResourceAttrDiff) + } + qa.ResourceFixture{ + HCL: ` + catalog_name = "main" + schema_name = "default" + table_type = "MANAGED" + name = "mytable" + cluster_id = "test-1234" + owner = "testuser" + ` + testCase.hcl, + InstanceState: instanceState, + ExpectedDiff: expectedDiff, + Resource: ResourceSqlTable(), + }.ApplyNoError(t) + }) + } +} + var baseClusterFixture = []qa.HTTPFixture{ { Method: "GET", @@ -1464,3 +1656,57 @@ func TestParseComment_unescapedquote(t *testing.T) { prsd := parseComment(cmt) assert.Equal(t, `Comment with\' unescaped quotes \'`, prsd) } + +func TestSqlTablesAPI_getTable_OptionsAndParametersProcessedCorrectly(t *testing.T) { + testCases := []struct { + name string + apiResponse SqlTableInfo + expectedEffectiveProperties map[string]string + }{ + { + name: "no properties or options", + apiResponse: SqlTableInfo{}, + expectedEffectiveProperties: nil, + }, + { + name: "empty properties and options", + apiResponse: SqlTableInfo{ + Properties: map[string]string{}, + }, + expectedEffectiveProperties: nil, + }, + { + name: "properties and options", + apiResponse: SqlTableInfo{ + // Options appear in Properties with a "option." prefix. + Properties: map[string]string{ + "myprop": "myval", + "option.myopt": "myval", + }, + }, + expectedEffectiveProperties: map[string]string{ + "myprop": "myval", + "option.myopt": "myval", + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + client, _, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.1/unity-catalog/tables/main.foo.bar", + Response: testCase.apiResponse, + }, + }) + assert.NoError(t, err) + api := NewSqlTablesAPI(context.Background(), client) + actual, err := api.getTable("main.foo.bar") + assert.NoError(t, err) + assert.Equal(t, testCase.expectedEffectiveProperties, actual.EffectiveProperties) + assert.Nil(t, actual.Properties) + assert.Nil(t, actual.Options) + }) + } +} diff --git a/docs/resources/workspace_binding.md b/docs/resources/workspace_binding.md index 3aa8b1c581..94f33912c1 100644 --- a/docs/resources/workspace_binding.md +++ b/docs/resources/workspace_binding.md @@ -35,7 +35,7 @@ The following arguments are required: * `workspace_id` - ID of the workspace. Change forces creation of a new resource. * `securable_name` - Name of securable. Change forces creation of a new resource. -* `securable_type` - Type of securable. Can be `catalog`, `external_location` or `storage_credential`. Default to `catalog`. Change forces creation of a new resource. +* `securable_type` - Type of securable. Can be `catalog`, `external-location` or `storage-credential`. Default to `catalog`. Change forces creation of a new resource. * `binding_type` - (Optional) Binding mode. Default to `BINDING_TYPE_READ_WRITE`. Possible values are `BINDING_TYPE_READ_ONLY`, `BINDING_TYPE_READ_WRITE`. ## Import @@ -43,5 +43,5 @@ The following arguments are required: This resource can be imported by using combination of workspace ID, securable type and name: ```sh -terraform import databricks_catalog_workspace_binding.this "||" +terraform import databricks_workspace_binding.this "||" ``` diff --git a/qa/testing.go b/qa/testing.go index aa39655326..7194975d06 100644 --- a/qa/testing.go +++ b/qa/testing.go @@ -310,6 +310,12 @@ func (f ResourceFixture) Apply(t *testing.T) (*schema.ResourceData, error) { ctx := context.Background() diff, err := resource.Diff(ctx, is, resourceConfig, client) if f.ExpectedDiff != nil { + // Users can specify that there is no diff by setting an empty but initialized map. + // resource.Diff returns nil if there is no diff. + if len(f.ExpectedDiff) == 0 { + assert.Nil(t, diff) + return nil, err + } assert.Equal(t, f.ExpectedDiff, diff.Attributes) return nil, err }