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.

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

fix: Fix parsing text in view, check parenthesis in ParseSchemaObjectIdentifierWithArguments #3102

Merged
merged 5 commits into from
Oct 3, 2024
Merged

fix: Fix parsing text in view, check parenthesis in ParseSchemaObjectIdentifierWithArguments #3102

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
beee8a5
Minor fixes
sfc-gh-jmichalak Sep 24, 2024
e8a82da
pre push
sfc-gh-jmichalak Sep 24, 2024
2bd2493
Fix
sfc-gh-jmichalak Sep 24, 2024
4d27d0a
Merge remote-tracking branch 'origin/main' into minor-fixes
sfc-gh-jmichalak Oct 2, 2024
b288c28
Minor improvements
sfc-gh-jmichalak Oct 2, 2024
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: 2 additions & 0 deletions docs/resources/view.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ description: |-

!> **Note about copy_grants** Fields like `is_recursive`, `is_temporary`, `copy_grants` and `statement` can not be ALTERed on Snowflake side (check [docs](https://docs.snowflake.com/en/sql-reference/sql/alter-view)), and a change means recreation of the resource. ForceNew can not be used because it does not preserve grants from `copy_grants`. Beware that even though a change is marked as update, the resource is recreated.

!> Due to Snowflake limitations, to properly compute diff on `statement` field, the provider parses a `text` field which contains the whole CREATE query used to create the resource. We recommend not using special characters, especially `(`, `,`, `)` in any of the fields, if possible.

~> **Required warehouse** For this resource, the provider uses [policy references](https://docs.snowflake.com/en/sql-reference/functions/policy_references) which requires a warehouse in the connection. Please, make sure you have either set a DEFAULT_WAREHOUSE for the user, or specified a warehouse in the provider configuration.

# snowflake_view (Resource)
Expand Down
1 change: 0 additions & 1 deletion pkg/datasources/parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ func ReadParameters(d *schema.ResourceData, meta interface{}) error {
}
}
parameters, err = client.Parameters.ShowParameters(ctx, &opts)

if err != nil {
return fmt.Errorf("error listing parameters: %w", err)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/sdk/identifier_parsers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package sdk
import (
"bytes"
"encoding/csv"
"errors"
"fmt"
"slices"
"strconv"
Expand Down Expand Up @@ -152,6 +153,9 @@ func ParseExternalObjectIdentifier(identifier string) (ExternalObjectIdentifier,

func ParseSchemaObjectIdentifierWithArguments(fullyQualifiedName string) (SchemaObjectIdentifierWithArguments, error) {
splitIdIndex := strings.IndexRune(fullyQualifiedName, '(')
if splitIdIndex == -1 {
return SchemaObjectIdentifierWithArguments{}, errors.New("unable to parse identifier: '(' not present")
}
parts, err := ParseIdentifierString(fullyQualifiedName[:splitIdIndex])
if err != nil {
return SchemaObjectIdentifierWithArguments{}, err
Expand Down
1 change: 1 addition & 0 deletions pkg/sdk/identifier_parsers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ func TestNewSchemaObjectIdentifierWithArgumentsFromFullyQualifiedName_WithRawInp
{RawInput: `abc.def.ghi(FLOAT, 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`},
}

for _, testCase := range testCases {
Expand Down
42 changes: 29 additions & 13 deletions pkg/snowflake/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,18 @@ func (e *ViewSelectStatementExtractor) consumeColumn() (isLast bool) {
e.consumeID()
if e.input[e.pos-1] == ')' {
isLast = true
return
}
e.consumeSpace()

ok := e.consumeToken("projection policy")
if ok {
e.consumeSpace()
e.consumeID()
if e.input[e.pos-1] == ')' {
isLast = true
e.consumeSpace()
return
}
e.consumeSpace()
}
Expand All @@ -95,13 +99,24 @@ func (e *ViewSelectStatementExtractor) consumeColumn() (isLast bool) {
e.consumeSpace()
e.consumeID()
e.consumeSpace()
e.consumeToken("using")
e.consumeSpace()
e.consumeIdentifierList()
if string(e.input[e.pos-2:e.pos]) == "))" {
isLast = true
if ok := e.consumeToken("using"); ok {
e.consumeSpace()
e.consumeIdentifierList()
if string(e.input[e.pos-3:e.pos-1]) == "))" {
isLast = true
e.consumeSpace()
return
}
e.consumeSpace()
}
e.consumeSpace()
}

e.consumeQuotedParameter("comment", false)
e.consumeSpace()
e.consumeToken(",")
if e.input[e.pos] == ')' {
e.consumeToken(")")
isLast = true
}
return
}
Expand Down Expand Up @@ -201,7 +216,7 @@ func (e *ViewSelectStatementExtractor) ExtractDynamicTable() (string, error) {
e.consumeSpace()
e.consumeComment()
e.consumeSpace()
e.consumeQuotedParameter("lag")
e.consumeQuotedParameter("lag", true)
e.consumeSpace()
e.consumeTokenParameter("warehouse")
e.consumeSpace()
Expand Down Expand Up @@ -266,22 +281,23 @@ func (e *ViewSelectStatementExtractor) consumeNonSpace() {
}

func (e *ViewSelectStatementExtractor) consumeComment() {
e.consumeQuotedParameter("comment")
e.consumeQuotedParameter("comment", true)
}

func (e *ViewSelectStatementExtractor) consumeQuotedParameter(param string) {
func (e *ViewSelectStatementExtractor) consumeQuotedParameter(param string, withEquals bool) {
if c := e.consumeToken(param); !c {
return
}

e.consumeSpace()

if c := e.consumeToken("="); !c {
return
if withEquals {
if c := e.consumeToken("="); !c {
return
}
e.consumeSpace()
}

e.consumeSpace()

if c := e.consumeToken("'"); !c {
return
}
Expand Down
21 changes: 16 additions & 5 deletions pkg/snowflake/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ from bar;`
issue2640 := `CREATE OR REPLACE SECURE VIEW "CLASSIFICATION" comment = 'Classification View of the union of classification tables' AS select * from AB1_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION union select * from AB2_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION`
withRowAccessAndAggregationPolicy := `CREATE SECURE VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
withRowAccessAndAggregationPolicyWithEntityKey := `CREATE SECURE VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
allFields := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id MASKING POLICY mp USING ("col1", "cond1") PROJECTION POLICY pp COMMENT = 'asdf', foo MASKING POLICY mp USING ("col1", "cond1")) COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
columnsListEndingWithMaskingPolicyWithoutUsing := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id PROJECTION POLICY pp MASKING POLICY mp COMMENT 'a (s) df', foo MASKING POLICY pp) COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
columnsListEndingWithMaskingPolicyWithUsing := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id PROJECTION POLICY pp MASKING POLICY mp COMMENT 'a (s) df', foo MASKING POLICY pp USING ("col1")) COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
columnsListEndingWithProjectionPolicy := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id PROJECTION POLICY pp MASKING POLICY mp COMMENT 'a (s) df' , foo PROJECTION POLICY pp) COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
columnsListEndingWithComment := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id PROJECTION POLICY pp MASKING POLICY mp COMMENT 'asdf', foo PROJECTION POLICY pp COMMENT 'foo (bar) hoge') COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
columnsListEndingWithID := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" ("ID", "FOO") COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
allFields := `CREATE OR REPLACE SECURE TEMPORARY VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" (id PROJECTION POLICY pp MASKING POLICY mp USING ("col1", "cond1") COMMENT 'asdf', foo MASKING POLICY mp USING ("col1", "cond1")) COMMENT = 'Terraform test resource' ROW ACCESS policy rap on (title, title2) AGGREGATION POLICY rap ENTITY KEY (foo, bar) AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES`
testStatement := "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES"
type args struct {
input string
}
Expand All @@ -61,11 +67,16 @@ from bar;`
{"comment", args{comment}, "select * from bar;", false},
{"commentEscape", args{commentEscape}, "select * from bar;", false},
{"identifier", args{identifier}, "select * from bar;", false},
{"full", args{full}, "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES", false},
{"full", args{full}, testStatement, false},
{"issue2640", args{issue2640}, "select * from AB1_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION union select * from AB2_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION", false},
{"with row access policy and aggregation policy", args{withRowAccessAndAggregationPolicy}, "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES", false},
{"with row access policy and aggregation policy with entity key", args{withRowAccessAndAggregationPolicyWithEntityKey}, "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES", false},
{"all fields", args{allFields}, "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES", false},
{"with row access policy and aggregation policy", args{withRowAccessAndAggregationPolicy}, testStatement, false},
{"with row access policy and aggregation policy with entity key", args{withRowAccessAndAggregationPolicyWithEntityKey}, testStatement, false},
{"with column list ending with masking policy without using", args{columnsListEndingWithMaskingPolicyWithoutUsing}, testStatement, false},
{"with column list ending with masking policy with using", args{columnsListEndingWithMaskingPolicyWithUsing}, testStatement, false},
{"with column list ending with projection using", args{columnsListEndingWithProjectionPolicy}, testStatement, false},
{"with column list ending with comment", args{columnsListEndingWithComment}, testStatement, false},
{"with column list ending with column name", args{columnsListEndingWithID}, testStatement, false},
{"all fields", args{allFields}, testStatement, false},
}
for _, tt := range tests {
tt := tt
Expand Down
2 changes: 2 additions & 0 deletions templates/resources/view.md.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ description: |-

!> **Note about copy_grants** Fields like `is_recursive`, `is_temporary`, `copy_grants` and `statement` can not be ALTERed on Snowflake side (check [docs](https://docs.snowflake.com/en/sql-reference/sql/alter-view)), and a change means recreation of the resource. ForceNew can not be used because it does not preserve grants from `copy_grants`. Beware that even though a change is marked as update, the resource is recreated.

!> Due to Snowflake limitations, to properly compute diff on `statement` field, the provider parses a `text` field which contains the whole CREATE query used to create the resource. We recommend not using special characters, especially `(`, `,`, `)` in any of the fields, if possible.

~> **Required warehouse** For this resource, the provider uses [policy references](https://docs.snowflake.com/en/sql-reference/functions/policy_references) which requires a warehouse in the connection. Please, make sure you have either set a DEFAULT_WAREHOUSE for the user, or specified a warehouse in the provider configuration.

# {{.Name}} ({{.Type}})
Expand Down
Loading
Loading