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.
>