Skip to content

Commit

Permalink
fix: Fix encode Snowflake ID for object identifiers (#2256)
Browse files Browse the repository at this point in the history 
* Add test that is failing for dots

* Fix encoding snowflake ID for object identifiers

* Fix linter complaints

* Add acceptance test

* Add issue description

* Add tests for nil and pointer

* Fix type
  • Loading branch information
@sfc-gh-asawicki
sfc-gh-asawicki authored Dec 12, 2023
1 parent 581d75c commit 1c98a80
Show file tree
Hide file tree
Showing 6 changed files with 188 additions and 22 deletions.
27 changes: 23 additions & 4 deletions pkg/helpers/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,29 @@ func EncodeSnowflakeID(attributes ...interface{}) string {
// is attribute already an object identifier?
if len(attributes) == 1 {
if id, ok := attributes[0].(sdk.ObjectIdentifier); ok {
// remove quotes and replace dots with pipes
parts := strings.Split(id.FullyQualifiedName(), ".")
for i, part := range parts {
parts[i] = strings.Trim(part, `"`)
if val := reflect.ValueOf(id); val.Kind() == reflect.Ptr && val.IsNil() {
log.Panicf("Nil object identifier received")
}
parts := make([]string, 0)
switch v := id.(type) {
case sdk.AccountObjectIdentifier:
parts = append(parts, v.Name())
case *sdk.AccountObjectIdentifier:
parts = append(parts, v.Name())
case sdk.DatabaseObjectIdentifier:
parts = append(parts, v.DatabaseName(), v.Name())
case *sdk.DatabaseObjectIdentifier:
parts = append(parts, v.DatabaseName(), v.Name())
case sdk.SchemaObjectIdentifier:
parts = append(parts, v.DatabaseName(), v.SchemaName(), v.Name())
case *sdk.SchemaObjectIdentifier:
parts = append(parts, v.DatabaseName(), v.SchemaName(), v.Name())
case sdk.TableColumnIdentifier:
parts = append(parts, v.DatabaseName(), v.SchemaName(), v.TableName(), v.Name())
case *sdk.TableColumnIdentifier:
parts = append(parts, v.DatabaseName(), v.SchemaName(), v.TableName(), v.Name())
default:
log.Panicf("Unsupported object identifier: %v", id)
}
return strings.Join(parts, IDDelimiter)
}
Expand Down
114 changes: 114 additions & 0 deletions pkg/helpers/helpers_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package helpers

import (
"fmt"
"testing"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -71,3 +73,115 @@ func TestDecodeSnowflakeParameterID(t *testing.T) {
require.Errorf(t, err, "incompatible identifier: %s", id)
})
}

// TODO: add tests for non object identifiers
func TestEncodeSnowflakeID(t *testing.T) {
testCases := map[string]struct {
identifier sdk.ObjectIdentifier
expectedEncodedID string
}{
"encodes account object identifier": {
identifier: sdk.NewAccountObjectIdentifier("database"),
expectedEncodedID: `database`,
},
"encodes quoted account object identifier": {
identifier: sdk.NewAccountObjectIdentifier("\"database\""),
expectedEncodedID: `database`,
},
"encodes account object identifier with a dot": {
identifier: sdk.NewAccountObjectIdentifier("data.base"),
expectedEncodedID: `data.base`,
},
"encodes pointer to account object identifier": {
identifier: sdk.Pointer(sdk.NewAccountObjectIdentifier("database")),
expectedEncodedID: `database`,
},
"encodes database object identifier": {
identifier: sdk.NewDatabaseObjectIdentifier("database", "schema"),
expectedEncodedID: `database|schema`,
},
"encodes quoted database object identifier": {
identifier: sdk.NewDatabaseObjectIdentifier("\"database\"", "\"schema\""),
expectedEncodedID: `database|schema`,
},
"encodes database object identifier with dots": {
identifier: sdk.NewDatabaseObjectIdentifier("data.base", "sche.ma"),
expectedEncodedID: `data.base|sche.ma`,
},
"encodes pointer to database object identifier": {
identifier: sdk.Pointer(sdk.NewDatabaseObjectIdentifier("database", "schema")),
expectedEncodedID: `database|schema`,
},
"encodes schema object identifier": {
identifier: sdk.NewSchemaObjectIdentifier("database", "schema", "table"),
expectedEncodedID: `database|schema|table`,
},
"encodes quoted schema object identifier": {
identifier: sdk.NewSchemaObjectIdentifier("\"database\"", "\"schema\"", "\"table\""),
expectedEncodedID: `database|schema|table`,
},
"encodes schema object identifier with dots": {
identifier: sdk.NewSchemaObjectIdentifier("data.base", "sche.ma", "tab.le"),
expectedEncodedID: `data.base|sche.ma|tab.le`,
},
"encodes pointer to schema object identifier": {
identifier: sdk.Pointer(sdk.NewSchemaObjectIdentifier("database", "schema", "table")),
expectedEncodedID: `database|schema|table`,
},
"encodes table column identifier": {
identifier: sdk.NewTableColumnIdentifier("database", "schema", "table", "column"),
expectedEncodedID: `database|schema|table|column`,
},
"encodes quoted table column identifier": {
identifier: sdk.NewTableColumnIdentifier("\"database\"", "\"schema\"", "\"table\"", "\"column\""),
expectedEncodedID: `database|schema|table|column`,
},
"encodes table column identifier with dots": {
identifier: sdk.NewTableColumnIdentifier("data.base", "sche.ma", "tab.le", "col.umn"),
expectedEncodedID: `data.base|sche.ma|tab.le|col.umn`,
},
"encodes pointer to table column identifier": {
identifier: sdk.Pointer(sdk.NewTableColumnIdentifier("database", "schema", "table", "column")),
expectedEncodedID: `database|schema|table|column`,
},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
encodedID := EncodeSnowflakeID(tc.identifier)
require.Equal(t, tc.expectedEncodedID, encodedID)
})
}

t.Run("panics for unsupported object identifier", func(t *testing.T) {
id := unsupportedObjectIdentifier{}
require.PanicsWithValue(t, fmt.Sprintf("Unsupported object identifier: %v", id), func() {
EncodeSnowflakeID(id)
})
})

nilTestCases := []any{
(*sdk.AccountObjectIdentifier)(nil),
(*sdk.DatabaseObjectIdentifier)(nil),
(*sdk.SchemaObjectIdentifier)(nil),
(*sdk.TableColumnIdentifier)(nil),
}

for i, tt := range nilTestCases {
t.Run(fmt.Sprintf("handle nil pointer to object identifier %d", i), func(t *testing.T) {
require.PanicsWithValue(t, "Nil object identifier received", func() {
EncodeSnowflakeID(tt)
})
})
}
}

type unsupportedObjectIdentifier struct{}

func (i unsupportedObjectIdentifier) Name() string {
return "name"
}

func (i unsupportedObjectIdentifier) FullyQualifiedName() string {
return "fully qualified name"
}
20 changes: 10 additions & 10 deletions pkg/resources/testdata/TestAcc_ExternalTable_basic/test.tf
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
resource "snowflake_storage_integration" "i" {
name = var.name
name = var.name
storage_allowed_locations = [var.location]
storage_provider = "S3"
storage_aws_role_arn = var.aws_arn
storage_provider = "S3"
storage_aws_role_arn = var.aws_arn
}

resource "snowflake_stage" "test" {
name = var.name
url = var.location
database = var.database
schema = var.schema
name = var.name
url = var.location
database = var.database
schema = var.schema
storage_integration = snowflake_storage_integration.i.name
}

resource "snowflake_external_table" "test_table" {
name = var.name
name = var.name
database = var.database
schema = var.schema
schema = var.schema
comment = "Terraform acceptance test"
column {
name = "column1"
Expand All @@ -29,5 +29,5 @@ resource "snowflake_external_table" "test_table" {
as = "($1:\"CreatedDate\"::timestamp)"
}
file_format = "TYPE = CSV"
location = "@\"${var.database}\".\"${var.schema}\".\"${snowflake_stage.test.name}\""
location = "@\"${var.database}\".\"${var.schema}\".\"${snowflake_stage.test.name}\""
}
26 changes: 26 additions & 0 deletions pkg/resources/user_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,3 +162,29 @@ resource "snowflake_user" "w" {
log.Printf("[DEBUG] s2 %s", s)
return fmt.Sprintf(s, prefix, prefix)
}

// TestAcc_User_issue2058 proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2058 issue.
// The problem was with a dot in user identifier.
// Before the fix it results in panic: interface conversion: sdk.ObjectIdentifier is sdk.DatabaseObjectIdentifier, not sdk.AccountObjectIdentifier error.
func TestAcc_User_issue2058(t *testing.T) {
r := require.New(t)
prefix := "tst-terraform" + strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + "user.123"
sshkey1, err := testhelpers.Fixture("userkey1")
r.NoError(err)
sshkey2, err := testhelpers.Fixture("userkey2")
r.NoError(err)

resource.Test(t, resource.TestCase{
Providers: acc.TestAccProviders(),
PreCheck: func() { acc.TestAccPreCheck(t) },
CheckDestroy: nil,
Steps: []resource.TestStep{
{
Config: uConfig(prefix, sshkey1, sshkey2),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_user.w", "name", prefix),
),
},
},
})
}
11 changes: 9 additions & 2 deletions pkg/sdk/identifier_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ type AccountObjectIdentifier struct {
}

func NewAccountObjectIdentifier(name string) AccountObjectIdentifier {
return AccountObjectIdentifier{name: name}
return AccountObjectIdentifier{
name: strings.Trim(name, `"`),
}
}

func NewAccountObjectIdentifierFromFullyQualifiedName(fullyQualifiedName string) AccountObjectIdentifier {
Expand Down Expand Up @@ -268,7 +270,12 @@ type TableColumnIdentifier struct {
}

func NewTableColumnIdentifier(databaseName, schemaName, tableName, columnName string) TableColumnIdentifier {
return TableColumnIdentifier{databaseName: databaseName, schemaName: schemaName, tableName: tableName, columnName: columnName}
return TableColumnIdentifier{
databaseName: strings.Trim(databaseName, `"`),
schemaName: strings.Trim(schemaName, `"`),
tableName: strings.Trim(tableName, `"`),
columnName: strings.Trim(columnName, `"`),
}
}

func NewTableColumnIdentifierFromFullyQualifiedName(fullyQualifiedName string) TableColumnIdentifier {
Expand Down
12 changes: 6 additions & 6 deletions pkg/sdk/privileges.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,12 @@ const (

// -- For ICEBERG TABLE
SchemaObjectPrivilegeApplyBudget SchemaObjectPrivilege = "APPLYBUDGET"
//SchemaObjectPrivilegeDelete SchemaObjectPrivilege = "DELETE" (duplicate)
//SchemaObjectPrivilegeInsert SchemaObjectPrivilege = "INSERT" (duplicate)
//SchemaObjectPrivilegeReferences SchemaObjectPrivilege = "REFERENCES" (duplicate)
//SchemaObjectPrivilegeSelect SchemaObjectPrivilege = "SELECT" (duplicate)
//SchemaObjectPrivilegeTruncate SchemaObjectPrivilege = "Truncate" (duplicate)
//SchemaObjectPrivilegeUpdate SchemaObjectPrivilege = "Update" (duplicate)
// SchemaObjectPrivilegeDelete SchemaObjectPrivilege = "DELETE" (duplicate)
// SchemaObjectPrivilegeInsert SchemaObjectPrivilege = "INSERT" (duplicate)
// SchemaObjectPrivilegeReferences SchemaObjectPrivilege = "REFERENCES" (duplicate)
// SchemaObjectPrivilegeSelect SchemaObjectPrivilege = "SELECT" (duplicate)
// SchemaObjectPrivilegeTruncate SchemaObjectPrivilege = "Truncate" (duplicate)
// SchemaObjectPrivilegeUpdate SchemaObjectPrivilege = "Update" (duplicate)

// -- For PIPE
// { MONITOR | OPERATE } [ , ... ]
Expand Down

0 comments on commit 1c98a80

Please sign in to comment.