feat: add support of positional parameter in the queries #110

rahul2393 · 2022-08-12T10:11:34Z

Fixes: #18

olavloite · 2022-08-16T05:26:55Z

.repo-metadata.json

+{
+  "name": "spanner-go-driver",
+  "name_pretty": "Cloud Spanner driver for database/sql package",
+  "product_documentation": "https://pkg.go.dev/github.com/googleapis/go-sql-spanner",
+  "client_documentation": "https://pkg.go.dev/github.com/googleapis/go-sql-spanner",
+  "release_level": "stable",
+  "language": "go",
+  "repo": "googleapis/go-sql-spanner",
+  "distribution_name": "cloud.google.com/go-sql-spanner/spanner",
+  "repo_short": "go-sql-spanner",
+  "library_type": "OTHER",
+  "codeowner_team": "@googleapis/go-sql-spanner"
+}


nit: this seems unrelated to this change and would be better moved to a separate PR

olavloite · 2022-08-16T05:29:46Z

README.md


 ```go
-db.QueryContext(ctx, "SELECT id, text FROM tweets WHERE likes > @likes", 500)


Could we rather split this into two separate examples; one block that only uses named parameters and one that only uses positional parameters?

olavloite · 2022-08-16T05:36:05Z

statement_parser.go

@@ -45,6 +46,76 @@ func union(m1 map[string]bool, m2 map[string]bool) map[string]bool {
 	return res
 }

+// convertPositionalParametersToNamedParameters returns the sql string with all the positional
+// parameters converted to named parameter. The sql string must be a valid Cloud Spanner sql statement.
+// It may contain comments and (string) literals without any restrictions. That is, string literals


I don't this function supports SQL strings with comments.

Suggested change

// It may contain comments and (string) literals without any restrictions. That is, string literals

// It may contain (string) literals without any restrictions. That is, string literals

olavloite · 2022-08-16T05:39:12Z

statement_parser.go

+					startQuote = c
+					// check whether it is a triple-quote
+					if len(runes) > index+2 && runes[index+1] == startQuote && runes[index+2] == startQuote {
+						isTripleQuoted = true


I think you can safely write a couple of more runes and increase the index more here, as you have already determined that they are all quotes.

olavloite · 2022-08-16T05:40:09Z

statement_parser.go

+					lastCharWasEscapeChar = false
+				} else if isTripleQuoted {
+					if len(runes) > index+2 && runes[index+1] == startQuote && runes[index+2] == startQuote {
+						isInQuoted = false


You have already determined that this is the end of a triple-quoted string here, so you can safely write the three runes and update the position instead of executing the same loop for the next two characters.

olavloite · 2022-08-16T05:48:03Z

statement_parser_test.go

+			input: `?'?it\\'?s'?`,
+			want:  `@p1'?it\\'?s'@p2`,


This does not seem correct. To me, the input string literal seems invalid. The double backslash in the string makes it a single backslash to Cloud Spanner. So the actual string literal here is ?it\. The ?s'? part that follows contains an unclosed literal.

olavloite · 2022-08-16T05:54:16Z

statement_parser_test.go

+			input: "?`?it\\\\`?s`?",
+			want:  "@p1`?it\\\\`?s`@p2",


nit: This test would be easier to read and understand if it used backticks to create the Golang string. Now you have to think about which part of the string is an escape to add a special character to the Golang string, and then after that, think about what the same escape character might mean to Cloud Spanner.

olavloite · 2022-08-16T05:54:41Z

statement_parser_test.go

