code review 41590043: database/sql: Check errors in QueryRow.Scan

database/sql: Check errors in QueryRow.Scan The previous coding did not correctly check for errors from the driver's Next() or Close(), which could mask genuine errors from the database, as witnessed in issue #6651. Even after this change errors from Close() will be ignored if the query returned no rows (as Rows.Next will have closed the handle already), but it is a lot easier for the drivers to guard against that. Fixes issue 6651.

[johto, 2013-12-12 23:33:41]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[johto, 2013-12-14 01:10:07]

On 12/13/13, 12:33 AM, I wrote: > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go Updated this CL to make the tests use a new hook instead. The previous approach was too ad-hoc, and extremely ugly. .marko 

[bradfitz, 2013-12-16 20:28:42]

Marko, You'll need to submit the CLA before for this to be accepted: http://golang.org/doc/contribute.html#copyright Please reply here when you've done so. Thanks! On Thu, Dec 12, 2013 at 3:33 PM, <marko@joh.to> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > database/sql: Check errors in QueryRow.Scan > > The previous coding did not correctly check for errors from the > driver's > Next() or Close(), which could mask genuine errors from the database, as > witnessed in issue #6651. > > Even after this change errors from Close() will be ignored if the query > returned no rows (as Rows.Next will have closed the handle already), but > it > is a lot easier for the drivers to guard against that. > > Fixes issue 6651. > > Please review this at https://codereview.appspot.com/41590043/ > > Affected files (+65, -4 lines): > M src/pkg/database/sql/fakedb_test.go > M src/pkg/database/sql/sql.go > M src/pkg/database/sql/sql_test.go > > > Index: src/pkg/database/sql/fakedb_test.go > =================================================================== > --- a/src/pkg/database/sql/fakedb_test.go > +++ b/src/pkg/database/sql/fakedb_test.go > @@ -592,6 +592,12 @@ > time.Sleep(time.Duration(args[1].(int64)) > * time.Millisecond) > } > } > + } else if s.table == "issue6651" { > + if len(s.whereCol) != 1 || s.whereCol[0] != > "failonrowsnext" { > + return nil, fmt.Errorf("fakedb: invalid where > clause for table issue6651") > + } > + failonrowsnext := args[0].(bool) > + return &issue6651Rows{failonrowsnext: failonrowsnext}, nil > } > > t.mu.Lock() > @@ -642,6 +648,26 @@ > return cursor, nil > } > > +type issue6651Rows struct { > + failonrowsnext bool > +} > + > +func (r *issue6651Rows) Columns() []string { > + return []string{"failure"} > +} > + > +func (r *issue6651Rows) Next(dest []driver.Value) error { > + if r.failonrowsnext { > + return fmt.Errorf("error in rows.Next") > + } > + dest[0] = false > + return nil > +} > + > +func (r *issue6651Rows) Close() error { > + return nil > +} > + > func (s *fakeStmt) NumInput() int { > return s.placeholders > } > Index: src/pkg/database/sql/sql.go > =================================================================== > --- a/src/pkg/database/sql/sql.go > +++ b/src/pkg/database/sql/sql.go > @@ -1495,10 +1495,12 @@ > closeStmt driver.Stmt // if non-nil, statement to Close on close > } > > -// Next prepares the next result row for reading with the Scan method. > -// It returns true on success, false if there is no next result row. > -// Every call to Scan, even the first one, must be preceded by a call > -// to Next. > +// Next prepares the next result row for reading with the Scan method. It > +// returns true on success, or false if there is no next result row or an > error > +// happened while preparing it. Err should be consulted to distinguish > between > +// the two cases. > +// > +// Every call to Scan, even the first one, must be preceded by a call to > Next. > func (rs *Rows) Next() bool { > if rs.closed { > return false > @@ -1625,12 +1627,19 @@ > } > > if !r.rows.Next() { > + if err := r.rows.Err(); err != nil { > + return err > + } > return ErrNoRows > } > err := r.rows.Scan(dest...) > if err != nil { > return err > } > + // Make sure the query can be processed to completion with no > errors. > + if err := r.rows.Close(); err != nil { > + return err > + } > > return nil > } > Index: src/pkg/database/sql/sql_test.go > =================================================================== > --- a/src/pkg/database/sql/sql_test.go > +++ b/src/pkg/database/sql/sql_test.go > @@ -58,6 +58,9 @@ > exec(t, db, "CREATE|magicquery|op=string,millis=int32") > exec(t, db, "INSERT|magicquery|op=sleep,millis=10") > } > + if name == "issue6651" { > + exec(t, db, "CREATE|issue6651|failonrowsnext=bool") > + } > return db > } > > @@ -660,6 +663,29 @@ > } > } > > +// Test issue 6651 > +func TestIssue6651(t *testing.T) { > + db := newTestDB(t, "issue6651") > + defer closeDB(t, db) > + > + var v bool > + err := db.QueryRow("SELECT|issue6651| > failonrowsnext|failonrowsnext=?", true).Scan(&v) > + want := "error in rows.Next" > + if err == nil || err.Error() != want { > + t.Errorf("error = %q; want %q", err, want) > + } > + > + want = "error in rows.Close" > + rowsCloseHook = func(rows *Rows, err *error) { > + *err = fmt.Errorf(want) > + } > + defer func() { rowsCloseHook = nil }() > + err = db.QueryRow("SELECT|issue6651|failonrowsnext|failonrowsnext=?", > false).Scan(&v) > + if err == nil || err.Error() != want { > + t.Errorf("error = %q; want %q", err, want) > + } > +} > + > type nullTestRow struct { > nullParam interface{} > notNullParam interface{} > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > 

[johto, 2013-12-16 20:32:15]

