fix: data retention time parameters

MIGRATION_GUIDE.md

@@ -10,6 +10,11 @@ across different versions.
 Longer context in [#2517](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2517).
 After this change, one apply may be required to update the state correctly for failover group resources using `ACCOUNT PARAMETERS`.
 
+### snowflake_schema resource changes
+#### *(behavior change)* Schema `data_retention_days`
+To make `snowflake_schema.data_retention_days` truly optional field (previously it was producing plan every time when no value was set),
+we added `-1` as a possible value as set it as default. That got rid of the unexpected plans when no value is set and added possibility to use default value assigned by Snowflake (see [the data retention period](https://docs.snowflake.com/en/user-guide/data-time-travel#data-retention-period)).
+
 ## v0.85.0 ➞ v0.86.0
 ### snowflake_table_constraint resource changes
 

docs/resources/schema.md

@@ -35,7 +35,7 @@ resource "snowflake_schema" "schema" {
 ### Optional
 
 - `comment` (String) Specifies a comment for the schema.
-- `data_retention_days` (Number) Specifies the number of days for which Time Travel actions (CLONE and UNDROP) can be performed on the schema, as well as specifying the default Time Travel retention time for all tables created in the schema.
+- `data_retention_days` (Number) Specifies the number of days for which Time Travel actions (CLONE and UNDROP) can be performed on the schema, as well as specifying the default Time Travel retention time for all tables created in the schema. Default value for this field is set to -1, which is a fallback to use Snowflake default.
 - `is_managed` (Boolean) Specifies a managed schema. Managed access schemas centralize privilege management with the schema owner.
 - `is_transient` (Boolean) Specifies a schema as transient. Transient schemas do not have a Fail-safe period so they do not incur additional storage costs once they leave Time Travel; however, this means they are also not protected by Fail-safe in the event of a data loss.
 - `tag` (Block List, Deprecated) Definitions of a tag to associate with the resource. (see [below for nested schema](#nestedblock--tag))

pkg/resources/schema.go

@@ -53,9 +53,9 @@ var schemaSchema = map[string]*schema.Schema{
 	"data_retention_days": {
 		Type:         schema.TypeInt,
 		Optional:     true,
-		Default:      1,
-		Description:  "Specifies the number of days for which Time Travel actions (CLONE and UNDROP) can be performed on the schema, as well as specifying the default Time Travel retention time for all tables created in the schema.",
-		ValidateFunc: validation.IntBetween(0, 90),
+		Default:      -1,
+		Description:  "Specifies the number of days for which Time Travel actions (CLONE and UNDROP) can be performed on the schema, as well as specifying the default Time Travel retention time for all tables created in the schema. Default value for this field is set to -1, which is a fallback to use Snowflake default.",
+		ValidateFunc: validation.IntBetween(-1, 90),
 	},
 	"tag": tagReferenceSchema,
 }
@@ -67,7 +67,6 @@ func Schema() *schema.Resource {
 		Read:   ReadSchema,
 		Update: UpdateSchema,
 		Delete: DeleteSchema,
-
 		Schema: schemaSchema,
 		Importer: &schema.ResourceImporter{
 			StateContext: schema.ImportStatePassthroughContext,
@@ -84,13 +83,19 @@ func CreateSchema(d *schema.ResourceData, meta interface{}) error {
 	client := sdk.NewClientFromDB(db)
 	ctx := context.Background()
 
-	err := client.Schemas.Create(ctx, sdk.NewDatabaseObjectIdentifier(database, name), &sdk.CreateSchemaOptions{
-		Transient:               GetPropertyAsPointer[bool](d, "is_transient"),
-		WithManagedAccess:       GetPropertyAsPointer[bool](d, "is_managed"),
-		DataRetentionTimeInDays: GetPropertyAsPointer[int](d, "data_retention_days"),
-		Tag:                     getPropertyTags(d, "tag"),
-		Comment:                 GetPropertyAsPointer[string](d, "comment"),
-	})
+	createReq := &sdk.CreateSchemaOptions{
+		Transient:         GetPropertyAsPointer[bool](d, "is_transient"),
+		WithManagedAccess: GetPropertyAsPointer[bool](d, "is_managed"),
+		Tag:               getPropertyTags(d, "tag"),
+		Comment:           GetPropertyAsPointer[string](d, "comment"),
+	}
+
+	dataRetentionTimeInDays := GetPropertyAsPointer[int](d, "data_retention_days")
+	if dataRetentionTimeInDays != nil && *dataRetentionTimeInDays != -1 {
+		createReq.DataRetentionTimeInDays = dataRetentionTimeInDays
+	}
+
+	err := client.Schemas.Create(ctx, sdk.NewDatabaseObjectIdentifier(database, name), createReq)
 	if err != nil {
 		return fmt.Errorf("error creating schema %v err = %w", name, err)
 	}
@@ -107,7 +112,7 @@ func ReadSchema(d *schema.ResourceData, meta interface{}) error {
 	ctx := context.Background()
 	id := helpers.DecodeSnowflakeID(d.Id()).(sdk.DatabaseObjectIdentifier)
 
-	_, err := client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id.DatabaseName()))
+	database, err := client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id.DatabaseName()))
 	if err != nil {
 		d.SetId("")
 	}
@@ -133,10 +138,15 @@ func ReadSchema(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
+	if dataRetentionDays := d.Get("data_retention_days"); dataRetentionDays.(int) != -1 || int64(database.RetentionTime) != retentionTime {
+		if err := d.Set("data_retention_days", retentionTime); err != nil {
+			return err
+		}
+	}
+
 	values := map[string]any{
-		"name":                s.Name,
-		"database":            s.DatabaseName,
-		"data_retention_days": retentionTime,
+		"name":     s.Name,
+		"database": s.DatabaseName,
 		// reset the options before reading back from the DB
 		"is_transient": false,
 		"is_managed":   false,
@@ -217,14 +227,24 @@ func UpdateSchema(d *schema.ResourceData, meta interface{}) error {
 	}
 
 	if d.HasChange("data_retention_days") {
-		days := d.Get("data_retention_days")
-		err := client.Schemas.Alter(ctx, id, &sdk.AlterSchemaOptions{
-			Set: &sdk.SchemaSet{
-				DataRetentionTimeInDays: sdk.Int(days.(int)),
-			},
-		})
-		if err != nil {
-			return fmt.Errorf("error updating data retention days on %v err = %w", d.Id(), err)
+		if days := d.Get("data_retention_days"); days.(int) != -1 {
+			err := client.Schemas.Alter(ctx, id, &sdk.AlterSchemaOptions{
+				Set: &sdk.SchemaSet{
+					DataRetentionTimeInDays: sdk.Int(days.(int)),
+				},
+			})
+			if err != nil {
+				return fmt.Errorf("error setting data retention days on %v err = %w", d.Id(), err)
+			}
+		} else {
+			err := client.Schemas.Alter(ctx, id, &sdk.AlterSchemaOptions{
+				Unset: &sdk.SchemaUnset{
+					DataRetentionTimeInDays: sdk.Bool(true),
+				},
+			})
+			if err != nil {
+				return fmt.Errorf("error unsetting data retention days on %v err = %w", d.Id(), err)
+			}
 		}
 	}
 

pkg/resources/schema_acceptance_test.go

@@ -4,9 +4,13 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"strconv"
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/terraform-plugin-testing/plancheck"
+	"github.com/stretchr/testify/require"
+
 	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-testing/config"
@@ -137,6 +141,214 @@ func TestAcc_Schema_TwoSchemasWithTheSameNameOnDifferentDatabases(t *testing.T)
 	})
 }
 
+// proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2356 issue is fixed.
+func TestAcc_Schema_DefaultDataRetentionTime(t *testing.T) {
+	databaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	schemaName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	id := sdk.NewDatabaseObjectIdentifier(databaseName, schemaName)
+
+	configVariablesWithoutSchemaDataRetentionTime := func(databaseDataRetentionTime int) config.Variables {
+		return config.Variables{
+			"database":                     config.StringVariable(databaseName),
+			"schema":                       config.StringVariable(schemaName),
+			"database_data_retention_time": config.IntegerVariable(databaseDataRetentionTime),
+		}
+	}
+
+	configVariablesWithSchemaDataRetentionTime := func(databaseDataRetentionTime int, schemaDataRetentionTime int) config.Variables {
+		vars := configVariablesWithoutSchemaDataRetentionTime(databaseDataRetentionTime)
+		vars["schema_data_retention_time"] = config.IntegerVariable(schemaDataRetentionTime)
+		return vars
+	}
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckSchemaDestroy,
+		Steps: []resource.TestStep{
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema"),
+				ConfigVariables: configVariablesWithoutSchemaDataRetentionTime(5),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "-1"),
+					checkDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema"),
+				ConfigVariables: configVariablesWithoutSchemaDataRetentionTime(10),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "-1"),
+					checkDataRetentionTime(id, 10, 10),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithSchema"),
+				ConfigVariables: configVariablesWithSchemaDataRetentionTime(10, 5),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "5"),
+					checkDataRetentionTime(id, 10, 5),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithSchema"),
+				ConfigVariables: configVariablesWithSchemaDataRetentionTime(10, 15),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "15"),
+					checkDataRetentionTime(id, 10, 15),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema"),
+				ConfigVariables: configVariablesWithoutSchemaDataRetentionTime(10),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "-1"),
+					checkDataRetentionTime(id, 10, 10),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithSchema"),
+				ConfigVariables: configVariablesWithSchemaDataRetentionTime(10, 0),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "0"),
+					checkDataRetentionTime(id, 10, 0),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithSchema"),
+				ConfigVariables: configVariablesWithSchemaDataRetentionTime(10, 3),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "3"),
+					checkDataRetentionTime(id, 10, 3),
+				),
+			},
+		},
+	})
+}
+
+func TestAcc_Schema_DefaultDataRetentionTime_SetOutsideOfTerraform(t *testing.T) {
+	databaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	schemaName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	id := sdk.NewDatabaseObjectIdentifier(databaseName, schemaName)
+
+	configVariablesWithoutSchemaDataRetentionTime := func(databaseDataRetentionTime int) config.Variables {
+		return config.Variables{
+			"database":                     config.StringVariable(databaseName),
+			"schema":                       config.StringVariable(schemaName),
+			"database_data_retention_time": config.IntegerVariable(databaseDataRetentionTime),
+		}
+	}
+
+	configVariablesWithSchemaDataRetentionTime := func(databaseDataRetentionTime int, schemaDataRetentionTime int) config.Variables {
+		vars := configVariablesWithoutSchemaDataRetentionTime(databaseDataRetentionTime)
+		vars["schema_data_retention_time"] = config.IntegerVariable(schemaDataRetentionTime)
+		return vars
+	}
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckSchemaDestroy,
+		Steps: []resource.TestStep{
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema"),
+				ConfigVariables: configVariablesWithoutSchemaDataRetentionTime(5),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "-1"),
+					checkDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				PreConfig:       setSchemaDataRetentionTime(t, id, 20),
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema"),
+				ConfigVariables: configVariablesWithoutSchemaDataRetentionTime(5),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "-1"),
+					checkDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Schema_DefaultDataRetentionTime/WithSchema"),
+				ConfigVariables: configVariablesWithSchemaDataRetentionTime(10, 3),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_schema.test", "data_retention_days", "3"),
+					checkDataRetentionTime(id, 10, 3),
+				),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PostApplyPostRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectEmptyPlan(),
+					},
+				},
+			},
+		},
+	})
+}
+
+func checkDataRetentionTime(id sdk.DatabaseObjectIdentifier, expectedDatabaseRetentionsDays int, expectedSchemaRetentionDays int) func(state *terraform.State) error {
+	return func(state *terraform.State) error {
+		db := acc.TestAccProvider.Meta().(*sql.DB)
+		client := sdk.NewClientFromDB(db)
+		ctx := context.Background()
+
+		schema, err := client.Schemas.ShowByID(ctx, id)
+		if err != nil {
+			return err
+		}
+
+		database, err := client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id.DatabaseName()))
+		if err != nil {
+			return err
+		}
+
+		// "retention_time" may sometimes be an empty string instead of an integer
+		var schemaRetentionTime int64
+		{
+			rt := schema.RetentionTime
+			if rt == "" {
+				rt = "0"
+			}
+
+			schemaRetentionTime, err = strconv.ParseInt(rt, 10, 64)
+			if err != nil {
+				return err
+			}
+		}
+
+		if database.RetentionTime != expectedDatabaseRetentionsDays {
+			return fmt.Errorf("invalid database retention time, expected: %d, got: %d", expectedDatabaseRetentionsDays, database.RetentionTime)
+		}
+
+		if schemaRetentionTime != int64(expectedSchemaRetentionDays) {
+			return fmt.Errorf("invalid schema retention time, expected: %d, got: %d", expectedSchemaRetentionDays, schemaRetentionTime)
+		}
+
+		return nil
+	}
+}
+
+func setSchemaDataRetentionTime(t *testing.T, id sdk.DatabaseObjectIdentifier, days int) func() {
+	t.Helper()
+
+	return func() {
+		client, err := sdk.NewDefaultClient()
+		require.NoError(t, err)
+		ctx := context.Background()
+
+		err = client.Schemas.Alter(ctx, id, &sdk.AlterSchemaOptions{
+			Set: &sdk.SchemaSet{
+				DataRetentionTimeInDays: sdk.Int(days),
+			},
+		})
+		require.NoError(t, err)
+	}
+}
+
 func testAccCheckSchemaDestroy(s *terraform.State) error {
 	db := acc.TestAccProvider.Meta().(*sql.DB)
 	client := sdk.NewClientFromDB(db)

pkg/resources/testdata/TestAcc_Schema_DefaultDataRetentionTime/WithSchema/test.tf

@@ -0,0 +1,10 @@
+resource "snowflake_database" "test" {
+  name                        = var.database
+  data_retention_time_in_days = var.database_data_retention_time
+}
+
+resource "snowflake_schema" "test" {
+  name                = var.schema
+  database            = snowflake_database.test.name
+  data_retention_days = var.schema_data_retention_time
+}

pkg/resources/testdata/TestAcc_Schema_DefaultDataRetentionTime/WithSchema/variables.tf

@@ -0,0 +1,15 @@
+variable "database" {
+  type = string
+}
+
+variable "schema" {
+  type = string
+}
+
+variable "database_data_retention_time" {
+  type = number
+}
+
+variable "schema_data_retention_time" {
+  type = number
+}

pkg/resources/testdata/TestAcc_Schema_DefaultDataRetentionTime/WithoutSchema/test.tf

@@ -0,0 +1,9 @@
+resource "snowflake_database" "test" {
+  name                        = var.database
+  data_retention_time_in_days = var.database_data_retention_time
+}
+
+resource "snowflake_schema" "test" {
+  name     = var.schema
+  database = snowflake_database.test.name
+}