+			err:     spanner.ToSpannerError(status.Errorf(codes.InvalidArgument, "statement contains an unclosed literal: %s", "?'''?it\\'?s \n ?it\\'?s'?")),
+		},
+	}
+	for _, tc := range tests {


This test does not contain any tests for triple-quoted strings.

olavloite · 2022-08-16T05:55:06Z

stmt.go

@@ -65,6 +65,10 @@ func (s *stmt) QueryContext(ctx context.Context, args []driver.NamedValue) (driv
 }

 func prepareSpannerStmt(q string, args []driver.NamedValue) (spanner.Statement, error) {
+	q, err := convertPositionalParametersToNamedParameters('?', q)


Has the statement already been cleared for comments at this point?

It might be good to do a quick check whether the string contains any occurrences of ? at all before sifting through the string searching for positional parameters.

It feels a little inefficient to first search for positional parameters and convert them to named parameters, only to call parseNamedParameters on the next line to loop through the same string once more to determine where those named parameters are.

I don't think we would want to support a mix of named and positional parameters in one SQL string. Would it be an option to only call one of the two functions here (so either convertPositionalParametersToNamedParameters or parseNamedParameters?

olavloite

LGTM with a couple of minor nits and test requests

olavloite · 2022-08-17T12:55:18Z

statement_parser.go

 						res = append(res, string(runes[startIndex:index]))
+						hasNamedParameter = true


nit: double assignment of hasNamedParameter

olavloite · 2022-08-17T12:57:13Z

statement_parser.go

@@ -199,14 +201,19 @@ func findParams(sql string) ([]string, error) {
 	var startQuote rune
 	lastCharWasEscapeChar := false
 	isTripleQuoted := false
+	hasNamedParameter := false
+	hasPositionalParameter := false
 	res := make([]string, 0)


nit: I think it would make sense to rename this variable now that this method returns two results. So maybe something like namedParams?

olavloite · 2022-08-17T12:58:56Z

statement_parser.go

+				if hasPositionalParameter {
+					return sql, nil, spanner.ToSpannerError(status.Errorf(codes.InvalidArgument, "statement must not contain both named and positional parameter: %s", sql))
+				}
+				parsedSQL.WriteRune(c)
 				index++
 				startIndex := index
 				for index < len(runes) {


Hmmm... Looking at this code again I start to wonder whether there might be a bug here (that was already there before this change): What happens if the SQL string contains just a single @? So for example select foo from bar where id=@ order by value. Would you mind adding a test for that?

In this case the returned sql string will be exactly the same with no params, and we expect Cloud Spanner to handle the error similar to what Go client library will do, TL;DR parser won't handle this case.

olavloite · 2022-08-17T13:01:29Z

statement_parser.go

-		return nil, spanner.ToSpannerError(status.Errorf(codes.InvalidArgument, "statement contains an unclosed literal: %s", sql))
+		return sql, nil, spanner.ToSpannerError(status.Errorf(codes.InvalidArgument, "statement contains an unclosed literal: %s", sql))
+	}
+	sql = strings.TrimSpace(parsedSQL.String())


Both for 'safety' and efficiency reasons, I think it would make sense here to only return the parsedSQL value if hasPositionalParameters is true. If hasPositionalParameters is false, we can just return the input sql value.

olavloite · 2022-08-17T13:05:35Z

statement_parser_test.go

+		""" WHERE Name like @name AND Email='[email protected]'`,
+			wantSQL: `SELECT * FROM """strange
+		@table
+		""" WHERE Name like @name AND Email='[email protected]'`,


Would you mind adding a test that has a triple-quoted identifier as the last part of the SQL string. So for example

select * from foo where ?=```strange @table```

Added the test, behaviour is similar to what JDBC spanner will return.

rahul2393 requested a review from a team as a code owner August 12, 2022 10:11

feat: add support of positional parameter in the queries

4978ae3

rahul2393 force-pushed the support-positional-parameters branch from e9426c3 to 4978ae3 Compare August 12, 2022 10:13

rahul2393 requested a review from olavloite August 12, 2022 10:13

olavloite requested changes Aug 16, 2022

View reviewed changes

rahul2393 added 2 commits August 17, 2022 10:40

incorporate requested changes

40284b5

Merge branch 'main' into support-positional-parameters

987bcfc

olavloite approved these changes Aug 17, 2022

View reviewed changes

fixed nits and added tests

df08ddf

rahul2393 force-pushed the support-positional-parameters branch 3 times, most recently from 43828ca to 3f20c19 Compare August 18, 2022 05:26

run test

4eb1eb9

rahul2393 force-pushed the support-positional-parameters branch from 3f20c19 to 4eb1eb9 Compare August 18, 2022 09:32

rahul2393 merged commit a71a457 into main Aug 18, 2022

release-please bot mentioned this pull request Sep 15, 2022

chore(main): release 1.0.0 #122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support of positional parameter in the queries #110

feat: add support of positional parameter in the queries #110

rahul2393 commented Aug 12, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite Aug 16, 2022

olavloite left a comment

olavloite Aug 17, 2022

olavloite Aug 17, 2022

olavloite Aug 17, 2022

rahul2393 Aug 17, 2022

olavloite Aug 17, 2022

olavloite Aug 17, 2022

rahul2393 Aug 17, 2022


		```go
		db.QueryContext(ctx, "SELECT id, text FROM tweets WHERE likes > @likes", 500)

	// It may contain comments and (string) literals without any restrictions. That is, string literals
	// It may contain (string) literals without any restrictions. That is, string literals

		res = append(res, string(runes[startIndex:index]))
		hasNamedParameter = true

feat: add support of positional parameter in the queries #110

feat: add support of positional parameter in the queries #110

Conversation

rahul2393 commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment