Skip to content

Commit

Permalink
Do not read query for dynamic table - final (#2329)
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-asawicki committed Jan 26, 2024
1 parent ea61380 commit 234130d
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 7 deletions.
40 changes: 38 additions & 2 deletions pkg/resources/dynamic_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package resources
import (
"context"
"database/sql"
"errors"
"log"
"strings"

Expand Down Expand Up @@ -190,8 +191,7 @@ func ReadDynamicTable(d *schema.ResourceData, meta interface{}) error {
return err
}
}
text := dynamicTable.Text
if strings.Contains(text, "OR REPLACE") {
if strings.Contains(dynamicTable.Text, "OR REPLACE") {
if err := d.Set("or_replace", true); err != nil {
return err
}
Expand Down Expand Up @@ -242,9 +242,45 @@ func ReadDynamicTable(d *schema.ResourceData, meta interface{}) error {
if err := d.Set("data_timestamp", dynamicTable.DataTimestamp.Format("2006-01-02T16:04:05.000 -0700")); err != nil {
return err
}

query, err := getQueryFromDDL(dynamicTable.Text)
if err != nil {
return err
}
if err := d.Set("query", query); err != nil {
return err
}

return nil
}

/*
* Previous implementation tried to match query part from the whole dynamic table DDL statement by just using `AS`.
* It was failing for table names containing `AS` (like `REASON`). It was also failing for other parts containing `AS`.
* We cannot simply match by ` AS ` because this can still be part of COMMENT or SELECT query itself.
* We have considered not setting the query at all but it was not ideal because of:
* - possible external changes to dynamic table (drop and recreate externally with different query);
* - import not 100% correct.
* We did not want to complicate the implementation too much by introducing parsers.
* One more thing worth mentioning is the whitespace that can be introduced by the user that is still returned by SHOW.
* For now, we just normalize the DDL before extraction of query.
*
* The outcome implementation matches by ` AS SELECT ` and checks the number of matches.
* If more matches are found, the error is returned to inform user about possible cause of error.
*
* Refer to issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2329.
*/
func getQueryFromDDL(text string) (string, error) {
normalizedDDL := normalizeQuery(text)
matchSubstring := " AS SELECT "
matches := strings.Count(strings.ToUpper(normalizedDDL), matchSubstring)
if matches != 1 {
return "", errors.New("too many matches found. There is no way of getting ONLY the 'query' used to create the dynamic table from Snowflake. We try to get it from the whole creation statement but there may be cases where it fails. Please submit the issue on Github (refer to #2329)")
}
idx := strings.Index(strings.ToUpper(normalizedDDL), " AS SELECT ")
return strings.TrimSpace(normalizedDDL[idx+4:]), nil
}

func parseTargetLag(v interface{}) sdk.TargetLag {
var result sdk.TargetLag
tl := v.([]interface{})[0].(map[string]interface{})
Expand Down
47 changes: 42 additions & 5 deletions pkg/resources/dynamic_table_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 @@ -291,11 +292,12 @@ func TestAcc_DynamicTable_issue2329(t *testing.T) {
query := fmt.Sprintf(`select "id" from "%v"."%v"."%v"`, acc.TestDatabaseName, acc.TestSchemaName, tableName)
m := func() map[string]config.Variable {
return map[string]config.Variable{
"name": config.StringVariable(dynamicTableName),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
"query": config.StringVariable(query),
"name": config.StringVariable(dynamicTableName),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
// spaces added on purpose
"query": config.StringVariable(" " + query),
"comment": config.StringVariable("Comment with AS on purpose"),
"table_name": config.StringVariable(tableName),
}
Expand Down Expand Up @@ -330,6 +332,41 @@ func TestAcc_DynamicTable_issue2329(t *testing.T) {
})
}

// TestAcc_DynamicTable_issue2329_with_matching_comment proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2329 issue.
func TestAcc_DynamicTable_issue2329_with_matching_comment(t *testing.T) {
dynamicTableName := strings.ToUpper(acctest.RandStringFromCharSet(4, acctest.CharSetAlpha) + "AS" + acctest.RandStringFromCharSet(4, acctest.CharSetAlpha))
tableName := dynamicTableName + "_table"
query := fmt.Sprintf(`select "id" from "%v"."%v"."%v"`, acc.TestDatabaseName, acc.TestSchemaName, tableName)
m := func() map[string]config.Variable {
return map[string]config.Variable{
"name": config.StringVariable(dynamicTableName),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"warehouse": config.StringVariable(acc.TestWarehouseName),
"query": config.StringVariable(query),
"comment": config.StringVariable("Comment with AS SELECT on purpose"),
"table_name": config.StringVariable(tableName),
}
}

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: testAccCheckDynamicTableDestroy,
Steps: []resource.TestStep{
// If we match more than one time (in this case in comment) we raise an explanation error.
{
ConfigDirectory: acc.ConfigurationDirectory("TestAcc_DynamicTable_issue2329/1"),
ConfigVariables: m(),
ExpectError: regexp.MustCompile(`too many matches found. There is no way of getting ONLY the 'query' used to create the dynamic table from Snowflake. We try to get it from the whole creation statement but there may be cases where it fails. Please submit the issue on Github \(refer to #2329\)`),
},
},
})
}

func testAccCheckDynamicTableDestroy(s *terraform.State) error {
db := acc.TestAccProvider.Meta().(*sql.DB)
client := sdk.NewClientFromDB(db)
Expand Down

0 comments on commit 234130d

Please sign in to comment.