Skip to content
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.

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

fix: Fix various small problems #2552

Merged
merged 5 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion docs/resources/view.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ SQL
### Optional

- `comment` (String) Specifies a comment for the view.
- `copy_grants` (Boolean) Retains the access permissions from the original view when a new view is created using the OR REPLACE clause.
- `copy_grants` (Boolean) Retains the access permissions from the original view when a new view is created using the OR REPLACE clause. OR REPLACE must be set when COPY GRANTS is set.
- `is_secure` (Boolean) Specifies that the view is secure.
- `or_replace` (Boolean) Overwrites the View if it exists.
- `tag` (Block List, Deprecated) Definitions of a tag to associate with the resource. (see [below for nested schema](#nestedblock--tag))
Expand Down
47 changes: 47 additions & 0 deletions pkg/resources/table_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1537,3 +1537,50 @@ func testAccCheckTableDestroy(s *terraform.State) error {
}
return nil
}

// proves issues https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2110 and https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2495
func TestAcc_Table_ClusterBy(t *testing.T) {
accName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: testAccCheckTableDestroy,
Steps: []resource.TestStep{
{
Config: tableConfigWithComplexClusterBy(accName, acc.TestDatabaseName, acc.TestSchemaName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_table.test_table", "name", accName),
resource.TestCheckResourceAttr("snowflake_table.test_table", "database", acc.TestDatabaseName),
resource.TestCheckResourceAttr("snowflake_table.test_table", "schema", acc.TestSchemaName),
resource.TestCheckResourceAttr("snowflake_table.test_table", "cluster_by.#", "2"),
resource.TestCheckResourceAttr("snowflake_table.test_table", "cluster_by.0", "date_trunc('month', LAST_LOAD_TIME)"),
resource.TestCheckResourceAttr("snowflake_table.test_table", "cluster_by.1", "COL1"),
),
},
},
})
}

func tableConfigWithComplexClusterBy(name string, databaseName string, schemaName string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be moved into testdata directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We did not agree to automatically move every config to a file - it is not necessarily more readable. Currently, we accept both options (they are both not ideal), but we have ideas for the better, third option (maybe coming soon).

return fmt.Sprintf(`
resource "snowflake_table" "test_table" {
name = "%[1]s"
database = "%[2]s"
schema = "%[3]s"
cluster_by = ["date_trunc('month', LAST_LOAD_TIME)", "COL1"]
column {
name = "COL1"
type = "VARCHAR(16777216)"
}
column {
name = "LAST_LOAD_TIME"
type = "TIMESTAMP_LTZ(6)"
nullable = true
}
}
`, name, databaseName, schemaName)
}
3 changes: 2 additions & 1 deletion pkg/resources/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,11 @@ var viewSchema = map[string]*schema.Schema{
Type: schema.TypeBool,
Optional: true,
Default: false,
Description: "Retains the access permissions from the original view when a new view is created using the OR REPLACE clause.",
Description: "Retains the access permissions from the original view when a new view is created using the OR REPLACE clause. OR REPLACE must be set when COPY GRANTS is set.",
DiffSuppressFunc: func(k, oldValue, newValue string, d *schema.ResourceData) bool {
return oldValue != "" && oldValue != newValue
},
RequiredWith: []string{"or_replace"},
},
"is_secure": {
Type: schema.TypeBool,
Expand Down
70 changes: 70 additions & 0 deletions pkg/resources/view_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"fmt"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -378,6 +379,38 @@ func TestAcc_ViewStatementUpdate(t *testing.T) {
})
}

func TestAcc_View_copyGrants(t *testing.T) {
accName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
query := "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES"

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: testAccCheckViewDestroy,
Steps: []resource.TestStep{
{
Config: viewConfigWithCopyGrants(acc.TestDatabaseName, acc.TestSchemaName, accName, query, true),
ExpectError: regexp.MustCompile("all of `copy_grants,or_replace` must be specified"),
},
{
Config: viewConfigWithCopyGrantsAndOrReplace(acc.TestDatabaseName, acc.TestSchemaName, accName, query, true, true),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_view.test", "name", accName),
),
},
{
Config: viewConfigWithOrReplace(acc.TestDatabaseName, acc.TestSchemaName, accName, query, true),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_view.test", "name", accName),
),
},
},
})
}

