fix: Fix #1947, #2134, #2173, and #2176 (#2192)

* Fix file format csv parameters config

Fixes #2176

* Fix show by id for dynamic table

Fixes #2173

* Add helper method

* Refactor test

* Fix 2134

Fixes #2134

* Fix 1947

Fixes #1947

* Fix fmt

* Fix after review

pkg/acceptance/testing.go

@@ -2,6 +2,8 @@ package acceptance
 
 import (
 	"context"
+	"path/filepath"
+	"strconv"
 	"sync"
 	"testing"
 
@@ -10,6 +12,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
 	"github.com/hashicorp/terraform-plugin-mux/tf5to6server"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-plugin-testing/config"
 )
 
 const (
@@ -72,3 +75,11 @@ func TestAccPreCheck(t *testing.T) {
 		}
 	})
 }
+
+// ConfigurationSameAsStepN should be used to obtain configuration for one of the previous steps to avoid duplication of configuration and var files.
+// Based on config.TestStepDirectory.
+func ConfigurationSameAsStepN(step int) func(config.TestStepConfigRequest) string {
+	return func(req config.TestStepConfigRequest) string {
+		return filepath.Join("testdata", req.TestName, strconv.Itoa(step))
+	}
+}

pkg/resources/common.go

@@ -0,0 +1,25 @@
+package resources
+
+import (
+	"strings"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+// DiffSuppressStatement will suppress diffs between statements if they differ in only case or in
+// runs of whitespace (\s+ = \s). This is needed because the snowflake api does not faithfully
+// round-trip queries, so we cannot do a simple character-wise comparison to detect changes.
+//
+// Warnings: We will have false positives in cases where a change in case or run of whitespace is
+// semantically significant.
+//
+// If we can find a sql parser that can handle the snowflake dialect then we should switch to parsing
+// queries and either comparing ASTs or emitting a canonical serialization for comparison. I couldn't
+// find such a library.
+func DiffSuppressStatement(_, old, new string, _ *schema.ResourceData) bool {
+	return strings.EqualFold(normalizeQuery(old), normalizeQuery(new))
+}
+
+func normalizeQuery(str string) string {
+	return strings.TrimSpace(space.ReplaceAllString(str, " "))
+}

pkg/resources/dynamic_table.go

@@ -63,9 +63,10 @@ var dynamicTableShema = map[string]*schema.Schema{
 		ForceNew:    true,
 	},
 	"query": {
-		Type:        schema.TypeString,
-		Required:    true,
-		Description: "Specifies the query to use to populate the dynamic table.",
+		Type:             schema.TypeString,
+		Required:         true,
+		Description:      "Specifies the query to use to populate the dynamic table.",
+		DiffSuppressFunc: DiffSuppressStatement,
 	},
 	"comment": {
 		Type:        schema.TypeString,
@@ -298,20 +299,25 @@ func UpdateDynamicTable(d *schema.ResourceData, meta interface{}) error {
 	id := helpers.DecodeSnowflakeID(d.Id()).(sdk.SchemaObjectIdentifier)
 	request := sdk.NewAlterDynamicTableRequest(id)
 
+	runSet := false
 	set := sdk.NewDynamicTableSetRequest()
 	if d.HasChange("target_lag") {
 		tl := parseTargetLag(d.Get("target_lag"))
 		set.WithTargetLag(tl)
+		runSet = true
 	}
 
 	if d.HasChange("warehouse") {
 		warehouseName := d.Get("warehouse").(string)
 		set.WithWarehouse(sdk.NewAccountObjectIdentifier(warehouseName))
+		runSet = true
 	}
 
-	request.WithSet(set)
-	if err := client.DynamicTables.Alter(ctx, request); err != nil {
-		return err
+	if runSet {
+		request.WithSet(set)
+		if err := client.DynamicTables.Alter(ctx, request); err != nil {
+			return err
+		}
 	}
 
 	if d.HasChange("comment") {

pkg/resources/dynamic_table_acceptance_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/plancheck"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 	"github.com/hashicorp/terraform-plugin-testing/tfversion"
 )
@@ -99,6 +100,142 @@ func TestAcc_DynamicTable_basic(t *testing.T) {
 	})
 }
 
+// TestAcc_DynamicTable_issue2173 proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2173 issue.
+func TestAcc_DynamicTable_issue2173(t *testing.T) {
+	dynamicTableName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	tableName := dynamicTableName + "_table"
+	query := fmt.Sprintf(`select "id" from "%v"."%v"."%v"`, acc.TestDatabaseName, acc.TestSchemaName, tableName)
+	otherSchema := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	m := func() map[string]config.Variable {
+		return map[string]config.Variable{
+			"name":         config.StringVariable(dynamicTableName),
+			"database":     config.StringVariable(acc.TestDatabaseName),
+			"schema":       config.StringVariable(acc.TestSchemaName),
+			"warehouse":    config.StringVariable(acc.TestWarehouseName),
+			"query":        config.StringVariable(query),
+			"comment":      config.StringVariable("Terraform acceptance test for GH issue 2173"),
+			"table_name":   config.StringVariable(tableName),
+			"other_schema": config.StringVariable(otherSchema),
+		}
+	}
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckDynamicTableDestroy,
+		Steps: []resource.TestStep{
+			{
+				ConfigDirectory: config.TestStepDirectory(),
+				ConfigVariables: m(),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{plancheck.ExpectNonEmptyPlan()},
+				},
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.other_schema", "name", otherSchema),
+					resource.TestCheckResourceAttr("snowflake_table.t", "name", tableName),
+				),
+			},
+			{
+				PreConfig:       func() { createDynamicTableOutsideTerraform(t, otherSchema, dynamicTableName, query) },
+				ConfigDirectory: config.TestStepDirectory(),
+				ConfigVariables: m(),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{plancheck.ExpectNonEmptyPlan()},
+				},
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_dynamic_table.dt", "name", dynamicTableName),
+				),
+			},
+			{
+				// We use the same config here as in the previous step so the plan should be empty.
+				ConfigDirectory: acc.ConfigurationSameAsStepN(2),
+				ConfigVariables: m(),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					/*
+					 * Before the fix this step resulted in
+					 *     # snowflake_dynamic_table.dt will be updated in-place
+					 *     ~ resource "snowflake_dynamic_table" "dt" {
+					 *         + comment              = "Terraform acceptance test for GH issue 2173"
+					 *           id                   = "terraform_test_database|terraform_test_schema|SFVNXKJFAA"
+					 *           name                 = "SFVNXKJFAA"
+					 *         ~ schema               = "MEYIYWUGGO" -> "terraform_test_schema"
+					 *           # (14 unchanged attributes hidden)
+					 *     }
+					 * which matches the issue description exactly (issue mentioned also query being changed but here for simplicity the same underlying table and query were used).
+					 */
+					PreApply: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()},
+				},
+			},
+		},
+	})
+}
+
+// TestAcc_DynamicTable_issue2134 proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2134 issue.
+func TestAcc_DynamicTable_issue2134(t *testing.T) {
+	dynamicTableName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	tableName := dynamicTableName + "_table"
+	// whitespace (initial tab) is added on purpose here
+	query := fmt.Sprintf(`	select "id" from "%v"."%v"."%v"`, acc.TestDatabaseName, acc.TestSchemaName, tableName)
+	m := func() map[string]config.Variable {
+		return map[string]config.Variable{
+			"name":       config.StringVariable(dynamicTableName),
+			"database":   config.StringVariable(acc.TestDatabaseName),
+			"schema":     config.StringVariable(acc.TestSchemaName),
+			"warehouse":  config.StringVariable(acc.TestWarehouseName),
+			"query":      config.StringVariable(query),
+			"comment":    config.StringVariable("Terraform acceptance test for GH issue 2134"),
+			"table_name": config.StringVariable(tableName),
+		}
+	}
+	m2 := m()
+	m2["comment"] = config.StringVariable("Changed comment")
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckDynamicTableDestroy,
+		Steps: []resource.TestStep{
+			/*
+			 * Before the fix the first step resulted in not empty plan (as expected)
+			 *     # snowflake_dynamic_table.dt will be updated in-place
+			 *     ~ resource "snowflake_dynamic_table" "dt" {
+			 *         id                   = "terraform_test_database|terraform_test_schema|IKLBYWKSOV"
+			 *         name                 = "IKLBYWKSOV"
+			 *         ~ query                = "select \"id\" from \"terraform_test_database\".\"terraform_test_schema\".\"IKLBYWKSOV_table\"" -> "\tselect \"id\" from \"terraform_test_database\".\"terraform_test_schema\".\"IKLBYWKSOV_table\""
+			 *         # (15 unchanged attributes hidden)
+			 *     }
+			 * which matches the issue description exactly.
+			 */
+			{
+				ConfigDirectory: config.TestStepDirectory(),
+				ConfigVariables: m(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_dynamic_table.dt", "name", dynamicTableName),
+				),
+			},
+			/*
+			 * Before the fix the second step resulted in SQL error (as expected)
+			 *     Error: 001003 (42000): SQL compilation error:
+			 *         syntax error line 1 at position 86 unexpected '<EOF>'.
+			 * which matches the issue description exactly.
+			 */
+			{
+				ConfigDirectory: acc.ConfigurationSameAsStepN(1),
+				ConfigVariables: m2,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_dynamic_table.dt", "name", dynamicTableName),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckDynamicTableDestroy(s *terraform.State) error {
 	db := acc.TestAccProvider.Meta().(*sql.DB)
 	client := sdk.NewClientFromDB(db)
@@ -115,3 +252,17 @@ func testAccCheckDynamicTableDestroy(s *terraform.State) error {
 	}
 	return nil
 }
+
+func createDynamicTableOutsideTerraform(t *testing.T, schemaName string, dynamicTableName string, query string) {
+	t.Helper()
+	client, err := sdk.NewDefaultClient()
+	if err != nil {
+		t.Fatal(err)
+	}
+	ctx := context.Background()
+
+	dynamicTableId := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, schemaName, dynamicTableName)
+	if err := client.DynamicTables.Create(ctx, sdk.NewCreateDynamicTableRequest(dynamicTableId, sdk.NewAccountObjectIdentifier(acc.TestWarehouseName), sdk.TargetLag{MaximumDuration: sdk.String("2 minutes")}, query)); err != nil {
+		t.Fatal(fmt.Errorf("error creating dynamic table: %w", err))
+	}
+}