Skip to content

Commit

Permalink
bump versions of all dependencies and fix sqlx issue
Browse files Browse the repository at this point in the history
Bump versions of dependencies in go.mod files to the latest offered.
Switch to use Go 1.16.
Fix long-standing parameter binding issue with sqlx. The new sqlx.BindDriver
function added to a recent sqlx release enables the fix.
  • Loading branch information
andy-kimball committed Aug 15, 2021
1 parent 0d1c754 commit bd52ae9
Show file tree
Hide file tree
Showing 10 changed files with 176 additions and 194 deletions.
41 changes: 3 additions & 38 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ package copyist
import (
"context"
"database/sql/driver"
"strings"

"github.com/jmoiron/sqlx"
)

// proxyConn records and plays back calls to driver.Conn methods.
Expand Down Expand Up @@ -77,9 +74,6 @@ func (c *proxyConn) ExecContext(
ctx context.Context, query string, args []driver.NamedValue,
) (driver.Result, error) {
if IsRecording() {
originalQuery := query
query = c.maybeRebind(query)

var res driver.Result
var err error
switch t := c.conn.(type) {
Expand All @@ -96,7 +90,7 @@ func (c *proxyConn) ExecContext(
return nil, driver.ErrSkip
}

currentSession.AddRecord(&record{Typ: ConnExec, Args: recordArgs{originalQuery, err}})
currentSession.AddRecord(&record{Typ: ConnExec, Args: recordArgs{query, err}})
if err != nil {
return nil, err
}
Expand All @@ -121,9 +115,6 @@ func (c *proxyConn) Prepare(query string) (driver.Stmt, error) {
// it must not store the context within the statement itself.
func (c *proxyConn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) {
if IsRecording() {
originalQuery := query
query = c.maybeRebind(query)

var stmt driver.Stmt
var err error
if prepCtx, ok := c.conn.(driver.ConnPrepareContext); ok {
Expand All @@ -132,7 +123,7 @@ func (c *proxyConn) PrepareContext(ctx context.Context, query string) (driver.St
stmt, err = c.conn.Prepare(query)
}

currentSession.AddRecord(&record{Typ: ConnPrepare, Args: recordArgs{originalQuery, err}})
currentSession.AddRecord(&record{Typ: ConnPrepare, Args: recordArgs{query, err}})
if err != nil {
return nil, err
}
Expand All @@ -155,9 +146,6 @@ func (c *proxyConn) QueryContext(
ctx context.Context, query string, args []driver.NamedValue,
) (driver.Rows, error) {
if IsRecording() {
originalQuery := query
query = c.maybeRebind(query)

var rows driver.Rows
var err error
switch t := c.conn.(type) {
Expand All @@ -174,7 +162,7 @@ func (c *proxyConn) QueryContext(
return nil, driver.ErrSkip
}

currentSession.AddRecord(&record{Typ: ConnQuery, Args: recordArgs{originalQuery, err}})
currentSession.AddRecord(&record{Typ: ConnQuery, Args: recordArgs{query, err}})
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -251,26 +239,3 @@ func (c *proxyConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.
}
return &proxyTx{}, nil
}

// TODO(andyk): This is a hack that works around problems with the sqlx
// library's named args. sqlx uses a hardcoded list of driver names to
// determine how to represent parameters in prepared queries. For example,
// postgres uses $1, mysql uses ?, sqlserver uses @, and so on. But since
// copyist defines a custom driver name, sqlx falls back to the default ?,
// which won't work with some databases. These issues describe the "custom
// driver" problem:
//
// https://github.com/jmoiron/sqlx/issues/400
// https://github.com/jmoiron/sqlx/issues/559
//
// Workaround this problem by rebinding the query if the bind type of the inner
// driver is different than the default ? character.
func (c *proxyConn) maybeRebind(query string) string {
// NOTE: This doesn't work in cases where the parameter character is
// in a quoted string, etc. Unfortunately, there's not much to be done.
bindType := sqlx.BindType(c.driver.driverName)
if bindType != sqlx.QUESTION && strings.IndexByte(query, '?') != -1 {
query = sqlx.Rebind(bindType, query)
}
return query
}
17 changes: 14 additions & 3 deletions copyist.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"runtime"
"strings"
"testing"

"github.com/jmoiron/sqlx"
)

// recordFlag instructs copyist to record all calls to the registered driver, if
Expand Down Expand Up @@ -98,11 +100,20 @@ func Register(driverName string) {
panic(fmt.Errorf("Register called twice for driver %s", driverName))
}

driver := &proxyDriver{driverName: driverName}
registered[driverName] = driver
copyistDriver := &proxyDriver{driverName: driverName}
registered[driverName] = copyistDriver

// sqlx uses a default list of driver names to determine how to represent
// parameters in prepared queries. For example, postgres uses $1, mysql
// uses ?, sqlserver uses @, and so on. But since copyist defines a custom
// driver name, sqlx falls back to the default ?, which won't work with some
// databases. Register the copyist driver name with sqlx and tell it to use
// the bind type of the underlying driver rather than the default ?.
copyistDriverName := copyistDriverName(driverName)
sqlx.BindDriver(copyistDriverName, sqlx.BindType(driverName))

// Register the copyist driver with the `sql` package.
sql.Register(copyistDriverName(driverName), driver)
sql.Register(copyistDriverName, copyistDriver)
}

// SetSessionInit sets the callback function that will be invoked at the
Expand Down
4 changes: 2 additions & 2 deletions drivertest/commontest/testdata/common_test.copyist
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
4=RowsNext 11:[2:"Andy"] 1:nil
5=ConnQuery 2:"SHOW session_id" 1:nil
6=RowsColumns 9:["session_id"]
7=RowsNext 11:[2:"1670ad6136d1f5460000000000000001"] 1:nil
7=RowsNext 11:[2:"169b031ac1c71b280000000000000001"] 1:nil
8=RowsNext 11:[] 7:EOF
9=RowsNext 11:[2:"1670ad613b4121d80000000000000001"] 1:nil
9=RowsNext 11:[2:"169b031ad2b390880000000000000001"] 1:nil

"TestIndirectOpen"=1,2,3,4
"TestPooling/ensure_connections_are_pooled_within_same_copyist_session"=1,5,6,7,8,5,6,7,8
Expand Down
12 changes: 6 additions & 6 deletions drivertest/go.mod
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
module github.com/cockroachdb/copyist/drivertest

go 1.15
go 1.16

// Use separate go.mod file so that importers of copyist do not need to deal
// with testing driver dependencies and "replace" directives in their own go.mod
// files.
// with driver dependencies used only for testing copyist or the "replace"
// directives, in their own go.mod files.
require (
github.com/cockroachdb/copyist v0.0.0-00010101000000-000000000000
github.com/fortytw2/leaktest v1.3.0
github.com/jackc/pgx/v4 v4.10.1
github.com/jmoiron/sqlx v1.3.1
github.com/lib/pq v1.7.0
github.com/jackc/pgx/v4 v4.13.0
github.com/jmoiron/sqlx v1.3.4
github.com/lib/pq v1.10.2
github.com/lib/pq/old v0.0.0-00010101000000-000000000000
github.com/stretchr/testify v1.7.0
)
Expand Down
Loading

0 comments on commit bd52ae9

Please sign in to comment.