fix: Do not panic in parsing identifiers with arguments (#4191)

Do not panic in parsing identifiers with arguments.

## References
#4187

Author: sfc-gh-jmichalak

MIGRATION_GUIDE.md

@@ -26,6 +26,27 @@ for changes required after enabling given [Snowflake BCR Bundle](https://docs.sn
 
 ## v2.10.1 ➞ v2.10.2
 
+### *(bugfix)* Improved validation of identifiers with arguments
+Previously, during parsing identifiers with argument types, when the identifier format was incorrect, the provider could panic with errors like:
+```
+Stack trace from the terraform-provider-snowflake_v2.8.0 plugin:
+
+panic: runtime error: index out of range [1] with length 1
+
+goroutine 142 [running]:
+github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk.ParseSchemaObjectIdentifierWithArguments({0x14000ff3a40, 0x28})
+github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk/identifier_parsers.go:164 +0x2bc
+```
+This could happen when e.g. the passed identifier was using the old pipe-separated format.
+
+In this version, the provider validates the expected number of parts, so the error message is like this:
+```
+unexpected number of parts 1 in identifier abc|def|ghi(varchar), expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...)>" where <argname> is optional
+```
+No changes in configuration are required.
+
+References: [#4187](https://github.com/snowflakedb/terraform-provider-snowflake/issues/4187)
+
 ### *(bugfix)* Disallowed setting `DATABASE ROLES` object type on `all` and `future` fields in `snowflake_grant_ownership` resource
 Previously, the provider allowed setting the `DATABASE ROLES` object type on `all` and `future` fields in the `grant_ownership` resource.
 This operation is not allowed in Snowflake, and such resource configuration resulted in errors being returned from Snowflake.
@@ -179,7 +200,7 @@ In this version we introduce a new attribute on the provider level: [`experiment
 
 We treat the available values as experiments, that may become stable feature/behavior in the future provider releases if successful.
 
-Currently, the only available experiment is `WAREHOUSE_SHOW_IMPROVED_PERFORMANCE`. When enabled, it uses a slightly different SHOW query to read warehouse details. It's meant to improve the performance for accounts with many warehouses. 
+Currently, the only available experiment is `WAREHOUSE_SHOW_IMPROVED_PERFORMANCE`. When enabled, it uses a slightly different SHOW query to read warehouse details. It's meant to improve the performance for accounts with many warehouses.
 
 **Important**: to benefit from this improvement, you need to have it enabled also on your Snowflake account. To do this, please reach out to us through your Snowflake Account Manager.
 

pkg/sdk/identifier_parsers.go

@@ -155,6 +155,9 @@ func ParseSchemaObjectIdentifierWithArguments(fullyQualifiedName string) (Schema
 	if err != nil {
 		return SchemaObjectIdentifierWithArguments{}, err
 	}
+	if len(parts) != 3 {
+		return SchemaObjectIdentifierWithArguments{}, fmt.Errorf(`unexpected number of parts %[1]d in identifier %[2]s, expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...)>" where <argname> is optional`, len(parts), fullyQualifiedName)
+	}
 	parsedArguments, err := ParseFunctionAndProcedureArguments(fullyQualifiedName[splitIdIndex:])
 	if err != nil {
 		return SchemaObjectIdentifierWithArguments{}, err
@@ -179,6 +182,9 @@ func ParseSchemaObjectIdentifierWithArgumentsAndReturnType(fullyQualifiedName st
 	if err != nil {
 		return SchemaObjectIdentifierWithArguments{}, err
 	}
+	if len(parts) != 3 {
+		return SchemaObjectIdentifierWithArguments{}, fmt.Errorf(`unexpected number of parts %[1]d in identifier %[2]s, expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...):<returntype>" where <argname> is optional`, len(parts), fullyQualifiedName)
+	}
 	functionHeader := parts[2]
 	leftParenthesisIndex := strings.IndexRune(functionHeader, '(')
 	functionName := functionHeader[:leftParenthesisIndex]

pkg/sdk/identifier_parsers_test.go

@@ -319,9 +319,11 @@ func TestNewSchemaObjectIdentifierWithArgumentsFromFullyQualifiedName_WithRawInp
 	}{
 		{RawInput: `abc.def.ghi()`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`)},
 		{RawInput: `abc.def.ghi(FLOAT, VECTOR(INT, 20))`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`, DataTypeFloat, "VECTOR(INT, 20)")},
+		{RawInput: `abc.def.ghi(arg1 FLOAT, arg2 VECTOR(INT, 20))`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`, DataTypeFloat, "VECTOR(INT, 20)")},
 		// TODO(SNOW-1571674): Won't work, because of the assumption that identifiers are not containing '(' and ')' parentheses (unfortunately, we're not able to produce meaningful errors for those cases)
 		{RawInput: `abc."(ef".ghi(FLOAT, VECTOR(INT, 20))`, Error: `unable to read identifier: abc."`},
 		{RawInput: `abc.def.ghi`, Error: `unable to parse identifier: '(' not present`},
+		{RawInput: `abc|def|ghi(varchar)`, Error: `unexpected number of parts 1 in identifier abc|def|ghi(varchar), expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...)>" where <argname> is optional`},
 	}
 
 	for _, testCase := range testCases {
@@ -350,6 +352,7 @@ func TestNewSchemaObjectIdentifierWithArgumentsAndReturnTypeFromFullyQualifiedNa
 		{RawInput: `abc.def."ghi(FLOAT, VECTOR(INT, 20)):NUMBER(10,2)"`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`, DataTypeFloat, "VECTOR(INT, 20)")},
 		{RawInput: `abc.def."ghi(FLOAT, VECTOR(INT, 20)):NUMBER"`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`, DataTypeFloat, "VECTOR(INT, 20)")},
 		{RawInput: `abc.def."ghi(ab FLOAT, VECTOR VECTOR(INT, 20), FLOAT):NUMBER"`, ExpectedIdentifierStructure: NewSchemaObjectIdentifierWithArguments(`abc`, `def`, `ghi`, DataTypeFloat, "VECTOR(INT, 20)", DataTypeFloat)},
+		{RawInput: `abc|def|ghi(varchar):NUMBER`, Error: `unexpected number of parts 1 in identifier abc|def|ghi(varchar):NUMBER, expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...):<returntype>" where <argname> is optional`},
 	}
 
 	for _, testCase := range testCases {

pkg/testacc/resource_function_java_acceptance_test.go

@@ -4,6 +4,7 @@ package testacc
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 	"time"
 
@@ -96,6 +97,7 @@ func TestAcc_FunctionJava_InlineBasic(t *testing.T) {
 			{
 				ResourceName:            functionModel.ResourceReference(),
 				ImportState:             true,
+				ImportStateId:           id.FullyQualifiedName(),
 				ImportStateVerify:       true,
 				ImportStateVerifyIgnore: []string{"is_secure", "arguments.0.arg_data_type", "null_input_behavior", "return_results_behavior"},
 				ImportStateCheck: assertThatImport(t,
@@ -548,3 +550,44 @@ func TestAcc_FunctionJava_handleExternalLanguageChange(t *testing.T) {
 		},
 	})
 }
+
+func TestAcc_FunctionJava_Issue4187(t *testing.T) {
+	className := "TestFunc"
+	funcName := "echoVarchar"
+	returnDataType := testdatatypes.DataTypeVarchar_100
+
+	id := testClient().Ids.RandomSchemaObjectIdentifierWithArgumentsNewDataTypes()
+
+	handler := fmt.Sprintf("%s.%s", className, funcName)
+	definition := testClient().Function.SampleJavaDefinitionNoArgs(t, className, funcName)
+
+	functionModel := model.FunctionJavaBasicInline("w", id, returnDataType, handler, definition)
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: functionsAndProceduresProviderFactory,
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: CheckDestroy(t, resources.FunctionJava),
+		Steps: []resource.TestStep{
+			// CREATE BASIC
+			{
+				Config: config.FromModels(t, functionModel),
+				Check: assertThat(t,
+					resourceassert.FunctionJavaResource(t, functionModel.ResourceReference()).
+						HasNameString(id.Name()).
+						HasFunctionDefinitionString(definition).
+						HasFunctionLanguageString("JAVA").
+						HasFullyQualifiedNameString(id.FullyQualifiedName()),
+				),
+			},
+			// IMPORT
+			{
+				ResourceName:  functionModel.ResourceReference(),
+				ImportState:   true,
+				ImportStateId: "a|b|c()",
+				ExpectError:   regexp.MustCompile(`unexpected number of parts 1 in identifier a|b|c(), expected 3 in a form of "<database_name>.<schema_name>.<schema_object_name>(<argname> <argtype>...)>" where <argname> is optional`),
+			},
+		},
+	})
+}