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

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

Merged
merged 5 commits into from
Aug 18, 2022
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
16 changes: 11 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,28 @@ for rows.Next() {

## Statements

Statements support follows the official [Google Cloud Spanner Go](https://pkg.go.dev/cloud.google.com/go/spanner) client style arguments.
Statements support follows the official [Google Cloud Spanner Go](https://pkg.go.dev/cloud.google.com/go/spanner) client style arguments as well as positional paramaters.

### Using positional patameter

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

Choose a reason for hiding this comment

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

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

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

db.ExecContext(ctx, "INSERT INTO tweets (id, text, rts) VALUES (?, ?, ?)", id, text, 10000)
```

db.ExecContext(ctx, "INSERT INTO tweets (id, text, rts) VALUES (@id, @text, @rts)", id, text, 10000)
### Using named patameter

```go
db.ExecContext(ctx, "DELETE FROM tweets WHERE id = @id", 14544498215374)
```

## Transactions

- Read-write transactions always uses the strongest isolation level and ignore the user-specified level.
- Read-only transactions do strong-reads by default. Read-only transactions must be ended by calling
either Commit or Rollback. Calling either of these methods will end the current read-only
transaction and return the session that is used to the session pool.
either Commit or Rollback. Calling either of these methods will end the current read-only
transaction and return the session that is used to the session pool.

``` go
tx, err := db.BeginTx(ctx, &sql.TxOptions{}) // Read-write transaction.
Expand Down
4 changes: 2 additions & 2 deletions driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -737,11 +737,11 @@ func (c *conn) Prepare(query string) (driver.Stmt, error) {
}

func (c *conn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) {
args, err := parseNamedParameters(query)
parsedSQL, args, err := parseParameters(query)
if err != nil {
return nil, err
}
return &stmt{conn: c, query: query, numArgs: len(args)}, nil
return &stmt{conn: c, query: parsedSQL, numArgs: len(args)}, nil
}

func (c *conn) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error) {
Expand Down
4 changes: 2 additions & 2 deletions examples/transactions/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func transaction(projectId, instanceId, databaseId string) error {
}

// The row that we inserted will be readable for the same transaction that started it.
rows, err := tx.QueryContext(ctx, "SELECT SingerId, Name FROM Singers WHERE SingerId = @id", 123)
rows, err := tx.QueryContext(ctx, "SELECT SingerId, Name FROM Singers WHERE SingerId = ?", 123)
if err != nil {
_ = tx.Rollback()
return err
Expand Down Expand Up @@ -92,7 +92,7 @@ func transaction(projectId, instanceId, databaseId string) error {
}

// This should now find the row.
row = db.QueryRowContext(ctx, "SELECT SingerId, Name FROM Singers WHERE SingerId = @id", 123)
row = db.QueryRowContext(ctx, "SELECT SingerId, Name FROM Singers WHERE SingerId = ?", 123)
if err := row.Err(); err != nil {
return err
}
Expand Down
63 changes: 50 additions & 13 deletions statement_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"reflect"
"regexp"
"strconv"
"strings"
"sync"
"unicode"
Expand All @@ -45,18 +46,19 @@ func union(m1 map[string]bool, m2 map[string]bool) map[string]bool {
return res
}

// parseNamedParameters returns the named parameters in the given sql string.
// parseParameters returns the parameters in the given sql string, if the input
// sql contains positional parameters it returns the converted sql string with
// all positional parameters replaced with named parameters.
// 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 containing for example an email address ('[email protected]') will be
// recognized as a string literal and not returned as a named parameter.
func parseNamedParameters(sql string) ([]string, error) {
func parseParameters(sql string) (string, []string, error) {
sql, err := removeCommentsAndTrim(sql)
if err != nil {
return nil, err
return sql, nil, err
}
sql = removeStatementHint(sql)
return findParams(sql)
return findParams('?', sql)
}

// RemoveCommentsAndTrim removes any comments in the query string and trims any
Expand Down Expand Up @@ -188,9 +190,9 @@ func removeStatementHint(sql string) string {
return sql
}

// This function assumes that all comments and statement hints have already
// This function assumes that all comments have already
// been removed from the statement.
func findParams(sql string) ([]string, error) {
func findParams(positionalParamChar rune, sql string) (string, []string, error) {
const paramPrefix = '@'
const singleQuote = '\''
const doubleQuote = '"'
Expand All @@ -199,14 +201,19 @@ func findParams(sql string) ([]string, error) {
var startQuote rune
lastCharWasEscapeChar := false
isTripleQuoted := false
res := make([]string, 0)
hasNamedParameter := false
hasPositionalParameter := false
namedParams := make([]string, 0)
parsedSQL := strings.Builder{}
parsedSQL.Grow(len(sql))
positionalParameterIndex := 1
index := 0
runes := []rune(sql)
for index < len(runes) {
c := runes[index]
if isInQuoted {
if (c == '\n' || c == '\r') && !isTripleQuoted {
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))
} else if c == startQuote {
if lastCharWasEscapeChar {
lastCharWasEscapeChar = false
Expand All @@ -215,6 +222,8 @@ func findParams(sql string) ([]string, error) {
isInQuoted = false
startQuote = 0
isTripleQuoted = false
parsedSQL.WriteRune(c)
parsedSQL.WriteRune(c)
index += 2
}
} else {
Expand All @@ -226,41 +235,69 @@ func findParams(sql string) ([]string, error) {
} else {
lastCharWasEscapeChar = false
}
parsedSQL.WriteRune(c)
} else {
// We are not in a quoted string. It's a parameter if it is an '@' followed by a letter or an underscore.
// See https://cloud.google.com/spanner/docs/lexical#identifiers for identifier rules.
if c == paramPrefix && len(runes) > index+1 && (unicode.IsLetter(runes[index+1]) || runes[index+1] == '_') {
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if !(unicode.IsLetter(runes[index]) || unicode.IsDigit(runes[index]) || runes[index] == '_') {
res = append(res, string(runes[startIndex:index]))
hasNamedParameter = true
namedParams = append(namedParams, string(runes[startIndex:index]))
parsedSQL.WriteRune(runes[index])
break
}
if index == len(runes)-1 {
res = append(res, string(runes[startIndex:]))
hasNamedParameter = true
namedParams = append(namedParams, string(runes[startIndex:]))
parsedSQL.WriteRune(runes[index])
break
}
parsedSQL.WriteRune(runes[index])
index++
}
} else if c == positionalParamChar {
if hasNamedParameter {
return sql, nil, spanner.ToSpannerError(status.Errorf(codes.InvalidArgument, "statement must not contain both named and positional parameter: %s", sql))
}
hasPositionalParameter = true
parsedSQL.WriteString("@p" + strconv.Itoa(positionalParameterIndex))
namedParams = append(namedParams, "p"+strconv.Itoa(positionalParameterIndex))
positionalParameterIndex++
} else {
if c == singleQuote || c == doubleQuote || c == backtick {
isInQuoted = true
startQuote = c
// Check whether it is a triple-quote.
if len(runes) > index+2 && runes[index+1] == startQuote && runes[index+2] == startQuote {
isTripleQuoted = true
parsedSQL.WriteRune(c)
parsedSQL.WriteRune(c)
index += 2
}
}
parsedSQL.WriteRune(c)
}
}
index++
}
if isInQuoted {
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))
}
if hasNamedParameter {
return sql, namedParams, nil
}
sql = strings.TrimSpace(parsedSQL.String())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

if len(sql) > 0 && sql[len(sql)-1] == ';' {
sql = sql
}
return res, nil
return sql, namedParams, nil
}

// isDDL returns true if the given sql string is a DDL statement.
Expand Down
Loading