Hi Brad, On 12/16/13, 9:28 PM, Brad Fitzpatrick wrote: > You'll need to submit the CLA before for this to be accepted: > http://golang.org/doc/contribute.html#copyright > > Please reply here when you've done so. Thanks! I've submitted the online form just now. .marko 

[bradfitz, 2013-12-16 20:48:29]

LGTM On Thu, Dec 12, 2013 at 3:33 PM, <marko@joh.to> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > database/sql: Check errors in QueryRow.Scan > > The previous coding did not correctly check for errors from the > driver's > Next() or Close(), which could mask genuine errors from the database, as > witnessed in issue #6651. > > Even after this change errors from Close() will be ignored if the query > returned no rows (as Rows.Next will have closed the handle already), but > it > is a lot easier for the drivers to guard against that. > > Fixes issue 6651. > > Please review this at https://codereview.appspot.com/41590043/ > > Affected files (+65, -4 lines): > M src/pkg/database/sql/fakedb_test.go > M src/pkg/database/sql/sql.go > M src/pkg/database/sql/sql_test.go > > > Index: src/pkg/database/sql/fakedb_test.go > =================================================================== > --- a/src/pkg/database/sql/fakedb_test.go > +++ b/src/pkg/database/sql/fakedb_test.go > @@ -592,6 +592,12 @@ > time.Sleep(time.Duration(args[1].(int64)) > * time.Millisecond) > } > } > + } else if s.table == "issue6651" { > + if len(s.whereCol) != 1 || s.whereCol[0] != > "failonrowsnext" { > + return nil, fmt.Errorf("fakedb: invalid where > clause for table issue6651") > + } > + failonrowsnext := args[0].(bool) > + return &issue6651Rows{failonrowsnext: failonrowsnext}, nil > } > > t.mu.Lock() > @@ -642,6 +648,26 @@ > return cursor, nil > } > > +type issue6651Rows struct { > + failonrowsnext bool > +} > + > +func (r *issue6651Rows) Columns() []string { > + return []string{"failure"} > +} > + > +func (r *issue6651Rows) Next(dest []driver.Value) error { > + if r.failonrowsnext { > + return fmt.Errorf("error in rows.Next") > + } > + dest[0] = false > + return nil > +} > + > +func (r *issue6651Rows) Close() error { > + return nil > +} > + > func (s *fakeStmt) NumInput() int { > return s.placeholders > } > Index: src/pkg/database/sql/sql.go > =================================================================== > --- a/src/pkg/database/sql/sql.go > +++ b/src/pkg/database/sql/sql.go > @@ -1495,10 +1495,12 @@ > closeStmt driver.Stmt // if non-nil, statement to Close on close > } > > -// Next prepares the next result row for reading with the Scan method. > -// It returns true on success, false if there is no next result row. > -// Every call to Scan, even the first one, must be preceded by a call > -// to Next. > +// Next prepares the next result row for reading with the Scan method. It > +// returns true on success, or false if there is no next result row or an > error > +// happened while preparing it. Err should be consulted to distinguish > between > +// the two cases. > +// > +// Every call to Scan, even the first one, must be preceded by a call to > Next. > func (rs *Rows) Next() bool { > if rs.closed { > return false > @@ -1625,12 +1627,19 @@ > } > > if !r.rows.Next() { > + if err := r.rows.Err(); err != nil { > + return err > + } > return ErrNoRows > } > err := r.rows.Scan(dest...) > if err != nil { > return err > } > + // Make sure the query can be processed to completion with no > errors. > + if err := r.rows.Close(); err != nil { > + return err > + } > > return nil > } > Index: src/pkg/database/sql/sql_test.go > =================================================================== > --- a/src/pkg/database/sql/sql_test.go > +++ b/src/pkg/database/sql/sql_test.go > @@ -58,6 +58,9 @@ > exec(t, db, "CREATE|magicquery|op=string,millis=int32") > exec(t, db, "INSERT|magicquery|op=sleep,millis=10") > } > + if name == "issue6651" { > + exec(t, db, "CREATE|issue6651|failonrowsnext=bool") > + } > return db > } > > @@ -660,6 +663,29 @@ > } > } > > +// Test issue 6651 > +func TestIssue6651(t *testing.T) { > + db := newTestDB(t, "issue6651") > + defer closeDB(t, db) > + > + var v bool > + err := db.QueryRow("SELECT|issue6651| > failonrowsnext|failonrowsnext=?", true).Scan(&v) > + want := "error in rows.Next" > + if err == nil || err.Error() != want { > + t.Errorf("error = %q; want %q", err, want) > + } > + > + want = "error in rows.Close" > + rowsCloseHook = func(rows *Rows, err *error) { > + *err = fmt.Errorf(want) > + } > + defer func() { rowsCloseHook = nil }() > + err = db.QueryRow("SELECT|issue6651|failonrowsnext|failonrowsnext=?", > false).Scan(&v) > + if err == nil || err.Error() != want { > + t.Errorf("error = %q; want %q", err, want) > + } > +} > + > type nullTestRow struct { > nullParam interface{} > notNullParam interface{} > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > 

[bradfitz, 2013-12-16 20:48:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=04f0931c9808 *** database/sql: Check errors in QueryRow.Scan The previous coding did not correctly check for errors from the driver's Next() or Close(), which could mask genuine errors from the database, as witnessed in issue #6651. Even after this change errors from Close() will be ignored if the query returned no rows (as Rows.Next will have closed the handle already), but it is a lot easier for the drivers to guard against that. Fixes issue 6651. R=golang-dev, bradfitz CC=golang-dev https://codereview.appspot.com/41590043 Committer: Brad Fitzpatrick <bradfitz@golang.org>