fix: unable to alter stage if url and si have changes (#1982) (#1983)

Previously, if both `URL` and `STORAGE_INTEGRATION` had been updated the provider would generate separate ALTER STAGE commands - one for `URL`, and another for `STORAGE` integration.  This can create an invalid change because:
- When changing `URL`: the new `URL` could be invalid with the existing storage integration `STORAGE_ALLOWED_LOCATIONS`
- When changing STORAGE_INTEGRATION: existing `URL` not present in the new storage integration `STORAGE_ALLOWED_LOCATIONS`

This MR now checks if both `URL` and `STORAGE_INTEGRATION` have changed and issues a single `ALTER` statement with both changes in one go.

Before:

ALTER STAGE the_stage SET URL = 's3://the_new_url'
ALTER STAGE the_stage SET STORAGE_INTEGRATION  = "new_storage_integration"

Now:

ALTER STAGE the_stage SET STORAGE_INTEGRATION  = "new_storage_integration" URL = 's3://the_new_url'

## Test Plan
* We have added tests for `ChangeStorageIntegrationAndUrl` (newly added function)
* We have added acceptance tests that replicate the bug (and prove this PR fixes it)
* We have added tests for `Stage` resource update to ensure:
  * The combined `ALTER STAGE` is only issued when both `URL` and `STORAGE_INTEGRATION` change
  * Existing behavior remains unchanged when only one of `URL`  and `STORAGE_INTEGRATION` change

## References
* Fixes #1982
* https://docs.snowflake.com/en/sql-reference/sql/alter-stage

Co-authored-by: Miguel Duarte <miguel.duarte@johnlewis.co.uk>

pkg/resources/stage.go

@@ -319,13 +319,6 @@ func UpdateStage(d *schema.ResourceData, meta interface{}) error {
 	builder := snowflake.NewStageBuilder(stage, dbName, schema)
 
 	db := meta.(*sql.DB)
-	if d.HasChange("url") {
-		url := d.Get("url")
-		q := builder.ChangeURL(url.(string))
-		if err := snowflake.Exec(db, q); err != nil {
-			return fmt.Errorf("error updating stage url on %v", d.Id())
-		}
-	}
 
 	if d.HasChange("credentials") {
 		credentials := d.Get("credentials")
@@ -335,11 +328,28 @@ func UpdateStage(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
-	if d.HasChange("storage_integration") {
+	if d.HasChange("storage_integration") && d.HasChange("url") {
 		si := d.Get("storage_integration")
-		q := builder.ChangeStorageIntegration(si.(string))
+		url := d.Get("url")
+		q := builder.ChangeStorageIntegrationAndUrl(si.(string), url.(string))
 		if err := snowflake.Exec(db, q); err != nil {
-			return fmt.Errorf("error updating stage storage integration on %v", d.Id())
+			return fmt.Errorf("error updating stage storage integration and url on %v", d.Id())
+		}
+	} else {
+		if d.HasChange("storage_integration") {
+			si := d.Get("storage_integration")
+			q := builder.ChangeStorageIntegration(si.(string))
+			if err := snowflake.Exec(db, q); err != nil {
+				return fmt.Errorf("error updating stage storage integration on %v", d.Id())
+			}
+		}
+
+		if d.HasChange("url") {
+			url := d.Get("url")
+			q := builder.ChangeURL(url.(string))
+			if err := snowflake.Exec(db, q); err != nil {
+				return fmt.Errorf("error updating stage url on %v", d.Id())
+			}
 		}
 	}
 

pkg/resources/stage_acceptance_test.go

@@ -0,0 +1,68 @@
+package resources_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+)
+
+func TestAccAlterStageWhenBothURLAndStorageIntegrationChange(t *testing.T) {
+	name := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)
+
+	resource.ParallelTest(t, resource.TestCase{
+		Providers:    providers(),
+		CheckDestroy: nil,
+		Steps: []resource.TestStep{
+			{
+				Config: stageIntegrationConfig(name, "si1", "s3://foo/"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_stage.test", "name", name),
+					resource.TestCheckResourceAttr("snowflake_stage.test", "url", "s3://foo/"),
+				),
+				Destroy: false,
+			},
+			{
+				Config: stageIntegrationConfig(name, "changed", "s3://changed/"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("snowflake_stage.test", "name", name),
+					resource.TestCheckResourceAttr("snowflake_stage.test", "url", "s3://changed/"),
+				),
+			},
+		},
+	})
+}
+
+func stageIntegrationConfig(name string, siNameSuffix string, url string) string {
+	resources := `
+resource "snowflake_database" "test" {
+	name = "%s"
+	comment = "Terraform acceptance test"
+}
+
+resource "snowflake_schema" "test" {
+	name = "%s"
+	database = snowflake_database.test.name
+	comment = "Terraform acceptance test"
+}
+
+resource "snowflake_storage_integration" "test" {
+	name = "%s%s"
+	storage_allowed_locations = ["%s"]
+	storage_provider = "S3"
+
+  	storage_aws_role_arn = "arn:aws:iam::000000000001:/role/test"
+}
+
+resource "snowflake_stage" "test" {
+	name = "%s"
+	url = "%s"
+	storage_integration = snowflake_storage_integration.test.name
+	schema = snowflake_schema.test.name
+	database = snowflake_database.test.name
+}
+`
+
+	return fmt.Sprintf(resources, name, name, name, siNameSuffix, url, name, url)
+}

pkg/resources/stage_test.go

@@ -107,3 +107,67 @@ func TestStageRead(t *testing.T) {
 		r.Nil(err)
 	})
 }
+
+func TestStageUpdateWithSIAndURL(t *testing.T) {
+	r := require.New(t)
+
+	in := map[string]interface{}{
+		"name":                "test_stage",
+		"database":            "test_db",
+		"schema":              "test_schema",
+		"url":                 "s3://changed_url",
+		"storage_integration": "changed_integration",
+	}
+
+	d := stage(t, "test_db|test_schema|test_stage", in)
+
+	WithMockDb(t, func(db *sql.DB, mock sqlmock.Sqlmock) {
+		mock.ExpectExec(`ALTER STAGE "test_db"."test_schema"."test_stage" SET STORAGE_INTEGRATION = "changed_integration" URL = 's3://changed_url'`).WillReturnResult(sqlmock.NewResult(1, 1))
+		expectReadStage(mock)
+		expectReadStageShow(mock)
+		err := resources.UpdateStage(d, db)
+		r.NoError(err)
+	})
+}
+
+func TestStageUpdateWithJustURL(t *testing.T) {
+	r := require.New(t)
+
+	in := map[string]interface{}{
+		"name":     "test_stage",
+		"database": "test_db",
+		"schema":   "test_schema",
+		"url":      "s3://changed_url",
+	}
+
+	d := stage(t, "test_db|test_schema|test_stage", in)
+
+	WithMockDb(t, func(db *sql.DB, mock sqlmock.Sqlmock) {
+		mock.ExpectExec(`ALTER STAGE "test_db"."test_schema"."test_stage" SET URL = 's3://changed_url'`).WillReturnResult(sqlmock.NewResult(1, 1))
+		expectReadStage(mock)
+		expectReadStageShow(mock)
+		err := resources.UpdateStage(d, db)
+		r.NoError(err)
+	})
+}
+
+func TestStageUpdateWithJustSI(t *testing.T) {
+	r := require.New(t)
+
+	in := map[string]interface{}{
+		"name":                "test_stage",
+		"database":            "test_db",
+		"schema":              "test_schema",
+		"storage_integration": "changed_integration",
+	}
+
+	d := stage(t, "test_db|test_schema|test_stage", in)
+
+	WithMockDb(t, func(db *sql.DB, mock sqlmock.Sqlmock) {
+		mock.ExpectExec(`ALTER STAGE "test_db"."test_schema"."test_stage" SET STORAGE_INTEGRATION = "changed_integration"`).WillReturnResult(sqlmock.NewResult(1, 1))
+		expectReadStage(mock)
+		expectReadStageShow(mock)
+		err := resources.UpdateStage(d, db)
+		r.NoError(err)
+	})
+}

pkg/snowflake/stage.go

@@ -198,6 +198,10 @@ func (sb *StageBuilder) ChangeStorageIntegration(s string) string {
 	return fmt.Sprintf(`ALTER STAGE %v SET STORAGE_INTEGRATION = "%v"`, sb.QualifiedName(), s)
 }
 
+func (sb *StageBuilder) ChangeStorageIntegrationAndUrl(s string, url string) string {
+	return fmt.Sprintf(`ALTER STAGE %v SET STORAGE_INTEGRATION = "%v" URL = '%v'`, sb.QualifiedName(), s, url)
+}
+
 // ChangeEncryption returns the SQL query that will update the encryption on the stage.
 func (sb *StageBuilder) ChangeEncryption(e string) string {
 	return fmt.Sprintf(`ALTER STAGE %v SET ENCRYPTION = (%v)`, sb.QualifiedName(), e)

pkg/snowflake/stage_test.go

@@ -86,6 +86,13 @@ func TestStageChangeStorageIntegration(t *testing.T) {
 	r.Equal(`ALTER STAGE "test_db"."test_schema"."test_stage" SET STORAGE_INTEGRATION = "MY_INTEGRATION"`, s.ChangeStorageIntegration("MY_INTEGRATION"))
 }
 
+func TestStageChangeStorageIntegrationAndUrl(t *testing.T) {
+	r := require.New(t)
+	s := NewStageBuilder("test_stage", "test_db", "test_schema")
+
+	r.Equal(`ALTER STAGE "test_db"."test_schema"."test_stage" SET STORAGE_INTEGRATION = "MY_INTEGRATION" URL = 's3://load/test'`, s.ChangeStorageIntegrationAndUrl("MY_INTEGRATION", "s3://load/test"))
+}
+
 func TestStageChangeCopyOptions(t *testing.T) {
 	r := require.New(t)
 	s := NewStageBuilder("test_stage", "test_db", "test_schema")