fix: Fix issues 2972 and 3007

MIGRATION_GUIDE.md

@@ -90,6 +90,30 @@ Some of the resources are excluded from this change:
 #### *(breaking change)* removed `qualified_name` from `snowflake_masking_policy`, `snowflake_network_rule`, `snowflake_password_policy` and `snowflake_table`
 Because of introducing a new `fully_qualified_name` field for all of the resources, `qualified_name` was removed from `snowflake_masking_policy`, `snowflake_network_rule`,  `snowflake_password_policy` and `snowflake_table`. Please adjust your configurations. State is automatically migrated.
 
+### snowflake_stage resource changes
+
+#### *(bugfix)* Correctly handle renamed/deleted stage
+
+Correctly handle the situation when stage was rename/deleted externally (earlier it resulted in a permanent loop). No action is required on the user's side.
+
+Connected issues: [#2972](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2972) 
+
+### snowflake_table resource changes
+
+#### *(bugfix)* Handle data type diff suppression better for text and number
+
+Data types are not entirely correctly handled inside the provider (read more e.g. in [#2735](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2735)). It will be still improved with the upcoming function, procedure, and table rework. Currently, diff suppression was fixed for text and number data types in the table resource with the following assumptions/limitations:
+- for numbers the default precision is 38 and the default scale is 0 (following the [docs](https://docs.snowflake.com/en/sql-reference/data-types-numeric#number))
+- for number types the following types are treated as synonyms: `NUMBER`, `DECIMAL`, `NUMERIC`, `INT`, `INTEGER`, `BIGINT`, `SMALLINT`, `TINYINT`, `BYTEINT`
+- for text the default length is 16777216 (following the [docs](https://docs.snowflake.com/en/sql-reference/data-types-text#varchar))
+- for text types the following types are treated as synonyms: `VARCHAR`, `CHAR`, `CHARACTER`, `STRING`, `TEXT`
+- whitespace and casing is ignored
+- if the type arguments cannot be parsed the defaults are used and therefore diff may be suppressed unexpectedly (please report such cases)
+
+No action is required on the user's side.
+
+Connected issues: [#3007](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3007)
+
 ### snowflake_user resource changes
 
 #### *(breaking change)* user parameters added to snowflake_user resource

pkg/acceptance/helpers/stage_client.go

@@ -128,3 +128,11 @@ func (c *StageClient) CopyIntoTableFromFile(t *testing.T, table, stage sdk.Schem
 	MATCH_BY_COLUMN_NAME = CASE_INSENSITIVE`, table.FullyQualifiedName(), stage.FullyQualifiedName(), filename))
 	require.NoError(t, err)
 }
+
+func (c *StageClient) Rename(t *testing.T, id sdk.SchemaObjectIdentifier, newId sdk.SchemaObjectIdentifier) {
+	t.Helper()
+	ctx := context.Background()
+
+	err := c.client().Alter(ctx, sdk.NewAlterStageRequest(id).WithRenameTo(&newId))
+	require.NoError(t, err)
+}

pkg/internal/util/strings.go

@@ -0,0 +1,12 @@
+package util
+
+import "strings"
+
+// TrimAllPrefixes removes all prefixes from the input. Order matters.
+func TrimAllPrefixes(text string, prefixes ...string) string {
+	result := text
+	for _, prefix := range prefixes {
+		result = strings.TrimPrefix(result, prefix)
+	}
+	return result
+}

pkg/internal/util/strings_test.go

@@ -0,0 +1,34 @@
+package util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_TrimAllPrefixes(t *testing.T) {
+	type test struct {
+		input    string
+		prefixes []string
+		expected string
+	}
+
+	tests := []test{
+		{input: "VARCHAR(30)", prefixes: []string{"VARCHAR", "TEXT"}, expected: "(30)"},
+		{input: "VARCHAR  (30) ", prefixes: []string{"VARCHAR", "TEXT"}, expected: "  (30) "},
+		{input: "VARCHAR(30)", prefixes: []string{"VARCHAR"}, expected: "(30)"},
+		{input: "VARCHAR(30)", prefixes: []string{}, expected: "VARCHAR(30)"},
+		{input: "VARCHARVARCHAR(30)", prefixes: []string{"VARCHAR"}, expected: "VARCHAR(30)"},
+		{input: "VARCHAR(30)", prefixes: []string{"NUMBER"}, expected: "VARCHAR(30)"},
+		{input: "VARCHARTEXT(30)", prefixes: []string{"VARCHAR", "TEXT"}, expected: "(30)"},
+		{input: "TEXTVARCHAR(30)", prefixes: []string{"VARCHAR", "TEXT"}, expected: "VARCHAR(30)"},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.input, func(t *testing.T) {
+			output := TrimAllPrefixes(tc.input, tc.prefixes...)
+			require.Equal(t, tc.expected, output)
+		})
+	}
+}

pkg/resources/helpers.go

@@ -5,10 +5,10 @@ import (
 	"slices"
 	"strings"
 
-	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
-
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/logging"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/snowflake"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
@@ -35,6 +35,39 @@ func dataTypeDiffSuppressFunc(_, old, new string, _ *schema.ResourceData) bool {
 	return oldDT == newDT
 }
 
+// DataTypeIssue3007DiffSuppressFunc is a temporary solution to handle data type suppression problems.
+// Currently, it handles only number and text data types.
+// It falls back to Snowflake defaults for arguments if no arguments were provided for the data type.
+// TODO [SNOW-1348103 or SNOW-1348106]: visit with functions and procedures rework
+func DataTypeIssue3007DiffSuppressFunc(_, old, new string, _ *schema.ResourceData) bool {
+	oldDataType, err := sdk.ToDataType(old)
+	if err != nil {
+		return false
+	}
+	newDataType, err := sdk.ToDataType(new)
+	if err != nil {
+		return false
+	}
+	if oldDataType != newDataType {
+		return false
+	}
+	switch v := oldDataType; v {
+	case sdk.DataTypeNumber:
+		logging.DebugLogger.Printf("[DEBUG] DataTypeIssue3007DiffSuppressFunc: Handling number data type diff suppression")
+		oldPrecision, oldScale := sdk.ParseNumberDataTypeRaw(old)
+		newPrecision, newScale := sdk.ParseNumberDataTypeRaw(new)
+		return oldPrecision == newPrecision && oldScale == newScale
+	case sdk.DataTypeVARCHAR:
+		logging.DebugLogger.Printf("[DEBUG] DataTypeIssue3007DiffSuppressFunc: Handling text data type diff suppression")
+		oldLength := sdk.ParseVarcharDataTypeRaw(old)
+		newLength := sdk.ParseVarcharDataTypeRaw(new)
+		return oldLength == newLength
+	default:
+		logging.DebugLogger.Printf("[DEBUG] DataTypeIssue3007DiffSuppressFunc: Diff suppression for %s can't be currently handled", v)
+	}
+	return true
+}
+
 func ignoreTrimSpaceSuppressFunc(_, old, new string, _ *schema.ResourceData) bool {
 	return strings.TrimSpace(old) == strings.TrimSpace(new)
 }

pkg/resources/helpers_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_GetPropertyAsPointer(t *testing.T) {
@@ -258,3 +259,147 @@ func TestListDiff(t *testing.T) {
 		})
 	}
 }
+
+func Test_DataTypeIssue3007DiffSuppressFunc(t *testing.T) {
+	testCases := []struct {
+		name     string
+		old      string
+		new      string
+		expected bool
+	}{
+		{
+			name:     "different data type",
+			old:      string(sdk.DataTypeVARCHAR),
+			new:      string(sdk.DataTypeNumber),
+			expected: false,
+		},
+		{
+			name:     "same number data type without arguments",
+			old:      string(sdk.DataTypeNumber),
+			new:      string(sdk.DataTypeNumber),
+			expected: true,
+		},
+		{
+			name:     "same number data type different casing",
+			old:      string(sdk.DataTypeNumber),
+			new:      "number",
+			expected: true,
+		},
+		{
+			name:     "same text data type without arguments",
+			old:      string(sdk.DataTypeVARCHAR),
+			new:      string(sdk.DataTypeVARCHAR),
+			expected: true,
+		},
+		{
+			name:     "same other data type",
+			old:      string(sdk.DataTypeFloat),
+			new:      string(sdk.DataTypeFloat),
+			expected: true,
+		},
+		{
+			name:     "synonym number data type without arguments",
+			old:      string(sdk.DataTypeNumber),
+			new:      "DECIMAL",
+			expected: true,
+		},
+		{
+			name:     "synonym text data type without arguments",
+			old:      string(sdk.DataTypeVARCHAR),
+			new:      "TEXT",
+			expected: true,
+		},
+		{
+			name:     "synonym other data type without arguments",
+			old:      string(sdk.DataTypeFloat),
+			new:      "DOUBLE",
+			expected: true,
+		},
+		{
+			name:     "synonym number data type same precision, no scale",
+			old:      "NUMBER(30)",
+			new:      "DECIMAL(30)",
+			expected: true,
+		},
+		{
+			name:     "synonym number data type precision implicit and same",
+			old:      "NUMBER",
+			new:      fmt.Sprintf("DECIMAL(%d)", sdk.DefaultNumberPrecision),
+			expected: true,
+		},
+		{
+			name:     "synonym number data type precision implicit and different",
+			old:      "NUMBER",
+			new:      "DECIMAL(30)",
+			expected: false,
+		},
+		{
+			name:     "number data type different precisions, no scale",
+			old:      "NUMBER(35)",
+			new:      "NUMBER(30)",
+			expected: false,
+		},
+		{
+			name:     "synonym number data type same precision, different scale",
+			old:      "NUMBER(30, 2)",
+			new:      "DECIMAL(30, 1)",
+			expected: false,
+		},
+		{
+			name:     "synonym number data type default scale implicit and explicit",
+			old:      "NUMBER(30)",
+			new:      fmt.Sprintf("DECIMAL(30, %d)", sdk.DefaultNumberScale),
+			expected: true,
+		},
+		{
+			name:     "synonym number data type default scale implicit and different",
+			old:      "NUMBER(30)",
+			new:      "DECIMAL(30, 3)",
+			expected: false,
+		},
+		{
+			name:     "synonym number data type both precision and scale implicit and explicit",
+			old:      "NUMBER",
+			new:      fmt.Sprintf("DECIMAL(%d, %d)", sdk.DefaultNumberPrecision, sdk.DefaultNumberScale),
+			expected: true,
+		},
+		{
+			name:     "synonym number data type both precision and scale implicit and scale different",
+			old:      "NUMBER",
+			new:      fmt.Sprintf("DECIMAL(%d, 2)", sdk.DefaultNumberPrecision),
+			expected: false,
+		},
+		{
+			name:     "synonym text data type same length",
+			old:      "VARCHAR(30)",
+			new:      "TEXT(30)",
+			expected: true,
+		},
+		{
+			name:     "synonym text data type different length",
+			old:      "VARCHAR(30)",
+			new:      "TEXT(40)",
+			expected: false,
+		},
+		{
+			name:     "synonym text data type length implicit and same",
+			old:      "VARCHAR",
+			new:      fmt.Sprintf("TEXT(%d)", sdk.DefaultVarcharLength),
+			expected: true,
+		},
+		{
+			name:     "synonym text data type length implicit and different",
+			old:      "VARCHAR",
+			new:      "TEXT(40)",
+			expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			result := resources.DataTypeIssue3007DiffSuppressFunc("", tc.old, tc.new, nil)
+			require.Equal(t, tc.expected, result)
+		})
+	}
+}

pkg/resources/stage.go

@@ -2,6 +2,7 @@ package resources
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -174,14 +175,17 @@ func ReadStage(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagn
 
 	properties, err := client.Stages.Describe(ctx, id)
 	if err != nil {
-		d.SetId("")
-		return diag.Diagnostics{
-			diag.Diagnostic{
-				Severity: diag.Error,
-				Summary:  "Failed to describe stage",
-				Detail:   fmt.Sprintf("Id: %s, Err: %s", d.Id(), err),
-			},
+		if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) {
+			d.SetId("")
+			return diag.Diagnostics{
+				diag.Diagnostic{
+					Severity: diag.Warning,
+					Summary:  "Failed to describe stage. Marking the resource as removed.",
+					Detail:   fmt.Sprintf("Stage: %s, Err: %s", id.FullyQualifiedName(), err),
+				},
+			}
 		}
+		return diag.FromErr(err)
 	}
 
 	stage, err := client.Stages.ShowByID(ctx, id)
@@ -191,7 +195,7 @@ func ReadStage(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagn
 			diag.Diagnostic{
 				Severity: diag.Error,
 				Summary:  "Failed to show stage by id",
-				Detail:   fmt.Sprintf("Id: %s, Err: %s", d.Id(), err),
+				Detail:   fmt.Sprintf("Stage: %s, Err: %s", id.FullyQualifiedName(), err),
 			},
 		}
 	}

pkg/resources/stage_acceptance_test.go

@@ -204,3 +204,54 @@ resource "snowflake_stage" "test" {
 }
 `, name, siNameSuffix, url, databaseName, schemaName)
 }
+
+func TestAcc_Stage_Issue2972(t *testing.T) {
+	stageId := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
+	newId := acc.TestClient().Ids.RandomSchemaObjectIdentifierInSchema(stageId.SchemaId())
+	resourceName := "snowflake_stage.test"
+
+	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.Stage),
+		Steps: []resource.TestStep{
+			{
+				Config: stageIssue2972Config(stageId),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "database", stageId.DatabaseName()),
+					resource.TestCheckResourceAttr(resourceName, "schema", stageId.SchemaName()),
+					resource.TestCheckResourceAttr(resourceName, "name", stageId.Name()),
+				),
+			},
+			{
+				PreConfig: func() {
+					acc.TestClient().Stage.Rename(t, stageId, newId)
+				},
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate),
+					},
+				},
+				Config: stageIssue2972Config(stageId),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "database", stageId.DatabaseName()),
+					resource.TestCheckResourceAttr(resourceName, "schema", stageId.SchemaName()),
+					resource.TestCheckResourceAttr(resourceName, "name", stageId.Name()),
+				),
+			},
+		},
+	})
+}
+
+func stageIssue2972Config(stageId sdk.SchemaObjectIdentifier) string {
+	return fmt.Sprintf(`
+resource "snowflake_stage" "test" {
+	name = "%[1]s"
+	database = "%[2]s"
+	schema = "%[3]s"
+}
+`, stageId.Name(), stageId.DatabaseName(), stageId.SchemaName())
+}