fix: database tests and introduce a new parameter (#2981)

## Changes
- #2978 (adjust migration guide)
- Fix database tests (remove tests for state upgraders, because they
were incorrect and document step-by-step process for manual testing; I
went through all of it)
- Introduce a new parameter to address #2826 and test it
- added a drop with retries just to be sure, but I don't think it should
ever fail other than some kind of strange networking error between
database create and drop schema
- I opted to drop the schema after setting id, so if we manage to create
a database, but not drop the schema, the provider will print out a
warning message that the public schema couldn't be dropped and it should
be done manually. The other approaches I considered:
- Do it before setting ID, but that would mean the Terraform running
create again and failing on CREATE DATABASE (because it's already
there).
- Do it like right now, but make sure the public schema is gone in the
alter, but I didn't go for that, because it could lead to further
complications with (what is considered "after create"; after successful
create? how would we keep the state of create status? etc.).

---------

Co-authored-by: Jakub Michalak <[email protected]>

MIGRATION_GUIDE.md

@@ -461,7 +461,9 @@ resource "snowflake_database" "test" {
 }
 ```
 
-If you had `from_database` set, it should migrate automatically.
+If you had `from_database` set, you should follow our [resource migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md) to remove 
+the database from state to later import it in the newer version of the provider.
+Otherwise, it may cause issues when migrating to v0.93.0.
 For now, we're dropping the possibility to create a clone database from other databases.
 The only way will be to clone a database manually and import it as `snowflake_database`, but if
 cloned databases diverge in behavior from standard databases, it may cause issues.

docs/resources/database.md

@@ -90,6 +90,7 @@ resource "snowflake_database" "primary" {
 - `comment` (String) Specifies a comment for the database.
 - `data_retention_time_in_days` (Number) Specifies the number of days for which Time Travel actions (CLONE and UNDROP) can be performed on the database, as well as specifying the default Time Travel retention time for all schemas created in the database. For more details, see [Understanding & Using Time Travel](https://docs.snowflake.com/en/user-guide/data-time-travel).
 - `default_ddl_collation` (String) Specifies a default collation specification for all schemas and tables added to the database. It can be overridden on schema or table level. For more information, see [collation specification](https://docs.snowflake.com/en/sql-reference/collation#label-collation-specification).
+- `drop_public_schema_on_creation` (Boolean) Specifies whether to drop public schema on creation or not. Modifying the parameter after database is already created won't have any effect.
 - `enable_console_output` (Boolean) If true, enables stdout/stderr fast path logging for anonymous stored procedures.
 - `external_volume` (String) The database parameter that specifies the default external volume to use for Iceberg tables. For more information, see [EXTERNAL_VOLUME](https://docs.snowflake.com/en/sql-reference/parameters#external-volume).
 - `is_transient` (Boolean) Specifies the database as transient. Transient databases 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.

pkg/acceptance/helpers/database_client.go

@@ -146,3 +146,13 @@ func (c *DatabaseClient) Show(t *testing.T, id sdk.AccountObjectIdentifier) (*sd
 
 	return c.client().ShowByID(ctx, id)
 }
+
+func (c *DatabaseClient) Describe(t *testing.T, id sdk.AccountObjectIdentifier) *sdk.DatabaseDetails {
+	t.Helper()
+	ctx := context.Background()
+
+	details, err := c.client().Describe(ctx, id)
+	require.NoError(t, err)
+
+	return details
+}

pkg/acceptance/snowflakechecks/database.go

@@ -3,6 +3,7 @@ package snowflakechecks
 import (
 	"errors"
 	"fmt"
+	"slices"
 	"testing"
 
 	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
@@ -26,3 +27,23 @@ func CheckDatabaseDataRetentionTimeInDays(t *testing.T, databaseId sdk.AccountOb
 		return errors.Join(errs...)
 	}
 }
+
+func DoesNotContainPublicSchema(t *testing.T, id sdk.AccountObjectIdentifier) resource.TestCheckFunc {
+	t.Helper()
+	return func(state *terraform.State) error {
+		if slices.ContainsFunc(acc.TestClient().Database.Describe(t, id).Rows, func(row sdk.DatabaseDetailsRow) bool { return row.Name == "PUBLIC" && row.Kind == "SCHEMA" }) {
+			return fmt.Errorf("expected database %s to not contain public schema", id.FullyQualifiedName())
+		}
+		return nil
+	}
+}
+
+func ContainsPublicSchema(t *testing.T, id sdk.AccountObjectIdentifier) resource.TestCheckFunc {
+	t.Helper()
+	return func(state *terraform.State) error {
+		if !slices.ContainsFunc(acc.TestClient().Database.Describe(t, id).Rows, func(row sdk.DatabaseDetailsRow) bool { return row.Name == "PUBLIC" && row.Kind == "SCHEMA" }) {
+			return fmt.Errorf("expected database %s to contain public schema", id.FullyQualifiedName())
+		}
+		return nil
+	}
+}

pkg/resources/database.go

@@ -6,6 +6,9 @@ import (
 	"fmt"
 	"slices"
 	"strings"
+	"time"
+
+	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/util"
 
 	"github.com/hashicorp/go-cty/cty"
 
@@ -22,6 +25,12 @@ var databaseSchema = map[string]*schema.Schema{
 		Required:    true,
 		Description: "Specifies the identifier for the database; must be unique for your account. As a best practice for [Database Replication and Failover](https://docs.snowflake.com/en/user-guide/db-replication-intro), it is recommended to give each secondary database the same name as its primary database. This practice supports referencing fully-qualified objects (i.e. '<db>.<schema>.<object>') by other objects in the same database, such as querying a fully-qualified table name in a view. If a secondary database has a different name from the primary database, then these object references would break in the secondary database.",
 	},
+	"drop_public_schema_on_creation": {
+		Type:             schema.TypeBool,
+		Optional:         true,
+		Description:      "Specifies whether to drop public schema on creation or not. Modifying the parameter after database is already created won't have any effect.",
+		DiffSuppressFunc: IgnoreAfterCreation,
+	},
 	"is_transient": {
 		Type:        schema.TypeBool,
 		Optional:    true,
@@ -124,6 +133,24 @@ func CreateDatabase(ctx context.Context, d *schema.ResourceData, meta any) diag.
 
 	var diags diag.Diagnostics
 
+	if d.Get("drop_public_schema_on_creation").(bool) {
+		var dropSchemaErrs []error
+		err := util.Retry(3, time.Second, func() (error, bool) {
+			if err := client.Schemas.Drop(ctx, sdk.NewDatabaseObjectIdentifier(id.Name(), "PUBLIC"), &sdk.DropSchemaOptions{IfExists: sdk.Bool(true)}); err != nil {
+				dropSchemaErrs = append(dropSchemaErrs, err)
+				return nil, false
+			}
+			return nil, true
+		})
+		if err != nil {
+			diags = append(diags, diag.Diagnostic{
+				Severity: diag.Warning,
+				Summary:  "Failed to drop public schema on creation (failed after 3 attempts)",
+				Detail:   fmt.Sprintf("The '%s' database was created successfully, but the provider was not able to remove public schema on creation. Please drop the public schema manually. Original errors: %s", id.Name(), errors.Join(dropSchemaErrs...)),
+			})
+		}
+	}
+
 	if v, ok := d.GetOk("replication"); ok {
 		replicationConfiguration := v.([]any)[0].(map[string]any)
 

pkg/resources/database_acceptance_test.go

@@ -2,7 +2,6 @@ package resources_test
 
 import (
 	"fmt"
-	"regexp"
 	"strconv"
 	"testing"
 
@@ -15,7 +14,6 @@ import (
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/importchecks"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/planchecks"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/snowflakechecks"
-	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/provider/resources"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-testing/config"
@@ -1160,233 +1158,81 @@ resource "snowflake_database" "test" {
 `, id.Name(), strconv.Quote(enableToAccount))
 }
 
