Skip to content

fix: external function varchar return type validation #3400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sfc-gh-jmichalak merged 2 commits into from
Feb 19, 2025
+87 −2
Merged

fix: external function varchar return type validation #3400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c977245
fix: external function varchar return type validation
nijave Feb 14, 2025
942f288
Merge branch 'dev' into nv-fix-varchar
sfc-gh-jmichalak Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions MIGRATION_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ across different versions.
> [!TIP]
> We highly recommend upgrading the versions one by one instead of bulk upgrades.

## v1.0.3 ➞ v1.0.4

### Fixed external_function VARCHAR return_type
VARCHAR external_function return_type did not work correctly before ([#3392](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3392)) but was fixed in this version.

## v1.0.2 ➞ v1.0.3

### Fixed METRIC_LEVEL parameter
Expand Down
4 changes: 2 additions & 2 deletions pkg/resources/external_function.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,8 @@ func ReadContextExternalFunction(ctx context.Context, d *schema.ResourceData, me
}
case "returns":
returnType := row.Value
// We first check for VARIANT or OBJECT
if returnType == "VARIANT" || returnType == "OBJECT" {
// We first check types that don't require size
if returnType == "VARIANT" || returnType == "OBJECT" || returnType == "VARCHAR" {

@sfc-gh-asawicki sfc-gh-asawicki Feb 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other types (like number, timestamp, etc.)? They also come in with-parameters and without-parameters flavours and they should be addressed too.

@nijave nijave Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really imo it seems much easier to just remove all the validation logic and let Snowflake handle it server-side on apply. Snowflake already has validation logic and it seems counterproductive trying to reproduce/reverse engineer that closed source logic in an OSS project.

@sfc-gh-asawicki sfc-gh-asawicki Feb 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned, the data types will be reworked in the provider. This logic will probably be trashed either way.

However, we need to handle the data type logic on the provider side to correctly handle plans and diff suppressions; we can't just remove it and hope for the best; these resources would be unusable.

Additionally, there are users, like you, who would prefer the "applies" to fail (full sf side validation), but there are also those who would like to have almost 100% parity between successful "plans" and successful "applies." The current approach lies somewhere in the middle.

@nijave nijave Feb 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, we need to handle the data type logic on the provider side to correctly handle plans and diff suppressions; we can't just remove it and hope for the best; these resources would be unusable.

I'm not sure what you mean here. Diff suppression is separate and orthogonal to config validation. It is very common practice in providers to do minimal config validation and allow the API to do full validation. Ref: AWS Terraform Provider

but there are also those who would like to have almost 100% parity between successful "plans" and successful "applies."

That's exactly the issue here. VARCHAR(10) plans correctly but fails to apply since VARCHAR(10) is submitted to Snowflake but VARCHAR is returned from Snowflake which fails validation when Terraform tries to save/update state

@sfc-gh-asawicki sfc-gh-asawicki Feb 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the issue here. VARCHAR(10) plans correctly but fails to apply since VARCHAR(10) is submitted to Snowflake but VARCHAR is returned from Snowflake which fails validation when Terraform tries to save/update state

No argument here: we know this preview resource is not handling data types correctly, we did not come up with this logic, and we will correct it while reworking this resource.

I'm not sure what you mean here. Diff suppression is separate and orthogonal to config validation. It is very common practice in providers to do minimal config validation and allow the API to do full validation. Ref: AWS Terraform Provider

Unfortunately, some logic is needed if you want to handle both the user config changes and the external changes properly and, at the same time, allow users to use data types without attributes (using default ones, e.g. NUMBER = NUMBER(38, 0)). For example, you need to be able to differentiate between these situations:

  1. User sets VARCHAR(10) in config; then, they change it to VARCHAR (change should happen)
  2. User sets VARCHAR(10) in config; Snowflake returns VARCHAR (change should be suppressed)

Other example:

  1. User sets NUMBER in the config, Snowflake returns NUMBER(38, 0) (change should be suppressed)
  2. User sets NUMBER in the config; then, they change it to NUMBER(38, 0) (change should happen - or not, depending on the approach we take; the question is how do we want to handle the default change in Snowflake and how would it be performed on SNowflake side for the existing objects)
  3. User sets NUMBER in the config; later on, Snowflake returns NUMBER(38, 2) (change should happen)

All these situations will be handled correctly when we rework this preview resource, which will not happen any time soon.

if err := d.Set("return_type", returnType); err != nil {
return diag.Errorf("error setting return_type: %v", err)
}
Expand Down
80 changes: 80 additions & 0 deletions pkg/resources/external_function_acceptance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,54 @@ func TestAcc_ExternalFunction_issue2528(t *testing.T) {
})
}

func TestAcc_ExternalFunction_issue3392_returnVarchar(t *testing.T) {
accName := acc.TestClient().Ids.Alpha()

resourceName := "snowflake_external_function.f"

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.ExternalFunction),
Steps: []resource.TestStep{
{
Config: externalFunctionConfigWithReturnType(acc.TestDatabaseName, acc.TestSchemaName, accName, "VARCHAR"),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "return_type", "VARCHAR"),
),
},
},
})
}

func TestAcc_ExternalFunction_issue3392_returnVarcharWithSize(t *testing.T) {
accName := acc.TestClient().Ids.Alpha()

resourceName := "snowflake_external_function.f"

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.ExternalFunction),
Steps: []resource.TestStep{
{
Config: externalFunctionConfigWithReturnType(acc.TestDatabaseName, acc.TestSchemaName, accName, "VARCHAR(10)"),
Check: resource.ComposeTestCheckFunc(
// Snowflake drops the size from VARCHAR when it's specified
resource.TestCheckResourceAttr(resourceName, "return_type", "VARCHAR"),
),
ExpectNonEmptyPlan: true,
},
},
})
}

// Proves that header parsing handles values wrapped in curly braces, e.g. `value = "{1}"`
func TestAcc_ExternalFunction_HeaderParsing(t *testing.T) {
id := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
Expand Down Expand Up @@ -494,6 +542,38 @@ resource "snowflake_external_function" "f" {
`, database, schema, name, returnNullAllowedText)
}

func externalFunctionConfigWithReturnType(database string, schema string, name string, returnType string) string {

return fmt.Sprintf(`
resource "snowflake_api_integration" "test_api_int" {
name = "%[3]s"
api_provider = "aws_api_gateway"
api_aws_role_arn = "arn:aws:iam::000000000001:/role/test"
api_allowed_prefixes = ["https://123456.execute-api.us-west-2.amazonaws.com/prod/"]
enabled = true
}

resource "snowflake_external_function" "f" {
name = "%[3]s"
database = "%[1]s"
schema = "%[2]s"
arg {
name = "ARG1"
type = "VARCHAR"
}
arg {
name = "ARG2"
type = "VARCHAR"
}
return_type = "%[4]s"
return_behavior = "IMMUTABLE"
api_integration = snowflake_api_integration.test_api_int.name
url_of_proxy_and_resource = "https://123456.execute-api.us-west-2.amazonaws.com/prod/test_func"
}

`, database, schema, name, returnType)
}

func externalFunctionConfigIssue2528(database string, schema string, name string, schema2 string) string {
return fmt.Sprintf(`
resource "snowflake_api_integration" "test_api_int" {
Expand Down