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

database/sql: Tx.ExecContext() no longer calls Stmt.CheckNamedValue() #23820

Closed
aletheia7 opened this issue Feb 14, 2018 · 10 comments
Closed

database/sql: Tx.ExecContext() no longer calls Stmt.CheckNamedValue() #23820

aletheia7 opened this issue Feb 14, 2018 · 10 comments
Assignees
Milestone

Comments

@aletheia7
Copy link

@aletheia7 aletheia7 commented Feb 14, 2018

What version of Go are you using (go version)?

go version go1.10rc2 linux/amd64

Does this issue reproduce with the latest release?

Version Issue?
1.10rc2 yes
1.9.4 no issue

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GORACE=""
GOROOT="/usr/lib/go-1.10"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.10/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-s -w"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build452258374=/tmp/go-build -gno-record-gcc-switches"

Debian Linux Testing/Buster

What did you do?

My internal app calls my sql.Driver. The sql.Tx.ExecContext() calls Stmt.CheckNamedValue() for each argument with 1.9.4. This no longer happens with 1.10rc2.

I wrote the sql.Driver and it's easy for me to insert a debug call into the Stmt.CheckNamedValue().

I have the Debian golang-1.9 and golang-1.10 (sid) installed side-by-side. I can switch to either one by changing a link under /usr/lib/go -> go-1.9 or go-1.10.

tx := db.BeginTx(ctx, &sql.TxOptions{})
tx.ExecContext(ctx, "sql string", sql.Named(), sql.Named(), sql.Named(), sql.Named())

What did you expect to see?

Stmt.CheckedNamedValue() should be called for each argument.

What did you see instead?

CheckedNameValue() is not called.

@aletheia7 aletheia7 changed the title database/sql.Tx.ExecContext() no longer calls Stmt.CheckNamedValue() database/sql: Tx.ExecContext() no longer calls Stmt.CheckNamedValue() Feb 14, 2018
@aletheia7
Copy link
Author

@aletheia7 aletheia7 commented Feb 14, 2018

Confirmed with master 755b36a

@kardianos kardianos self-assigned this Mar 5, 2018
@andybons andybons added the NeedsFix label Mar 7, 2018
@andybons andybons added this to the Go1.11 milestone Mar 7, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

What database driver are you using?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

If your driver has a ExecContext method on your Conn, your CheckNamedValue must be defined on the Conn, not the Stmt.

I've reviewed the database/sql code in question. Everything looks like it will work as intended.

Feel free to post further questions to golang-nuts or https://groups.google.com/forum/#!forum/golang-sql groups.

@kardianos kardianos closed this Mar 16, 2018
@aletheia7
Copy link
Author

@aletheia7 aletheia7 commented Mar 16, 2018

The ExecContext receiver is on the Conn and the Stmt.

@aletheia7
Copy link
Author

@aletheia7 aletheia7 commented Mar 16, 2018

You say "looks like it will work as intended." Did you actually test?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

You said Stmt.CheckNamedValue is not being called. You said ExecContext is defined on Conn. This this is working as expected. you need to (implement Conn.CheckNamedValue) or (remove Conn.ExecContext and Conn.Exec).

@aletheia7
Copy link
Author

@aletheia7 aletheia7 commented Mar 16, 2018

Thanks. I'll try today. Worked in 1.9 but not 1.10.

@aletheia7
Copy link
Author

@aletheia7 aletheia7 commented Mar 17, 2018

@kardianos @andybons, Thank you for looking at this and your help.

I created a go test that can be dropped into database/sql that shows how go 1.9 calls Stmt.CheckNamedValue() and 1.10 does not. I created a simple test driver that implements the bare minimum. Can you help me understand why driver.Conn must implement CheckNamedValue()? Here is how I am thinking about this:

  1. Previously we discussed ExecContext(). ExecContext() is not implemented in the test driver. QueryContext() is used. Difference between go 1.9/1.10 occurs with QueryContext().
  2. NamedValueChecker states, "NamedValueChecker may be optionally implemented by Conn or Stmt." My driver chooses to implement Stmt.NamedValueChecker and not implement NamedValueChecker on the Conn.I cannot find any documentation that states NamedValueChecker must be present for Conn.QueryContext(). Perhaps this is a documentation bug?
  3. For the provided test function, TestNamedValue(), go 1.9 calls:
    • Con.Prepare()
    • Stmt.CheckNamedValue() ...
    • Stmt.QueryContext()
  4. go 1.10 does not make the same calls. go 1.10 calls:
    • Con.QueryContext()
  5. go 1.9 and go 1.10 should make the same calls. go 1.9 ignores Con.QueryContext() and calls Con.Prepare() to get a Stmt. go 1.10 calls Con.QueryContext() and does not call Prepare(). Shouldn't go 1.10 call Prepare()?

go 1.9 output

go test -v -run '^TestNamedValue$'
=== RUN   TestNamedValue
--- PASS: TestNamedValue (0.00s)
        m_test.go:128: go version: go1.9.4
        m_test.go:129: before QueryContext called
        m_test.go:24: TCon.Prepare
        m_test.go:95: Stmt.CheckNamedValue  7
        m_test.go:95: Stmt.CheckNamedValue  8
        m_test.go:88: Stmt.QueryContext
        m_test.go:135: after QueryContext called, Stmt.CheckNamedValue called?
PASS
ok      database/sql    0.002s

go 1.10 output

go test -v -run '^TestNamedValue$'
=== RUN   TestNamedValue
--- PASS: TestNamedValue (0.00s)
        m_test.go:128: go version: go1.10rc2
        m_test.go:129: before QueryContext called
        m_test.go:34: TCon.QueryContext
        m_test.go:135: after QueryContext called, Stmt.CheckNamedValue called?