-func TestAcc_Database_UpgradeFromShare(t *testing.T) {
-	_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)
-
+func TestAcc_Database_WithoutPublicSchema(t *testing.T) {
 	id := acc.TestClient().Ids.RandomAccountObjectIdentifier()
-	secondaryClientLocator := acc.SecondaryClient(t).GetAccountLocator()
-
-	shareExternalId := createShareableDatabase(t)
 
 	resource.Test(t, resource.TestCase{
-		PreCheck: func() { acc.TestAccPreCheck(t) },
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
 		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
 			tfversion.RequireAbove(tfversion.Version1_5_0),
 		},
+		CheckDestroy: acc.CheckDestroy(t, resources.Database),
 		Steps: []resource.TestStep{
 			{
-				ExternalProviders: map[string]resource.ExternalProvider{
-					"snowflake": {
-						VersionConstraint: "=0.92.0",
-						Source:            "Snowflake-Labs/snowflake",
-					},
-				},
-				Config: databaseStateUpgraderFromShareOld(id, secondaryClientLocator, shareExternalId),
+				Config: databaseWithDropPublicSchemaConfig(id, true),
 				Check: resource.ComposeTestCheckFunc(
 					resource.TestCheckResourceAttr("snowflake_database.test", "id", id.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.test", "name", id.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.test", "from_share.provider", secondaryClientLocator),
-					resource.TestCheckResourceAttr("snowflake_database.test", "from_share.share", shareExternalId.Name()),
+					snowflakechecks.DoesNotContainPublicSchema(t, id),
 				),
 			},
+			// Change in parameter shouldn't change the state Snowflake
 			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromShareNewAfterUpgrade(id),
-				ExpectError:              regexp.MustCompile("failed to upgrade the state with database created from share, please use snowflake_shared_database or deprecated snowflake_database_old instead"),
-			},
-			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromShareNew(id, shareExternalId),
-				ResourceName:             "snowflake_shared_database.test",
-				ImportStateId:            id.FullyQualifiedName(),
-				ImportState:              true,
-			},
-		},
-	})
-}
-
-func databaseStateUpgraderFromShareOld(id sdk.AccountObjectIdentifier, secondaryClientLocator string, externalShare sdk.ExternalObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-	data_retention_time_in_days = 0 # to avoid in-place update to -1
-	from_share = {
-		provider = "%s"
-		share = "%s"
-	}
-}
-`, id.Name(), secondaryClientLocator, externalShare.Name())
-}
-
-func databaseStateUpgraderFromShareNewAfterUpgrade(id sdk.AccountObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-	data_retention_time_in_days = 0 # to avoid in-place update to -1
-}
-`, id.Name())
-}
-
-func databaseStateUpgraderFromShareNew(id sdk.AccountObjectIdentifier, externalShare sdk.ExternalObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_shared_database" "test" {
-	name = "%s"
-	from_share = %s
-}
-`, id.Name(), strconv.Quote(externalShare.FullyQualifiedName()))
-}
-
-func TestAcc_Database_UpgradeFromReplica(t *testing.T) {
-	_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)
-
-	id := acc.TestClient().Ids.RandomAccountObjectIdentifier()
-	_, primaryDatabaseId, databaseCleanup := acc.SecondaryTestClient().Database.CreatePrimaryDatabase(t, []sdk.AccountIdentifier{
-		acc.TestClient().Account.GetAccountIdentifier(t),
-	})
-	t.Cleanup(databaseCleanup)
-
-	resource.Test(t, resource.TestCase{
-		PreCheck: func() { acc.TestAccPreCheck(t) },
-		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
-			tfversion.RequireAbove(tfversion.Version1_5_0),
-		},
-		Steps: []resource.TestStep{
-			{
-				ExternalProviders: map[string]resource.ExternalProvider{
-					"snowflake": {
-						VersionConstraint: "=0.92.0",
-						Source:            "Snowflake-Labs/snowflake",
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_database.test", plancheck.ResourceActionNoop),
 					},
 				},
-				Config: databaseStateUpgraderFromReplicaOld(id, primaryDatabaseId),
+				Config: databaseWithDropPublicSchemaConfig(id, false),
 				Check: resource.ComposeTestCheckFunc(
 					resource.TestCheckResourceAttr("snowflake_database.test", "id", id.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.test", "name", id.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.test", "from_replica", primaryDatabaseId.FullyQualifiedName()),
+					snowflakechecks.DoesNotContainPublicSchema(t, id),
 				),
 			},
-			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromReplicaNewAfterUpgrade(id),
-				ExpectError:              regexp.MustCompile("failed to upgrade the state with database created from replica, please use snowflake_secondary_database or deprecated snowflake_database_old instead"),
-			},
-			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromReplicaNew(id, primaryDatabaseId),
-				ResourceName:             "snowflake_secondary_database.test",
-				ImportStateId:            id.FullyQualifiedName(),
-				ImportState:              true,
-			},
 		},
 	})
 }
 
-func databaseStateUpgraderFromReplicaOld(id sdk.AccountObjectIdentifier, primaryDatabaseId sdk.ExternalObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-	data_retention_time_in_days = 0 # to avoid in-place update to -1
-	from_replica = %s
-}
-`, id.Name(), strconv.Quote(primaryDatabaseId.FullyQualifiedName()))
-}
-
-func databaseStateUpgraderFromReplicaNewAfterUpgrade(id sdk.AccountObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-	data_retention_time_in_days = 0
-}
-`, id.Name())
-}
-
-func databaseStateUpgraderFromReplicaNew(id sdk.AccountObjectIdentifier, primaryDatabaseId sdk.ExternalObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_secondary_database" "test" {
-	name = "%s"
-	as_replica_of = %s
-}
-`, id.Name(), strconv.Quote(id.FullyQualifiedName()))
-}
-
-func TestAcc_Database_UpgradeFromClonedDatabase(t *testing.T) {
+func TestAcc_Database_WithPublicSchema(t *testing.T) {
 	id := acc.TestClient().Ids.RandomAccountObjectIdentifier()
-	cloneId := acc.TestClient().Ids.RandomAccountObjectIdentifier()
 
 	resource.Test(t, resource.TestCase{
-		PreCheck: func() { acc.TestAccPreCheck(t) },
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
 		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
 			tfversion.RequireAbove(tfversion.Version1_5_0),
 		},
 		CheckDestroy: acc.CheckDestroy(t, resources.Database),
 		Steps: []resource.TestStep{
 			{
-				ExternalProviders: map[string]resource.ExternalProvider{
-					"snowflake": {
-						VersionConstraint: "=0.92.0",
-						Source:            "Snowflake-Labs/snowflake",
-					},
-				},
-				Config: databaseStateUpgraderFromDatabaseOld(id, cloneId),
+				Config: databaseWithDropPublicSchemaConfig(id, false),
 				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr("snowflake_database.cloned", "id", cloneId.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.cloned", "name", cloneId.Name()),
-					resource.TestCheckResourceAttr("snowflake_database.cloned", "from_database", id.Name()),
+					resource.TestCheckResourceAttr("snowflake_database.test", "id", id.Name()),
+					snowflakechecks.ContainsPublicSchema(t, id),
 				),
 			},
+			// Change in parameter shouldn't change the state Snowflake
 			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromDatabaseNewAfterUpgrade(id, cloneId),
-				ExpectError:              regexp.MustCompile("failed to upgrade the state with database created from database, please use snowflake_database or deprecated snowflake_database_old instead"),
-			},
-			{
-				ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
-				Config:                   databaseStateUpgraderFromDatabaseNew(id, cloneId),
-				ResourceName:             "snowflake_database.cloned",
-				ImportStateId:            cloneId.FullyQualifiedName(),
-				ImportState:              true,
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("snowflake_database.test", plancheck.ResourceActionNoop),
+					},
+				},
+				Config: databaseWithDropPublicSchemaConfig(id, true),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_database.test", "id", id.Name()),
+					snowflakechecks.ContainsPublicSchema(t, id),
+				),
 			},
 		},
 	})
 }
 