func viewConfigWithGrants(databaseName string, schemaName string, selectStatement string) string {
return fmt.Sprintf(`
resource "snowflake_table" "table" {
Expand Down Expand Up @@ -430,6 +463,43 @@ data "snowflake_grants" "grants" {
databaseName, schemaName)
}

func viewConfigWithCopyGrants(databaseName string, schemaName string, name string, selectStatement string, copyGrants bool) string {
return fmt.Sprintf(`
resource "snowflake_view" "test" {
name = "%[3]s"
database = "%[1]s"
schema = "%[2]s"
statement = "%[4]s"
copy_grants = %[5]t
}
`, databaseName, schemaName, name, selectStatement, copyGrants)
}

func viewConfigWithCopyGrantsAndOrReplace(databaseName string, schemaName string, name string, selectStatement string, copyGrants bool, orReplace bool) string {
return fmt.Sprintf(`
resource "snowflake_view" "test" {
name = "%[3]s"
database = "%[1]s"
schema = "%[2]s"
statement = "%[4]s"
copy_grants = %[5]t
or_replace = %[6]t
}
`, databaseName, schemaName, name, selectStatement, copyGrants, orReplace)
}

func viewConfigWithOrReplace(databaseName string, schemaName string, name string, selectStatement string, orReplace bool) string {
return fmt.Sprintf(`
resource "snowflake_view" "test" {
name = "%[3]s"
database = "%[1]s"
schema = "%[2]s"
statement = "%[4]s"
or_replace = %[5]t
}
`, databaseName, schemaName, name, selectStatement, orReplace)
}

func testAccCheckViewDestroy(s *terraform.State) error {
db := acc.TestAccProvider.Meta().(*sql.DB)
client := sdk.NewClientFromDB(db)
Expand Down
28 changes: 24 additions & 4 deletions pkg/sdk/tables.go
Original file line number Diff line number Diff line change
Expand Up @@ -564,11 +564,31 @@ func (v *Table) GetClusterByKeys() []string {
}

statementWithoutLinear := strings.TrimSuffix(strings.Replace(v.ClusterBy, "LINEAR(", "", 1), ")")
keysRaw := strings.Split(statementWithoutLinear, ",")
keysClean := make([]string, 0, len(keysRaw))
for _, key := range keysRaw {
keysClean = append(keysClean, strings.TrimSpace(key))
return splitClusterBy(statementWithoutLinear)
}

func splitClusterBy(statementWithoutLinear string) []string {
keysClean := make([]string, 0)

var current string
var open int
for _, character := range statementWithoutLinear {
strChar := string(character)
switch strChar {
case "(":
open++
case ")":
open--
case ",":
if open == 0 {
keysClean = append(keysClean, strings.TrimSpace(current))
current = ""
continue
}
}
current += strChar
}
keysClean = append(keysClean, strings.TrimSpace(current))

return keysClean
}
Expand Down
18 changes: 18 additions & 0 deletions pkg/sdk/tables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1629,4 +1629,22 @@ func TestTable_GetClusterByKeys(t *testing.T) {

assert.Equal(t, []string{"abc", "def"}, table.GetClusterByKeys())
})

t.Run("with function with one param", func(t *testing.T) {
table := Table{ClusterBy: "LINEAR(some_func(some_param),other_param)"}

assert.Equal(t, []string{"some_func(some_param)", "other_param"}, table.GetClusterByKeys())
})

t.Run("with function with more than one param", func(t *testing.T) {
table := Table{ClusterBy: "LINEAR(date_trunc('HOUR',TIMESTAMP_HR),CLIENT_ID)"}

assert.Equal(t, []string{"date_trunc('HOUR',TIMESTAMP_HR)", "CLIENT_ID"}, table.GetClusterByKeys())
})

t.Run("with nested functions", func(t *testing.T) {
table := Table{ClusterBy: "LINEAR(some_func(some_param, some_other_param, other_func(some_param, some_other_param)),other_param)"}

assert.Equal(t, []string{"some_func(some_param, some_other_param, other_func(some_param, some_other_param))", "other_param"}, table.GetClusterByKeys())
})
}
Loading