PASS
ok      database/sql    0.002s

Drop m_test.go under database/sql. Run go test -v -run '^TestNamedValue$' for go 1.9/1.10 to repeat the results.

m_test.go

package sql

import (
        "context"
        "database/sql/driver"
        "io"
        "runtime"
        "testing"
)

type Driver struct {
        T *testing.T
}

func (o *Driver) Open(dsn string) (driver.Conn, error) {
        return &TCon{t: o.T}, nil
}

type TCon struct {
        t *testing.T
}

func (o *TCon) Prepare(query string) (driver.Stmt, error) {
        o.t.Log("TCon.Prepare")
        return &TStmt{con: o}, nil
}

// func (o *TCon) ExecContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Result, error) {
//      o.t.Log("TCon.ExecContext")
//      return nil, driver.ErrSkip
// }

func (o *TCon) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error) {
        o.t.Log("TCon.QueryContext")
        return &TRows{&TStmt{o}}, nil
}

func (o *TCon) Close() error {
        return nil
}

func (o *TCon) Begin() (driver.Tx, error) {
        return o, nil
}

func (o *TCon) Commit() error {
        return nil
}

func (o *TCon) Rollback() error {
        return nil
}

type TStmt struct {
        con *TCon
}

func (o TStmt) Close() error {
        return nil
}

func (o *TStmt) NumInput() int {
        return -1
}
// Docs are incorrect. Exec must be implemented by Stmt for driver.Stmt
// http://godoc.org/database/sql/driver#Stmt
//
func (o *TStmt) Exec(args []driver.Value) (driver.Result, error) {
        o.con.t.Log("Stmt.Exec")
        return nil, driver.ErrSkip
}

// Docs are incorrect. Exec must be implemented by Stmt for driver.Stmt
// http://godoc.org/database/sql/driver#Stmt
//
func (o *TStmt) Query(args []driver.Value) (driver.Rows, error) {
        o.con.t.Log("Stmt.Query")
        return nil, driver.ErrSkip
}

func (o *TStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (driver.Result, error) {
        o.con.t.Log("Stmt.ExecContext")
        return nil, driver.ErrSkip
}

func (o *TStmt) QueryContext(ctx context.Context, args []driver.NamedValue) (driver.Rows, error) {
        o.con.t.Log("Stmt.QueryContext")
        return &TRows{st: o}, nil
}

// http://gd/database/sql/driver#NamedValueChecker
//
func (o *TStmt) CheckNamedValue(nv *driver.NamedValue) error {
        o.con.t.Log("Stmt.CheckNamedValue", nv.Name, nv.Value)
        return nil
}

type TRows struct {
        st *TStmt
}

func (o *TRows) Columns() []string {
        return []string{}
}

func (o *TRows) Close() error {
        return nil
}

func (o *TRows) Next(dest []driver.Value) error {
        return io.EOF
}

func TestNamedValue(t *testing.T) {
        Register("testdb", &Driver{t})
        db, err := Open("testdb", "")
        if err != nil {
                t.Fatal(err)
        }
        defer db.Close()
        ctx := context.Background()
        con, err := db.Conn(ctx)
        if err != nil {
                t.Fatal(err)
        }
        defer con.Close()
        t.Log("go version:", runtime.Version())
        t.Log("before QueryContext called")
rows, err := con.QueryContext(ctx, "select ...", Named("", 7), Named("", 8))
        if err != nil {
                t.Fatal(err)
                return
        }
        t.Log("after QueryContext called, Stmt.CheckNamedValue called?")
        defer rows.Close()
        for rows.Next() {
                if err := rows.Scan(); err != nil {
                        t.Fatal(err)
                        return
                }
        }
        if err := rows.Err(); err != nil {
                t.Fatal(err)
                return
        }
}
aletheia7 added a commit to aletheia7/odb that referenced this issue Mar 17, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 17, 2018

@aletheia7 Great write-up. I'll respond in detail:

Here is the docs for driver.ExecerContext:

ExecerContext is an optional interface that may be implemented by a Conn.

If a Conn does not implement ExecerContext, the sql package's DB.Exec will fall back to Execer; if the Conn does not implement Execer either, DB.Exec will first prepare a query, execute the statement, and then close the statement.

ExecerContext may return ErrSkip.

ExecerContext must honor the context timeout and return when the context is canceled.

In otherwords, if Execer and ExecerContext is defined on driver.Conn, the execution flow bypasses the creation of a driver.Stmt. If there is no driver.Stmt, it does not know if CheckNamedValue is defined on Stmt, because it has been provided no Stmt. Thus it just looks for ExecerContext. This is the logical reason why it doesn't work in Go1.10.

Go1.9 first checks for the non-context driver variants for Queryer and Execer, then it checks for the context versions but only if the non-context versions exist (Queryer must exist for QueryerContext to be used in Go1.9). As documented in the Go1.10 release notes here https://golang.org/doc/go1.10#database/sql/driver

Drivers that implement ExecerContext no longer need to implement Execer; similarly, drivers that implement QueryerContext no longer need to implement Queryer. Previously, even if the context-based interfaces were implemented they were ignored unless the non-context-based interfaces were also implemented.

Your reference driver as listed above (which is great! Thanks for documenting your case so clearly!) only implements QueryerContext, not Queryer. You can also tell from the helpful trace you included that the Go1.9 version does NOT call the QueryContext method but is forced to create a Stmt. However in Go1.10 the QueryContext method is checked for independent of the Query method, so it no longer creates a Stmt. No Stmt, no CheckNamedValue (as defined in your system).

To resolve this issue, you should either remove QueryContext from TCon, or add CheckNamedValue to TCon.

@golang golang locked and limited conversation to collaborators Mar 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.