-func databaseStateUpgraderFromDatabaseOld(id sdk.AccountObjectIdentifier, secondId sdk.AccountObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-	data_retention_time_in_days = 0 # to avoid in-place update to -1
-}
-
-resource "snowflake_database" "cloned" {
-	name = "%s"
-	data_retention_time_in_days = 0 # to avoid in-place update to -1
-	from_database = snowflake_database.test.name
-}
-`, id.Name(), secondId.Name())
-}
-
-func databaseStateUpgraderFromDatabaseNewAfterUpgrade(id sdk.AccountObjectIdentifier, secondId sdk.AccountObjectIdentifier) string {
+func databaseWithDropPublicSchemaConfig(id sdk.AccountObjectIdentifier, withDropPublicSchema bool) string {
 	return fmt.Sprintf(`
 resource "snowflake_database" "test" {
 	name = "%s"
-	data_retention_time_in_days = 0
-}
-
-resource "snowflake_database" "cloned" {
-	name = "%s"
-	data_retention_time_in_days = 0
-}
-`, id.Name(), secondId.Name())
-}
-
-func databaseStateUpgraderFromDatabaseNew(id sdk.AccountObjectIdentifier, secondId sdk.AccountObjectIdentifier) string {
-	return fmt.Sprintf(`
-resource "snowflake_database" "test" {
-	name = "%s"
-}
-
-resource "snowflake_database" "cloned" {
-	name = "%s"
+	drop_public_schema_on_creation = %s
 }
-`, id.Name(), secondId.Name())
+`, id.Name(), strconv.FormatBool(withDropPublicSchema))
 }