From c22c035fff024fb8c427e2c109245d6fc811eb93 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 16 Sep 2024 13:40:11 +0200 Subject: [PATCH 1/6] Fix views --- .../row_access_policy_resource_ext.go | 2 +- .../model/row_access_policy_model_ext.go | 2 +- .../row_access_policies_acceptance_test.go | 12 ++- pkg/resources/diff_suppressions.go | 7 ++ pkg/resources/row_access_policy.go | 12 +-- .../row_access_policy_acceptance_test.go | 44 ++++++----- .../testdata/TestAcc_View/basic/test.tf | 2 +- .../testdata/TestAcc_View/basic/variables.tf | 2 +- .../TestAcc_View/basic_copy_grants/test.tf | 2 +- .../basic_copy_grants/variables.tf | 2 +- .../TestAcc_View/basic_is_recursive/test.tf | 2 +- .../basic_is_recursive/variables.tf | 2 +- .../TestAcc_View/basic_update/test.tf | 2 +- .../TestAcc_View/basic_update/variables.tf | 2 +- pkg/resources/view.go | 3 +- pkg/resources/view_acceptance_test.go | 74 +++++++++++++++---- pkg/schemas/row_access_policy.go | 23 +++++- pkg/sdk/row_access_policies_gen.go | 28 +------ pkg/sdk/row_access_policies_gen_test.go | 30 +------- pkg/sdk/row_access_policies_impl_gen.go | 29 +++++++- ...ow_access_policies_gen_integration_test.go | 25 ++++--- 21 files changed, 182 insertions(+), 125 deletions(-) diff --git a/pkg/acceptance/bettertestspoc/assert/resourceassert/row_access_policy_resource_ext.go b/pkg/acceptance/bettertestspoc/assert/resourceassert/row_access_policy_resource_ext.go index 725e0012ea..ec54107493 100644 --- a/pkg/acceptance/bettertestspoc/assert/resourceassert/row_access_policy_resource_ext.go +++ b/pkg/acceptance/bettertestspoc/assert/resourceassert/row_access_policy_resource_ext.go @@ -12,7 +12,7 @@ func (r *RowAccessPolicyResourceAssert) HasArguments(args []sdk.RowAccessPolicyA r.AddAssertion(assert.ValueSet("argument.#", strconv.FormatInt(int64(len(args)), 10))) for i, v := range args { r.AddAssertion(assert.ValueSet(fmt.Sprintf("argument.%d.name", i), v.Name)) - r.AddAssertion(assert.ValueSet(fmt.Sprintf("argument.%d.type", i), v.Type)) + r.AddAssertion(assert.ValueSet(fmt.Sprintf("argument.%d.type", i), string(v.Type))) } return r } diff --git a/pkg/acceptance/bettertestspoc/config/model/row_access_policy_model_ext.go b/pkg/acceptance/bettertestspoc/config/model/row_access_policy_model_ext.go index fc11800ba3..848c07ee5c 100644 --- a/pkg/acceptance/bettertestspoc/config/model/row_access_policy_model_ext.go +++ b/pkg/acceptance/bettertestspoc/config/model/row_access_policy_model_ext.go @@ -11,7 +11,7 @@ func (r *RowAccessPolicyModel) WithArgument(argument []sdk.RowAccessPolicyArgume for i, v := range argument { maps[i] = config.MapVariable(map[string]config.Variable{ "name": config.StringVariable(v.Name), - "type": config.StringVariable(v.Type), + "type": config.StringVariable(string(v.Type)), }) } r.Argument = tfconfig.SetVariable(maps...) diff --git a/pkg/datasources/row_access_policies_acceptance_test.go b/pkg/datasources/row_access_policies_acceptance_test.go index eff2a614f9..66352d1a71 100644 --- a/pkg/datasources/row_access_policies_acceptance_test.go +++ b/pkg/datasources/row_access_policies_acceptance_test.go @@ -27,11 +27,11 @@ func TestAcc_RowAccessPolicies(t *testing.T) { policyModel := model.RowAccessPolicy("test", []sdk.RowAccessPolicyArgument{ { Name: "a", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "b", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }, body, id.DatabaseName(), id.Name(), id.SchemaName()).WithComment("foo") @@ -64,7 +64,11 @@ func TestAcc_RowAccessPolicies(t *testing.T) { assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.body", "case when current_role() in ('ANALYST') then true else false end")), assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.name", id.Name())), assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.return_type", "BOOLEAN")), - assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature", "(a VARCHAR, b VARCHAR)")), + assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature.#", "2")), + assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature.0.name", "a")), + assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature.0.type", string(sdk.DataTypeVARCHAR))), + assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature.1.name", "b")), + assert.Check(resource.TestCheckResourceAttr(dsName, "row_access_policies.0.describe_output.0.signature.1.type", string(sdk.DataTypeVARCHAR))), ), }, { @@ -107,7 +111,7 @@ func TestAcc_RowAccessPolicies_Filtering(t *testing.T) { "arguments": config.SetVariable( config.MapVariable(map[string]config.Variable{ "name": config.StringVariable("a"), - "type": config.StringVariable("VARCHAR"), + "type": config.StringVariable(string(sdk.DataTypeVARCHAR)), }), ), "body": config.StringVariable("case when current_role() in ('ANALYST') then true else false end"), diff --git a/pkg/resources/diff_suppressions.go b/pkg/resources/diff_suppressions.go index 7f60139b5e..aace546ff5 100644 --- a/pkg/resources/diff_suppressions.go +++ b/pkg/resources/diff_suppressions.go @@ -221,3 +221,10 @@ func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceD } return slices.Equal(oldId, newId) } + +func ignoreEmpty(k, oldValue, newValue string, _ *schema.ResourceData) bool { + if strings.HasSuffix(k, ".#") && newValue == "0" { + return true + } + return newValue == "" +} diff --git a/pkg/resources/row_access_policy.go b/pkg/resources/row_access_policy.go index afe4d7ba12..3c07409f32 100644 --- a/pkg/resources/row_access_policy.go +++ b/pkg/resources/row_access_policy.go @@ -156,11 +156,7 @@ func ImportRowAccessPolicy(ctx context.Context, d *schema.ResourceData, meta any if err := d.Set("body", policyDescription.Body); err != nil { return nil, err } - args, err := policyDescription.Arguments() - if err != nil { - return nil, err - } - if err := d.Set("argument", schemas.RowAccessPolicyArgumentsToSchema(args)); err != nil { + if err := d.Set("argument", schemas.RowAccessPolicyArgumentsToSchema(policyDescription.Signature)); err != nil { return nil, err } return []*schema.ResourceData{d}, nil @@ -233,11 +229,7 @@ func ReadRowAccessPolicy(ctx context.Context, d *schema.ResourceData, meta any) if err := d.Set("body", rowAccessPolicyDescription.Body); err != nil { return diag.FromErr(err) } - args, err := rowAccessPolicyDescription.Arguments() - if err != nil { - return diag.FromErr(err) - } - if err := d.Set("argument", schemas.RowAccessPolicyArgumentsToSchema(args)); err != nil { + if err := d.Set("argument", schemas.RowAccessPolicyArgumentsToSchema(rowAccessPolicyDescription.Signature)); err != nil { return diag.FromErr(err) } if err = d.Set(ShowOutputAttributeName, []map[string]any{schemas.RowAccessPolicyToSchema(rowAccessPolicy)}); err != nil { diff --git a/pkg/resources/row_access_policy_acceptance_test.go b/pkg/resources/row_access_policy_acceptance_test.go index 796f1df167..5c931fe182 100644 --- a/pkg/resources/row_access_policy_acceptance_test.go +++ b/pkg/resources/row_access_policy_acceptance_test.go @@ -29,21 +29,21 @@ func TestAcc_RowAccessPolicy(t *testing.T) { argument := []sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "B", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, } changedArgument := []sdk.RowAccessPolicyArgument{ { Name: "C", - Type: string(sdk.DataTypeBoolean), + Type: sdk.DataTypeBoolean, }, { Name: "D", - Type: string(sdk.DataTypeTimestampNTZ), + Type: sdk.DataTypeTimestampNTZ, }, } policyModel := model.RowAccessPolicy("test", argument, body, id.DatabaseName(), id.Name(), id.SchemaName()).WithComment("Terraform acceptance test") @@ -80,7 +80,11 @@ func TestAcc_RowAccessPolicy(t *testing.T) { assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.body", body)), assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.name", id.Name())), assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.return_type", "BOOLEAN")), - assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature", "(A VARCHAR, B VARCHAR)")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature.#", "2")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature.0.name", "A")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature.0.type", string(sdk.DataTypeVARCHAR))), + assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature.1.name", "B")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "describe_output.0.signature.1.type", string(sdk.DataTypeVARCHAR))), ), }, // change comment and expression @@ -194,7 +198,7 @@ func TestAcc_RowAccessPolicy_Issue2053(t *testing.T) { policyModel := model.RowAccessPolicy("test", []sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }, body, id.DatabaseName(), id.Name(), id.SchemaName()) resource.Test(t, resource.TestCase{ @@ -252,7 +256,7 @@ func TestAcc_RowAccessPolicy_Rename(t *testing.T) { policyModel := model.RowAccessPolicy("test", []sdk.RowAccessPolicyArgument{ { Name: "a", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }, body, id.DatabaseName(), id.Name(), id.SchemaName()) @@ -390,7 +394,7 @@ func TestAcc_RowAccessPolicy_DataTypeAliases(t *testing.T) { HasArguments([]sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }), ), @@ -406,11 +410,11 @@ func TestAcc_view_migrateFromVersion_0_95_0_LowercaseArgName(t *testing.T) { policyModel := model.RowAccessPolicy("test", []sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "b", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }, body, id.DatabaseName(), id.Name(), id.SchemaName()) @@ -441,8 +445,8 @@ func TestAcc_view_migrateFromVersion_0_95_0_LowercaseArgName(t *testing.T) { HasSchemaString(id.SchemaName()). HasFullyQualifiedNameString(id.FullyQualifiedName()), assert.Check(resource.TestCheckResourceAttr(resourceName, "row_access_expression", body)), - assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.A", "VARCHAR")), - assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.B", "VARCHAR")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.A", string(sdk.DataTypeVARCHAR))), + assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.B", string(sdk.DataTypeVARCHAR))), ), }, { @@ -466,11 +470,11 @@ func TestAcc_view_migrateFromVersion_0_95_0_LowercaseArgName(t *testing.T) { HasArguments([]sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "b", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }), ), @@ -486,11 +490,11 @@ func TestAcc_view_migrateFromVersion_0_95_0_UppercaseArgName(t *testing.T) { policyModel := model.RowAccessPolicy("test", []sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "B", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }, body, id.DatabaseName(), id.Name(), id.SchemaName()) @@ -521,8 +525,8 @@ func TestAcc_view_migrateFromVersion_0_95_0_UppercaseArgName(t *testing.T) { HasSchemaString(id.SchemaName()). HasFullyQualifiedNameString(id.FullyQualifiedName()), assert.Check(resource.TestCheckResourceAttr(resourceName, "row_access_expression", body)), - assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.A", "VARCHAR")), - assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.B", "VARCHAR")), + assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.A", string(sdk.DataTypeVARCHAR))), + assert.Check(resource.TestCheckResourceAttr(resourceName, "signature.B", string(sdk.DataTypeVARCHAR))), ), }, { @@ -546,11 +550,11 @@ func TestAcc_view_migrateFromVersion_0_95_0_UppercaseArgName(t *testing.T) { HasArguments([]sdk.RowAccessPolicyArgument{ { Name: "A", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, { Name: "B", - Type: string(sdk.DataTypeVARCHAR), + Type: sdk.DataTypeVARCHAR, }, }), ), diff --git a/pkg/resources/testdata/TestAcc_View/basic/test.tf b/pkg/resources/testdata/TestAcc_View/basic/test.tf index 905a79328a..e61457cfac 100644 --- a/pkg/resources/testdata/TestAcc_View/basic/test.tf +++ b/pkg/resources/testdata/TestAcc_View/basic/test.tf @@ -5,7 +5,7 @@ resource "snowflake_view" "test" { statement = var.statement dynamic "column" { - for_each = var.columns + for_each = var.column content { column_name = column.value["column_name"] } diff --git a/pkg/resources/testdata/TestAcc_View/basic/variables.tf b/pkg/resources/testdata/TestAcc_View/basic/variables.tf index 2219f130a5..dc7d599d87 100644 --- a/pkg/resources/testdata/TestAcc_View/basic/variables.tf +++ b/pkg/resources/testdata/TestAcc_View/basic/variables.tf @@ -14,6 +14,6 @@ variable "statement" { type = string } -variable "columns" { +variable "column" { type = set(map(string)) } diff --git a/pkg/resources/testdata/TestAcc_View/basic_copy_grants/test.tf b/pkg/resources/testdata/TestAcc_View/basic_copy_grants/test.tf index cdc7295f9d..7586548757 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_copy_grants/test.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_copy_grants/test.tf @@ -7,7 +7,7 @@ resource "snowflake_view" "test" { is_secure = var.is_secure dynamic "column" { - for_each = var.columns + for_each = var.column content { column_name = column.value["column_name"] } diff --git a/pkg/resources/testdata/TestAcc_View/basic_copy_grants/variables.tf b/pkg/resources/testdata/TestAcc_View/basic_copy_grants/variables.tf index 86e63c4564..71a25def0e 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_copy_grants/variables.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_copy_grants/variables.tf @@ -22,6 +22,6 @@ variable "is_secure" { type = bool } -variable "columns" { +variable "column" { type = set(map(string)) } diff --git a/pkg/resources/testdata/TestAcc_View/basic_is_recursive/test.tf b/pkg/resources/testdata/TestAcc_View/basic_is_recursive/test.tf index 42308b4d48..54dc35e75e 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_is_recursive/test.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_is_recursive/test.tf @@ -6,7 +6,7 @@ resource "snowflake_view" "test" { is_recursive = var.is_recursive dynamic "column" { - for_each = var.columns + for_each = var.column content { column_name = column.value["column_name"] } diff --git a/pkg/resources/testdata/TestAcc_View/basic_is_recursive/variables.tf b/pkg/resources/testdata/TestAcc_View/basic_is_recursive/variables.tf index b38898bbcf..e3c42edcee 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_is_recursive/variables.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_is_recursive/variables.tf @@ -18,6 +18,6 @@ variable "is_recursive" { type = bool } -variable "columns" { +variable "column" { type = set(map(string)) } diff --git a/pkg/resources/testdata/TestAcc_View/basic_update/test.tf b/pkg/resources/testdata/TestAcc_View/basic_update/test.tf index 2df97a8f62..fd96e26315 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_update/test.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_update/test.tf @@ -4,7 +4,7 @@ resource "snowflake_view" "test" { schema = var.schema dynamic "column" { - for_each = var.columns + for_each = var.column content { column_name = column.value["column_name"] } diff --git a/pkg/resources/testdata/TestAcc_View/basic_update/variables.tf b/pkg/resources/testdata/TestAcc_View/basic_update/variables.tf index 4b814dd06d..2a299e3bdb 100644 --- a/pkg/resources/testdata/TestAcc_View/basic_update/variables.tf +++ b/pkg/resources/testdata/TestAcc_View/basic_update/variables.tf @@ -50,6 +50,6 @@ variable "schedule_status" { type = string } -variable "columns" { +variable "column" { type = set(map(string)) } diff --git a/pkg/resources/view.go b/pkg/resources/view.go index 0d66abadea..da319cfbf9 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -194,7 +194,8 @@ var viewSchema = map[string]*schema.Schema{ }, }, }, - Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. (You do not need to specify the data types of the columns.)", + Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake.", + DiffSuppressFunc: ignoreEmpty, }, "comment": { Type: schema.TypeString, diff --git a/pkg/resources/view_acceptance_test.go b/pkg/resources/view_acceptance_test.go index cf29e6b78f..cfe216b3a2 100644 --- a/pkg/resources/view_acceptance_test.go +++ b/pkg/resources/view_acceptance_test.go @@ -5,21 +5,20 @@ import ( "regexp" "testing" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/objectassert" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" - - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random" - acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" accconfig "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" + tfconfig "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/objectassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/resourceassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert/resourceshowoutputassert" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/config/model" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/importchecks" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeroles" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/provider/resources" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" @@ -71,7 +70,7 @@ func TestAcc_View_basic(t *testing.T) { "database": config.StringVariable(id.DatabaseName()), "schema": config.StringVariable(id.SchemaName()), "statement": config.StringVariable(configStatement), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ID"), }), @@ -100,7 +99,7 @@ func TestAcc_View_basic(t *testing.T) { "data_metric_schedule_using_cron": config.StringVariable(cron), "comment": config.StringVariable(comment), "schedule_status": config.StringVariable(string(scheduleStatus)), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ID"), }), @@ -427,7 +426,7 @@ func TestAcc_View_recursive(t *testing.T) { "schema": config.StringVariable(id.SchemaName()), "statement": config.StringVariable(statement), "is_recursive": config.BoolVariable(true), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ROLE_NAME"), }), @@ -705,7 +704,7 @@ end;; "database": config.StringVariable(id.DatabaseName()), "schema": config.StringVariable(id.SchemaName()), "statement": config.StringVariable(statement), - "columns": config.SetVariable( + "column": config.SetVariable( collections.Map(columns, func(columnName string) config.Variable { return config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable(columnName), @@ -717,7 +716,7 @@ end;; basicViewWithPolicies := func() config.Variables { conf := basicView("ID", "FOO") - delete(conf, "columns") + delete(conf, "column") conf["projection_name"] = config.StringVariable(projectionPolicy.FullyQualifiedName()) conf["masking_name"] = config.StringVariable(maskingPolicy.ID().FullyQualifiedName()) conf["masking_using"] = config.ListVariable(config.StringVariable("ID")) @@ -820,7 +819,7 @@ func TestAcc_View_Rename(t *testing.T) { "database": config.StringVariable(identifier.DatabaseName()), "schema": config.StringVariable(identifier.SchemaName()), "statement": config.StringVariable(statement), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ROLE_NAME"), }), @@ -865,6 +864,51 @@ func TestAcc_View_Rename(t *testing.T) { }) } +func TestAcc_View_Issue3073(t *testing.T) { + t.Setenv(string(testenvs.ConfigureClientOnce), "") + statement := "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES" + id := acc.TestClient().Ids.RandomSchemaObjectIdentifier() + viewModel := model.View("test", id.DatabaseName(), id.Name(), id.SchemaName(), statement) + viewModelWithColumns := model.View("test", id.DatabaseName(), id.Name(), id.SchemaName(), statement).WithColumnValue(config.SetVariable( + config.MapVariable(map[string]config.Variable{ + "column_name": config.StringVariable("ROLE_NAME"), + }), + config.MapVariable(map[string]config.Variable{ + "column_name": config.StringVariable("ROLE_OWNER"), + }), + )) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: acc.CheckDestroy(t, resources.View), + Steps: []resource.TestStep{ + { + Config: accconfig.FromModel(t, viewModel), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_view.test", "name", id.Name()), + ), + }, + // specify the columns + { + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_View/basic"), + ConfigVariables: tfconfig.ConfigVariablesFromModel(t, viewModelWithColumns), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_view.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_view.test", "name", id.Name()), + ), + }, + }, + }) +} + func TestAcc_ViewChangeCopyGrants(t *testing.T) { t.Setenv(string(testenvs.ConfigureClientOnce), "") id := acc.TestClient().Ids.RandomSchemaObjectIdentifier() @@ -878,7 +922,7 @@ func TestAcc_ViewChangeCopyGrants(t *testing.T) { "statement": config.StringVariable(statement), "copy_grants": config.BoolVariable(copyGrants), "is_secure": config.BoolVariable(true), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ID"), }), @@ -946,7 +990,7 @@ func TestAcc_ViewChangeCopyGrantsReversed(t *testing.T) { "statement": config.StringVariable(statement), "copy_grants": config.BoolVariable(copyGrants), "is_secure": config.BoolVariable(true), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ID"), }), @@ -1098,7 +1142,7 @@ func TestAcc_view_migrateFromVersion_0_94_1(t *testing.T) { "database": config.StringVariable(id.DatabaseName()), "schema": config.StringVariable(id.SchemaName()), "statement": config.StringVariable(statement), - "columns": config.SetVariable( + "column": config.SetVariable( config.MapVariable(map[string]config.Variable{ "column_name": config.StringVariable("ROLE_NAME"), }), diff --git a/pkg/schemas/row_access_policy.go b/pkg/schemas/row_access_policy.go index ddb04f49aa..578f74c830 100644 --- a/pkg/schemas/row_access_policy.go +++ b/pkg/schemas/row_access_policy.go @@ -11,7 +11,19 @@ var RowAccessPolicyDescribeSchema = map[string]*schema.Schema{ Computed: true, }, "signature": { - Type: schema.TypeString, + Type: schema.TypeList, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "name": { + Type: schema.TypeString, + Computed: true, + }, + "type": { + Type: schema.TypeString, + Computed: true, + }, + }, + }, Computed: true, }, "return_type": { @@ -25,9 +37,16 @@ var RowAccessPolicyDescribeSchema = map[string]*schema.Schema{ } func RowAccessPolicyDescriptionToSchema(description sdk.RowAccessPolicyDescription) map[string]any { + signatureElem := make([]map[string]any, len(description.Signature)) + for i, v := range description.Signature { + signatureElem[i] = map[string]any{ + "name": v.Name, + "type": string(v.Type), + } + } return map[string]any{ "name": description.Name, - "signature": description.Signature, + "signature": signatureElem, "return_type": description.ReturnType, "body": description.Body, } diff --git a/pkg/sdk/row_access_policies_gen.go b/pkg/sdk/row_access_policies_gen.go index df9d720e11..64ba7c4096 100644 --- a/pkg/sdk/row_access_policies_gen.go +++ b/pkg/sdk/row_access_policies_gen.go @@ -3,8 +3,6 @@ package sdk import ( "context" "database/sql" - "fmt" - "strings" ) type RowAccessPolicies interface { @@ -109,33 +107,11 @@ type describeRowAccessPolicyDBRow struct { type RowAccessPolicyDescription struct { Name string - Signature string + Signature []RowAccessPolicyArgument ReturnType string Body string } type RowAccessPolicyArgument struct { Name string - Type string -} - -// TODO(SNOW-1596962): Fully support VECTOR data type -// TODO(SNOW-1660588): Use ParseFunctionArgumentsFromString -func (d *RowAccessPolicyDescription) Arguments() ([]RowAccessPolicyArgument, error) { - // Format in database is `(column )` - plainSignature := strings.ReplaceAll(d.Signature, "(", "") - plainSignature = strings.ReplaceAll(plainSignature, ")", "") - signatureParts := strings.Split(plainSignature, ", ") - arguments := make([]RowAccessPolicyArgument, len(signatureParts)) - - for i, e := range signatureParts { - parts := strings.Split(e, " ") - if len(parts) < 2 { - return nil, fmt.Errorf("parsing policy arguments: expected argument name and type, got %s", e) - } - arguments[i] = RowAccessPolicyArgument{ - Name: strings.Join(parts[:len(parts)-1], " "), - Type: parts[len(parts)-1], - } - } - return arguments, nil + Type DataType } diff --git a/pkg/sdk/row_access_policies_gen_test.go b/pkg/sdk/row_access_policies_gen_test.go index f4590f3b9e..c45d5da68b 100644 --- a/pkg/sdk/row_access_policies_gen_test.go +++ b/pkg/sdk/row_access_policies_gen_test.go @@ -297,35 +297,11 @@ func TestRowAccessPolicyDescription_Arguments(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &RowAccessPolicyDescription{ + d := &describeRowAccessPolicyDBRow{ Signature: tt.signature, } - got, err := d.Arguments() - require.NoError(t, err) - require.Equal(t, tt.want, got) - }) - } -} - -func TestRowAccessPolicyDescription_ArgumentsInvalid(t *testing.T) { - tests := []struct { - name string - signature string - err string - }{ - { - name: "signature without data type", - signature: "(A)", - err: "parsing policy arguments: expected argument name and type, got A", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - d := &RowAccessPolicyDescription{ - Signature: tt.signature, - } - _, err := d.Arguments() - require.ErrorContains(t, err, tt.err) + got := d.convert() + require.Equal(t, tt.want, got.Signature) }) } } diff --git a/pkg/sdk/row_access_policies_impl_gen.go b/pkg/sdk/row_access_policies_impl_gen.go index e008b5a62c..d02092e9cd 100644 --- a/pkg/sdk/row_access_policies_impl_gen.go +++ b/pkg/sdk/row_access_policies_impl_gen.go @@ -2,6 +2,8 @@ package sdk import ( "context" + "log" + "strings" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" ) @@ -133,9 +135,34 @@ func (r *DescribeRowAccessPolicyRequest) toOpts() *DescribeRowAccessPolicyOption func (r describeRowAccessPolicyDBRow) convert() *RowAccessPolicyDescription { rowAccessPolicyDescription := &RowAccessPolicyDescription{ Name: r.Name, - Signature: r.Signature, ReturnType: r.ReturnType, Body: r.Body, } + // Format in database is `(column )` + // TODO(SNOW-1596962): Fully support VECTOR data type + // TODO(SNOW-1660588): Use ParseFunctionArgumentsFromString + plainSignature := strings.ReplaceAll(r.Signature, "(", "") + plainSignature = strings.ReplaceAll(plainSignature, ")", "") + signatureParts := strings.Split(plainSignature, ", ") + arguments := make([]RowAccessPolicyArgument, len(signatureParts)) + + for i, e := range signatureParts { + parts := strings.Split(e, " ") + if len(parts) < 2 { + log.Printf("[DEBUG] parsing policy arguments: expected argument name and type, got %s", e) + continue + } + dataType, err := ToDataType(parts[len(parts)-1]) + if err != nil { + log.Printf("[DEBUG] converting row access policy db row: invalid data type %s", dataType) + continue + } + arguments[i] = RowAccessPolicyArgument{ + Name: strings.Join(parts[:len(parts)-1], " "), + Type: dataType, + } + } + rowAccessPolicyDescription.Signature = arguments + return rowAccessPolicyDescription } diff --git a/pkg/sdk/testint/row_access_policies_gen_integration_test.go b/pkg/sdk/testint/row_access_policies_gen_integration_test.go index 87d1eb79fe..516b5c4787 100644 --- a/pkg/sdk/testint/row_access_policies_gen_integration_test.go +++ b/pkg/sdk/testint/row_access_policies_gen_integration_test.go @@ -1,7 +1,6 @@ package testint import ( - "fmt" "testing" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random" @@ -28,11 +27,11 @@ func TestInt_RowAccessPolicies(t *testing.T) { assert.Equal(t, "ROLE", rowAccessPolicy.OwnerRoleType) } - assertRowAccessPolicyDescription := func(t *testing.T, rowAccessPolicyDescription *sdk.RowAccessPolicyDescription, id sdk.SchemaObjectIdentifier, expectedSignature string, expectedBody string) { + assertRowAccessPolicyDescription := func(t *testing.T, rowAccessPolicyDescription *sdk.RowAccessPolicyDescription, id sdk.SchemaObjectIdentifier, signature []sdk.RowAccessPolicyArgument, expectedBody string) { t.Helper() assert.Equal(t, sdk.RowAccessPolicyDescription{ Name: id.Name(), - Signature: expectedSignature, + Signature: signature, ReturnType: "BOOLEAN", Body: expectedBody, }, *rowAccessPolicyDescription) @@ -251,7 +250,10 @@ func TestInt_RowAccessPolicies(t *testing.T) { returnedRowAccessPolicyDescription, err := client.RowAccessPolicies.Describe(ctx, rowAccessPolicy.ID()) require.NoError(t, err) - assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), fmt.Sprintf("(%s %s)", argName, argType), body) + assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), []sdk.RowAccessPolicyArgument{{ + Name: argName, + Type: argType, + }}, body) }) t.Run("describe row access policy: with timestamp data type normalization", func(t *testing.T) { @@ -267,7 +269,10 @@ func TestInt_RowAccessPolicies(t *testing.T) { returnedRowAccessPolicyDescription, err := client.RowAccessPolicies.Describe(ctx, rowAccessPolicy.ID()) require.NoError(t, err) - assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), fmt.Sprintf("(%s %s)", argName, sdk.DataTypeTimestampNTZ), body) + assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), []sdk.RowAccessPolicyArgument{{ + Name: argName, + Type: sdk.DataTypeTimestampNTZ, + }}, body) }) t.Run("describe row access policy: with varchar data type normalization", func(t *testing.T) { @@ -283,7 +288,10 @@ func TestInt_RowAccessPolicies(t *testing.T) { returnedRowAccessPolicyDescription, err := client.RowAccessPolicies.Describe(ctx, rowAccessPolicy.ID()) require.NoError(t, err) - assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), fmt.Sprintf("(%s %s)", argName, sdk.DataTypeVARCHAR), body) + assertRowAccessPolicyDescription(t, returnedRowAccessPolicyDescription, rowAccessPolicy.ID(), []sdk.RowAccessPolicyArgument{{ + Name: argName, + Type: sdk.DataTypeVARCHAR, + }}, body) }) t.Run("describe row access policy: non-existing", func(t *testing.T) { @@ -368,7 +376,6 @@ func TestInt_RowAccessPoliciesDescribe(t *testing.T) { assert.Equal(t, "true", policyDetails.Body) assert.Equal(t, id.Name(), policyDetails.Name) assert.Equal(t, "BOOLEAN", policyDetails.ReturnType) - gotArgs, err := policyDetails.Arguments() require.NoError(t, err) wantArgs := make([]sdk.RowAccessPolicyArgument, len(args)) for i, arg := range args { @@ -376,9 +383,9 @@ func TestInt_RowAccessPoliciesDescribe(t *testing.T) { require.NoError(t, err) wantArgs[i] = sdk.RowAccessPolicyArgument{ Name: arg.Name, - Type: string(dataType), + Type: dataType, } } - assert.Equal(t, wantArgs, gotArgs) + assert.Equal(t, wantArgs, policyDetails.Signature) }) } From 6bfcef35b8ae54080fb97a7ff15c9c53a1cd13c6 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 16 Sep 2024 13:42:35 +0200 Subject: [PATCH 2/6] Rename --- pkg/resources/diff_suppressions.go | 2 +- pkg/resources/view.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/resources/diff_suppressions.go b/pkg/resources/diff_suppressions.go index aace546ff5..d380ad9a38 100644 --- a/pkg/resources/diff_suppressions.go +++ b/pkg/resources/diff_suppressions.go @@ -222,7 +222,7 @@ func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceD return slices.Equal(oldId, newId) } -func ignoreEmpty(k, oldValue, newValue string, _ *schema.ResourceData) bool { +func ignoreEmptyList(k, oldValue, newValue string, _ *schema.ResourceData) bool { if strings.HasSuffix(k, ".#") && newValue == "0" { return true } diff --git a/pkg/resources/view.go b/pkg/resources/view.go index da319cfbf9..9c6f327fb4 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -195,7 +195,7 @@ var viewSchema = map[string]*schema.Schema{ }, }, Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake.", - DiffSuppressFunc: ignoreEmpty, + DiffSuppressFunc: ignoreEmptyList, }, "comment": { Type: schema.TypeString, From 634bada461d1674574cdd28c45cc7303dd9f0d49 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 16 Sep 2024 14:14:55 +0200 Subject: [PATCH 3/6] Fixes --- docs/data-sources/row_access_policies.md | 11 ++++++++++- docs/guides/identifiers.md | 12 +++++++++--- docs/resources/row_access_policy.md | 11 ++++++++++- docs/resources/view.md | 2 +- pkg/resources/view_acceptance_test.go | 18 +++++++++++++++++- templates/guides/identifiers.md.tmpl | 12 +++++++++--- 6 files changed, 56 insertions(+), 10 deletions(-) diff --git a/docs/data-sources/row_access_policies.md b/docs/data-sources/row_access_policies.md index 076c8b87f9..b6ccb31cd8 100644 --- a/docs/data-sources/row_access_policies.md +++ b/docs/data-sources/row_access_policies.md @@ -152,7 +152,16 @@ Read-Only: - `body` (String) - `name` (String) - `return_type` (String) -- `signature` (String) +- `signature` (List of Object) (see [below for nested schema](#nestedobjatt--row_access_policies--describe_output--signature)) + + +### Nested Schema for `row_access_policies.describe_output.signature` + +Read-Only: + +- `name` (String) +- `type` (String) + diff --git a/docs/guides/identifiers.md b/docs/guides/identifiers.md index 94def58363..1813be44e0 100644 --- a/docs/guides/identifiers.md +++ b/docs/guides/identifiers.md @@ -12,13 +12,19 @@ With the combination of quotes, old parsing methods, and other factors, it was a For example, instead of writing -```object_name = “\”${snowflake_table.database}\”.\”${snowflake_table.schema}\”.\”${snowflake_table.name}\””``` +``` +object_name = “\”${snowflake_table.database}\”.\”${snowflake_table.schema}\”.\”${snowflake_table.name}\”” +# for procedures +object_name = “\”${snowflake_procedure.database}\”.\”${snowflake_procedure.schema}\”.\”${snowflake_procedure.name}(NUMBER, VARCHAR)\”” +``` now we can write -```object_name = snowflake_table.fully_qualified_name``` +``` +object_name = snowflake_table.fully_qualified_name +``` -This is our recommended way of referencing other objects. However, if you don't manage table in Terraform, you can construct the proper id yourself like before: `"\"database_name\".\"schema_name\".\"table_name\""` Note that quotes are necessary for correct parsing of an identifier. +This is our recommended way of referencing other objects. However, if you don't manage the referenced object in Terraform, you can construct the proper id yourself like before: `"\"database_name\".\"schema_name\".\"object_name\""` for schema-level objects, or `"\"database_name\".\"schema_name\".\"procedure_name(NUMBER, VARCHAR)\""` for procedures. Note that quotes are necessary for correct parsing of an identifier. This change was announced in v0.95.0 [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#new-fully_qualified_name-field-in-the-resources). diff --git a/docs/resources/row_access_policy.md b/docs/resources/row_access_policy.md index c11557428e..5e0c651176 100644 --- a/docs/resources/row_access_policy.md +++ b/docs/resources/row_access_policy.md @@ -76,7 +76,16 @@ Read-Only: - `body` (String) - `name` (String) - `return_type` (String) -- `signature` (String) +- `signature` (List of Object) (see [below for nested schema](#nestedobjatt--describe_output--signature)) + + +### Nested Schema for `describe_output.signature` + +Read-Only: + +- `name` (String) +- `type` (String) + diff --git a/docs/resources/view.md b/docs/resources/view.md index ab2682b498..ba07139c62 100644 --- a/docs/resources/view.md +++ b/docs/resources/view.md @@ -98,7 +98,7 @@ SQL - `aggregation_policy` (Block List, Max: 1) Specifies the aggregation policy to set on a view. (see [below for nested schema](#nestedblock--aggregation_policy)) - `change_tracking` (String) Specifies to enable or disable change tracking on the table. Available options are: "true" or "false". When the value is not set in the configuration the provider will put "default" there which means to use the Snowflake default for this value. -- `column` (Block List) If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. (You do not need to specify the data types of the columns.) (see [below for nested schema](#nestedblock--column)) +- `column` (Block List) If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake. (see [below for nested schema](#nestedblock--column)) - `comment` (String) Specifies a comment for the view. - `copy_grants` (Boolean) Retains the access permissions from the original view when a new view is created using the OR REPLACE clause. - `data_metric_function` (Block Set) Data metric functions used for the view. (see [below for nested schema](#nestedblock--data_metric_function)) diff --git a/pkg/resources/view_acceptance_test.go b/pkg/resources/view_acceptance_test.go index cfe216b3a2..9c00b77092 100644 --- a/pkg/resources/view_acceptance_test.go +++ b/pkg/resources/view_acceptance_test.go @@ -888,6 +888,11 @@ func TestAcc_View_Issue3073(t *testing.T) { Steps: []resource.TestStep{ { Config: accconfig.FromModel(t, viewModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_view.test", plancheck.ResourceActionNoop), + }, + }, Check: resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr("snowflake_view.test", "name", id.Name()), ), @@ -897,7 +902,18 @@ func TestAcc_View_Issue3073(t *testing.T) { ConfigDirectory: acc.ConfigurationDirectory("TestAcc_View/basic"), ConfigVariables: tfconfig.ConfigVariablesFromModel(t, viewModelWithColumns), ConfigPlanChecks: resource.ConfigPlanChecks{ - PreApply: []plancheck.PlanCheck{ + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_view.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_view.test", "name", id.Name()), + ), + }, + { + Config: accconfig.FromModel(t, viewModel), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPostRefresh: []plancheck.PlanCheck{ plancheck.ExpectResourceAction("snowflake_view.test", plancheck.ResourceActionNoop), }, }, diff --git a/templates/guides/identifiers.md.tmpl b/templates/guides/identifiers.md.tmpl index 94def58363..1813be44e0 100644 --- a/templates/guides/identifiers.md.tmpl +++ b/templates/guides/identifiers.md.tmpl @@ -12,13 +12,19 @@ With the combination of quotes, old parsing methods, and other factors, it was a For example, instead of writing -```object_name = “\”${snowflake_table.database}\”.\”${snowflake_table.schema}\”.\”${snowflake_table.name}\””``` +``` +object_name = “\”${snowflake_table.database}\”.\”${snowflake_table.schema}\”.\”${snowflake_table.name}\”” +# for procedures +object_name = “\”${snowflake_procedure.database}\”.\”${snowflake_procedure.schema}\”.\”${snowflake_procedure.name}(NUMBER, VARCHAR)\”” +``` now we can write -```object_name = snowflake_table.fully_qualified_name``` +``` +object_name = snowflake_table.fully_qualified_name +``` -This is our recommended way of referencing other objects. However, if you don't manage table in Terraform, you can construct the proper id yourself like before: `"\"database_name\".\"schema_name\".\"table_name\""` Note that quotes are necessary for correct parsing of an identifier. +This is our recommended way of referencing other objects. However, if you don't manage the referenced object in Terraform, you can construct the proper id yourself like before: `"\"database_name\".\"schema_name\".\"object_name\""` for schema-level objects, or `"\"database_name\".\"schema_name\".\"procedure_name(NUMBER, VARCHAR)\""` for procedures. Note that quotes are necessary for correct parsing of an identifier. This change was announced in v0.95.0 [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#new-fully_qualified_name-field-in-the-resources). From 1b92b97d9e6e7d28caf9fee65d66a25b55232ed1 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Mon, 16 Sep 2024 16:26:44 +0200 Subject: [PATCH 4/6] Fix tests --- pkg/resources/diff_suppressions.go | 7 +++---- pkg/resources/row_access_policy_acceptance_test.go | 3 --- pkg/resources/view.go | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/resources/diff_suppressions.go b/pkg/resources/diff_suppressions.go index d380ad9a38..90afb0ab31 100644 --- a/pkg/resources/diff_suppressions.go +++ b/pkg/resources/diff_suppressions.go @@ -222,9 +222,8 @@ func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceD return slices.Equal(oldId, newId) } -func ignoreEmptyList(k, oldValue, newValue string, _ *schema.ResourceData) bool { - if strings.HasSuffix(k, ".#") && newValue == "0" { - return true +func ignoreEmptyList(key string) schema.SchemaDiffSuppressFunc { + return func(k, old, new string, d *schema.ResourceData) bool { + return key == k && new == "0" } - return newValue == "" } diff --git a/pkg/resources/row_access_policy_acceptance_test.go b/pkg/resources/row_access_policy_acceptance_test.go index 5c931fe182..f8e7b89812 100644 --- a/pkg/resources/row_access_policy_acceptance_test.go +++ b/pkg/resources/row_access_policy_acceptance_test.go @@ -534,9 +534,6 @@ func TestAcc_view_migrateFromVersion_0_95_0_UppercaseArgName(t *testing.T) { ConfigDirectory: acc.ConfigurationDirectory("TestAcc_RowAccessPolicy/basic"), ConfigVariables: tfconfig.ConfigVariablesFromModel(t, policyModel), ConfigPlanChecks: resource.ConfigPlanChecks{ - PreApply: []plancheck.PlanCheck{ - plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), - }, PostApplyPostRefresh: []plancheck.PlanCheck{ plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), }, diff --git a/pkg/resources/view.go b/pkg/resources/view.go index 9c6f327fb4..c2ec18ecec 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -195,7 +195,7 @@ var viewSchema = map[string]*schema.Schema{ }, }, Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake.", - DiffSuppressFunc: ignoreEmptyList, + DiffSuppressFunc: ignoreEmptyList("column.#"), }, "comment": { Type: schema.TypeString, From a8c1461e27d7ecefa1cf6e751ba341ec9591c9d4 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Tue, 17 Sep 2024 11:07:55 +0200 Subject: [PATCH 5/6] Fix tests --- pkg/resources/diff_suppressions.go | 17 ++++++-- pkg/resources/diff_suppressions_test.go | 54 +++++++++++++++++++++++++ pkg/resources/view.go | 2 +- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/pkg/resources/diff_suppressions.go b/pkg/resources/diff_suppressions.go index 90afb0ab31..a4be599d48 100644 --- a/pkg/resources/diff_suppressions.go +++ b/pkg/resources/diff_suppressions.go @@ -222,8 +222,19 @@ func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceD return slices.Equal(oldId, newId) } -func ignoreEmptyList(key string) schema.SchemaDiffSuppressFunc { - return func(k, old, new string, d *schema.ResourceData) bool { - return key == k && new == "0" +// IgnoreNewEmptyList suppresses the diff if `new` list is empty or compared subfield is ignored +func IgnoreNewEmptyList(subfields []string) schema.SchemaDiffSuppressFunc { + return func(k, old, new string, _ *schema.ResourceData) bool { + parts := strings.SplitN(k, ".", 3) + if len(parts) < 2 { + log.Printf("[DEBUG] invalid resource key: %s", parts) + return false + } + // key is element count + if parts[1] == "#" && new == "0" { + return true + } + // key is one of the ignored subfields + return len(parts) > 2 && slices.Contains(subfields, parts[2]) && new == "" } } diff --git a/pkg/resources/diff_suppressions_test.go b/pkg/resources/diff_suppressions_test.go index f7ff8cc131..bd9cd8973a 100644 --- a/pkg/resources/diff_suppressions_test.go +++ b/pkg/resources/diff_suppressions_test.go @@ -8,6 +8,7 @@ import ( "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NormalizeAndCompare(t *testing.T) { @@ -150,3 +151,56 @@ func Test_NormalizeAndCompareIdentifiersSet(t *testing.T) { // assert.True(t, resources.NormalizeAndCompareIdentifiersInSet("value")("value.doesnt_matter", "", "SCHEMA.OBJECT.IDENTIFIER", resourceData)) }) } + +func Test_ignoreNewEmptyList(t *testing.T) { + tests := []struct { + name string + subfields []string + key string + old string + new string + suppress bool + }{ + { + name: "suppress on zero count", + key: "a.#", + old: "5", + new: "0", + suppress: true, + }, + { + name: "suppress on ignored field", + key: "a.0.b", + subfields: []string{"b"}, + suppress: true, + }, + { + name: "suppress on nested ignored field", + key: "a.0.b.c.d", + subfields: []string{"b.c.d"}, + suppress: true, + }, + { + name: "do not suppress on non-zero count", + key: "a.#", + new: "5", + suppress: false, + }, + { + name: "do not suppress on non-ignored field", + key: "a.0.b", + subfields: []string{"c"}, + suppress: false, + }, + { + name: "do not suppress on invalid key", + key: "a", + suppress: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.suppress, resources.IgnoreNewEmptyList(tt.subfields)(tt.key, tt.old, tt.new, nil)) + }) + } +} diff --git a/pkg/resources/view.go b/pkg/resources/view.go index c2ec18ecec..a01ed3e78c 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -195,7 +195,7 @@ var viewSchema = map[string]*schema.Schema{ }, }, Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake.", - DiffSuppressFunc: ignoreEmptyList("column.#"), + DiffSuppressFunc: IgnoreNewEmptyList([]string{"column_name"}), }, "comment": { Type: schema.TypeString, From c4cf1a5ccd0a54d6b54ad8d323ccf0ae38b861b2 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Tue, 17 Sep 2024 13:51:39 +0200 Subject: [PATCH 6/6] Review suggestions --- pkg/resources/diff_suppressions.go | 6 +++--- pkg/resources/diff_suppressions_test.go | 2 +- pkg/resources/view.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/resources/diff_suppressions.go b/pkg/resources/diff_suppressions.go index a4be599d48..3e23be3ee0 100644 --- a/pkg/resources/diff_suppressions.go +++ b/pkg/resources/diff_suppressions.go @@ -222,8 +222,8 @@ func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceD return slices.Equal(oldId, newId) } -// IgnoreNewEmptyList suppresses the diff if `new` list is empty or compared subfield is ignored -func IgnoreNewEmptyList(subfields []string) schema.SchemaDiffSuppressFunc { +// IgnoreNewEmptyListOrSubfields suppresses the diff if `new` list is empty or compared subfield is ignored. Subfields can be nested. +func IgnoreNewEmptyListOrSubfields(ignoredSubfields ...string) schema.SchemaDiffSuppressFunc { return func(k, old, new string, _ *schema.ResourceData) bool { parts := strings.SplitN(k, ".", 3) if len(parts) < 2 { @@ -235,6 +235,6 @@ func IgnoreNewEmptyList(subfields []string) schema.SchemaDiffSuppressFunc { return true } // key is one of the ignored subfields - return len(parts) > 2 && slices.Contains(subfields, parts[2]) && new == "" + return len(parts) == 3 && slices.Contains(ignoredSubfields, parts[2]) && new == "" } } diff --git a/pkg/resources/diff_suppressions_test.go b/pkg/resources/diff_suppressions_test.go index bd9cd8973a..7c54f03938 100644 --- a/pkg/resources/diff_suppressions_test.go +++ b/pkg/resources/diff_suppressions_test.go @@ -200,7 +200,7 @@ func Test_ignoreNewEmptyList(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.suppress, resources.IgnoreNewEmptyList(tt.subfields)(tt.key, tt.old, tt.new, nil)) + require.Equal(t, tt.suppress, resources.IgnoreNewEmptyListOrSubfields(tt.subfields...)(tt.key, tt.old, tt.new, nil)) }) } } diff --git a/pkg/resources/view.go b/pkg/resources/view.go index a01ed3e78c..d9f2f6bb51 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -195,7 +195,7 @@ var viewSchema = map[string]*schema.Schema{ }, }, Description: "If you want to change the name of a column or add a comment to a column in the new view, include a column list that specifies the column names and (if needed) comments about the columns. You do not need to specify the data types of the columns. If this field is not specified, columns are inferred from the `statement` field by Snowflake.", - DiffSuppressFunc: IgnoreNewEmptyList([]string{"column_name"}), + DiffSuppressFunc: IgnoreNewEmptyListOrSubfields("column_name"), }, "comment": { Type: schema.TypeString,