fix: data retention time parameters follow-up (#2530)

<!-- Feel free to delete comments as you fill this in -->
A follow-up for
#2502.
In this pr I applied the same changes to the Database and Table
resource. On the note side, I left some comments in the table resource.
There's still a deprecated data retention parameter that I would remove
because it wouldn't be used with the new implementation that always
expects a value (-1 if not set). We can add a note in the migration note
and change the description of the deprecated parameter stating that it
doesn't have any effect on the table configuration. Also in the database
tests I'm changing account-level data retention time which I think
shouldn't (but may) affect other tests. I would leave it anyway, because
on delete we set back the default value and crashes may occur, but very
rarely (I think).

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] acceptance tests
<!-- add more below if you think they are relevant -->

MIGRATION_GUIDE.md

@@ -26,11 +26,15 @@ The Snowflake provider will use the following order of precedence when determini
 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),
+### snowflake_database, snowflake_schema, and snowflake_table resource changes
+#### *(behavior change)* Database `data_retention_time_in_days` + Schema `data_retention_days` + Table `data_retention_time_in_days`
+To make data retention fields truly optional (previously they were 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)).
 
+### snowflake_table resource changes
+#### *(behavior change)* Table `data_retention_days` field removed in favor of `data_retention_time_in_days`
+To define data retention days for table `data_retention_time_in_days` should be used as deprecated `data_retention_days` field is being removed.
+
 ## v0.85.0 ➞ v0.86.0
 ### snowflake_table_constraint resource changes
 

docs/resources/database.md

@@ -55,7 +55,7 @@ resource "snowflake_database" "from_share" {
 ### Optional
 
 - `comment` (String)
-- `data_retention_time_in_days` (Number) Number of days for which Snowflake retains historical data for performing Time Travel actions (SELECT, CLONE, UNDROP) on the object. A value of 0 effectively disables Time Travel for the specified database, schema, or table. For more information, see Understanding & Using Time Travel.
+- `data_retention_time_in_days` (Number) Number of days for which Snowflake retains historical data for performing Time Travel actions (SELECT, CLONE, UNDROP) on the object. A value of 0 effectively disables Time Travel for the specified database. Default value for this field is set to -1, which is a fallback to use Snowflake default. For more information, see Understanding & Using Time Travel.
 - `from_database` (String) Specify a database to create a clone from.
 - `from_replica` (String) Specify a fully-qualified path to a database to create a replica from. A fully qualified path follows the format of "<organization_name>"."<account_name>"."<db_name>". An example would be: "myorg1"."account1"."db1"
 - `from_share` (Map of String) Specify a provider and a share in this map to create a database from a share.

docs/resources/table.md

@@ -95,8 +95,7 @@ resource "snowflake_table" "table" {
 - `change_tracking` (Boolean) Specifies whether to enable change tracking on the table. Default false.
 - `cluster_by` (List of String) A list of one or more table columns/expressions to be used as clustering key(s) for the table
 - `comment` (String) Specifies a comment for the table.
-- `data_retention_days` (Number, Deprecated) Specifies the retention period for the table so that Time Travel actions (SELECT, CLONE, UNDROP) can be performed on historical data in the table. Default value is 1, if you wish to inherit the parent schema setting then pass in the schema attribute to this argument.
-- `data_retention_time_in_days` (Number) Specifies the retention period for the table so that Time Travel actions (SELECT, CLONE, UNDROP) can be performed on historical data in the table. Default value is 1, if you wish to inherit the parent schema setting then pass in the schema attribute to this argument.
+- `data_retention_time_in_days` (Number) Specifies the retention period for the table so that Time Travel actions (SELECT, CLONE, UNDROP) can be performed on historical data in the table. If you wish to inherit the parent schema setting then pass in the schema attribute to this argument or do not fill this parameter at all; the default value for this field is -1, which is a fallback to use Snowflake default - in this case the schema value
 - `primary_key` (Block List, Max: 1, Deprecated) Definitions of primary key constraint to create on table (see [below for nested schema](#nestedblock--primary_key))
 - `tag` (Block List, Deprecated) Definitions of a tag to associate with the resource. (see [below for nested schema](#nestedblock--tag))
 

pkg/resources/database.go

@@ -4,7 +4,11 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"log"
 	"slices"
+	"strconv"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -28,10 +32,11 @@ var databaseSchema = map[string]*schema.Schema{
 		ForceNew:    true,
 	},
 	"data_retention_time_in_days": {
-		Type:        schema.TypeInt,
-		Optional:    true,
-		Description: "Number of days for which Snowflake retains historical data for performing Time Travel actions (SELECT, CLONE, UNDROP) on the object. A value of 0 effectively disables Time Travel for the specified database, schema, or table. For more information, see Understanding & Using Time Travel.",
-		Default:     1,
+		Type:         schema.TypeInt,
+		Optional:     true,
+		Default:      -1,
+		Description:  "Number of days for which Snowflake retains historical data for performing Time Travel actions (SELECT, CLONE, UNDROP) on the object. A value of 0 effectively disables Time Travel for the specified database. Default value for this field is set to -1, which is a fallback to use Snowflake default. For more information, see Understanding & Using Time Travel.",
+		ValidateFunc: validation.IntBetween(-1, 90),
 	},
 	"from_share": {
 		Type:          schema.TypeMap,
@@ -120,8 +125,9 @@ func CreateDatabase(d *schema.ResourceData, meta interface{}) error {
 	// Is it a Secondary Database?
 	if primaryName, ok := d.GetOk("from_replica"); ok {
 		primaryID := sdk.NewExternalObjectIdentifierFromFullyQualifiedName(primaryName.(string))
-		opts := &sdk.CreateSecondaryDatabaseOptions{
-			DataRetentionTimeInDays: sdk.Int(d.Get("data_retention_time_in_days").(int)),
+		opts := &sdk.CreateSecondaryDatabaseOptions{}
+		if v := d.Get("data_retention_time_in_days"); v.(int) != -1 {
+			opts.DataRetentionTimeInDays = sdk.Int(v.(int))
 		}
 		err := client.Databases.CreateSecondary(ctx, id, primaryID, opts)
 		if err != nil {
@@ -148,7 +154,7 @@ func CreateDatabase(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
-	if v, ok := d.GetOk("data_retention_time_in_days"); ok {
+	if v := d.Get("data_retention_time_in_days"); v.(int) != -1 {
 		opts.DataRetentionTimeInDays = sdk.Int(v.(int))
 	}
 
@@ -192,6 +198,7 @@ func ReadDatabase(d *schema.ResourceData, meta interface{}) error {
 	database, err := client.Databases.ShowByID(ctx, id)
 	if err != nil {
 		d.SetId("")
+		log.Printf("Database %s not found, err = %s", name, err)
 		return nil
 	}
 
@@ -202,10 +209,21 @@ func ReadDatabase(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	if err := d.Set("data_retention_time_in_days", database.RetentionTime); err != nil {
+	dataRetention, err := client.Parameters.ShowAccountParameter(ctx, sdk.AccountParameterDataRetentionTimeInDays)
+	if err != nil {
+		return err
+	}
+	paramDataRetention, err := strconv.Atoi(dataRetention.Value)
+	if err != nil {
 		return err
 	}
 
+	if dataRetentionDays := d.Get("data_retention_time_in_days"); dataRetentionDays.(int) != -1 || database.RetentionTime != paramDataRetention {
+		if err := d.Set("data_retention_time_in_days", database.RetentionTime); err != nil {
+			return err
+		}
+	}
+
 	if err := d.Set("is_transient", database.Transient); err != nil {
 		return err
 	}
@@ -250,15 +268,24 @@ func UpdateDatabase(d *schema.ResourceData, meta interface{}) error {
 	}
 
 	if d.HasChange("data_retention_time_in_days") {
-		days := d.Get("data_retention_time_in_days").(int)
-		opts := &sdk.AlterDatabaseOptions{
-			Set: &sdk.DatabaseSet{
-				DataRetentionTimeInDays: sdk.Int(days),
-			},
-		}
-		err := client.Databases.Alter(ctx, id, opts)
-		if err != nil {
-			return fmt.Errorf("error updating database data retention time on %v err = %w", d.Id(), err)
+		if days := d.Get("data_retention_time_in_days"); days.(int) != -1 {
+			err := client.Databases.Alter(ctx, id, &sdk.AlterDatabaseOptions{
+				Set: &sdk.DatabaseSet{
+					DataRetentionTimeInDays: sdk.Int(days.(int)),
+				},
+			})
+			if err != nil {
+				return fmt.Errorf("error when setting database data retention time on %v err = %w", d.Id(), err)
+			}
+		} else {
+			err := client.Databases.Alter(ctx, id, &sdk.AlterDatabaseOptions{
+				Unset: &sdk.DatabaseUnset{
+					DataRetentionTimeInDays: sdk.Bool(true),
+				},
+			})
+			if err != nil {
+				return fmt.Errorf("error when usetting database data retention time on %v err = %w", d.Id(), err)
+			}
 		}
 	}
 

pkg/resources/database_acceptance_test.go

@@ -2,7 +2,9 @@ package resources_test
 
 import (
 	"context"
+	"database/sql"
 	"fmt"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -178,6 +180,167 @@ func TestAcc_Database_issue2021(t *testing.T) {
 	})
 }
 
+// proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2356 issue is fixed.
+func TestAcc_Database_DefaultDataRetentionTime(t *testing.T) {
+	databaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	id := sdk.NewAccountObjectIdentifier(databaseName)
+
+	configVariablesWithoutDatabaseDataRetentionTime := func() config.Variables {
+		return config.Variables{
+			"database": config.StringVariable(databaseName),
+		}
+	}
+
+	configVariablesWithDatabaseDataRetentionTime := func(databaseDataRetentionTime int) config.Variables {
+		vars := configVariablesWithoutDatabaseDataRetentionTime()
+		vars["database_data_retention_time"] = config.IntegerVariable(databaseDataRetentionTime)
+		return vars
+	}
+
+	client, err := sdk.NewDefaultClient()
+	require.NoError(t, err)
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: nil,
+		Steps: []resource.TestStep{
+			{
+				PreConfig:       updateAccountParameter(t, client, sdk.AccountParameterDataRetentionTimeInDays, true, "5"),
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithoutDataRetentionSet"),
+				ConfigVariables: configVariablesWithoutDatabaseDataRetentionTime(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "-1"),
+					checkAccountAndDatabaseDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				PreConfig:       updateAccountParameter(t, client, sdk.AccountParameterDataRetentionTimeInDays, false, "10"),
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithoutDataRetentionSet"),
+				ConfigVariables: configVariablesWithoutDatabaseDataRetentionTime(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "-1"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 10),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithDataRetentionSet"),
+				ConfigVariables: configVariablesWithDatabaseDataRetentionTime(5),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "5"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 5),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithDataRetentionSet"),
+				ConfigVariables: configVariablesWithDatabaseDataRetentionTime(15),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "15"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 15),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithoutDataRetentionSet"),
+				ConfigVariables: configVariablesWithoutDatabaseDataRetentionTime(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "-1"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 10),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithDataRetentionSet"),
+				ConfigVariables: configVariablesWithDatabaseDataRetentionTime(0),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "0"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 0),
+				),
+			},
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithDataRetentionSet"),
+				ConfigVariables: configVariablesWithDatabaseDataRetentionTime(3),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "3"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 3),
+				),
+			},
+		},
+	})
+}
+
+// proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2356 issue is fixed.
+func TestAcc_Database_DefaultDataRetentionTime_SetOutsideOfTerraform(t *testing.T) {
+	databaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
+	id := sdk.NewAccountObjectIdentifier(databaseName)
+
+	configVariablesWithoutDatabaseDataRetentionTime := func() config.Variables {
+		return config.Variables{
+			"database": config.StringVariable(databaseName),
+		}
+	}
+
+	configVariablesWithDatabaseDataRetentionTime := func(databaseDataRetentionTime int) config.Variables {
+		vars := configVariablesWithoutDatabaseDataRetentionTime()
+		vars["database_data_retention_time"] = config.IntegerVariable(databaseDataRetentionTime)
+		return vars
+	}
+
+	client, err := sdk.NewDefaultClient()
+	require.NoError(t, err)
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: nil,
+		Steps: []resource.TestStep{
+			{
+				PreConfig:       updateAccountParameter(t, client, sdk.AccountParameterDataRetentionTimeInDays, true, "5"),
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithoutDataRetentionSet"),
+				ConfigVariables: configVariablesWithoutDatabaseDataRetentionTime(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "-1"),
+					checkAccountAndDatabaseDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				PreConfig: func() {
+					err = client.Databases.Alter(context.Background(), id, &sdk.AlterDatabaseOptions{
+						Set: &sdk.DatabaseSet{
+							DataRetentionTimeInDays: sdk.Int(20),
+						},
+					})
+					require.NoError(t, err)
+				},
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithoutDataRetentionSet"),
+				ConfigVariables: configVariablesWithoutDatabaseDataRetentionTime(),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "-1"),
+					checkAccountAndDatabaseDataRetentionTime(id, 5, 5),
+				),
+			},
+			{
+				PreConfig:       updateAccountParameter(t, client, sdk.AccountParameterDataRetentionTimeInDays, false, "10"),
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Database_DefaultDataRetentionTime/WithDataRetentionSet"),
+				ConfigVariables: configVariablesWithDatabaseDataRetentionTime(3),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "data_retention_time_in_days", "3"),
+					checkAccountAndDatabaseDataRetentionTime(id, 10, 3),
+				),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PostApplyPostRefresh: []plancheck.PlanCheck{
+						plancheck.ExpectEmptyPlan(),
+					},
+				},
+			},
+		},
+	})
+}
+
 func dbConfig(prefix string) string {
 	s := `
 resource "snowflake_database" "db" {
@@ -293,3 +456,35 @@ func testAccCheckIfDatabaseIsReplicated(t *testing.T, id string) func(state *ter
 		return nil
 	}
 }
+
+func checkAccountAndDatabaseDataRetentionTime(id sdk.AccountObjectIdentifier, expectedAccountRetentionDays int, expectedDatabaseRetentionsDays 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()
+
+		database, err := client.Databases.ShowByID(ctx, id)
+		if err != nil {
+			return err
+		}
+
+		if database.RetentionTime != expectedDatabaseRetentionsDays {
+			return fmt.Errorf("invalid database retention time, expected: %d, got: %d", expectedDatabaseRetentionsDays, database.RetentionTime)
+		}
+
+		param, err := client.Parameters.ShowAccountParameter(ctx, sdk.AccountParameterDataRetentionTimeInDays)
+		if err != nil {
+			return err
+		}
+		accountRetentionDays, err := strconv.Atoi(param.Value)
+		if err != nil {
+			return err
+		}
+
+		if accountRetentionDays != expectedAccountRetentionDays {
+			return fmt.Errorf("invalid account retention time, expected: %d, got: %d", expectedAccountRetentionDays, accountRetentionDays)
+		}
+
+		return nil
+	}
+}

pkg/resources/helpers_test.go

@@ -548,3 +548,25 @@ func queriedPrivilegesContainAtLeast(query func(client *sdk.Client, ctx context.
 		return nil
 	}
 }
+
+func updateAccountParameter(t *testing.T, client *sdk.Client, parameter sdk.AccountParameter, temporarily bool, newValue string) func() {
+	t.Helper()
+
+	ctx := context.Background()
+
+	param, err := client.Parameters.ShowAccountParameter(ctx, parameter)
+	require.NoError(t, err)
+	oldValue := param.Value
+
+	if temporarily {
+		t.Cleanup(func() {
+			err = client.Parameters.SetAccountParameter(ctx, parameter, oldValue)
+			require.NoError(t, err)
+		})
+	}
+
+	return func() {
+		err = client.Parameters.SetAccountParameter(ctx, parameter, newValue)
+		require.NoError(t, err)
+	}
+}