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: Rows.Scan does not use the %w verb when wrapping errors #38099

Closed
sfllaw opened this issue Mar 26, 2020 · 14 comments
Closed

database/sql: Rows.Scan does not use the %w verb when wrapping errors #38099

sfllaw opened this issue Mar 26, 2020 · 14 comments

Comments

@sfllaw
Copy link

@sfllaw sfllaw commented Mar 26, 2020

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

$ go version
go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/slaw/.cache/go-build"
GOENV="/home/slaw/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/slaw/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build400385127=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.13.4 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.13.4
uname -sr: Linux 5.3.0-40-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 19.10
Release:	19.10
Codename:	eoan
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.30-0ubuntu2.1) stable release version 2.30.
gdb --version: GNU gdb (Ubuntu 8.3-0ubuntu1) 8.3

What did you do?

database/sql.Rows.Scan does not use the %w verb when wrapping errors:

return fmt.Errorf(`sql: Scan error on column index %d, name %q: %v`, i, rs.rowsi.Columns()[i], err)

Here is a motivating example involving a custom type that implements database/sql.Scanner:

package main

import (
	"database/sql"
	"errors"
	"fmt"

	_ "github.com/mattn/go-sqlite3"
)

var ErrInvalid = errors.New("not an ASCII string")

type ASCII []byte

func (a ASCII) Scan(src interface{}) error {
	// Elide code that parses src into a.
	return ErrInvalid
}

func main() {
	db, err := sql.Open("sqlite3", ":memory:")
	if err != nil {
		panic(err)
	}
	defer db.Close()

	var a ASCII
	if err := db.QueryRow(`SELECT "世界"`).Scan(&a); err != nil {
		if !errors.Is(err, ErrInvalid) {
			panic(fmt.Errorf("unexpected error: %w", err))
		}
		fmt.Printf("err is ErrInvalid: %v\n", err)
	}
}

What did you expect to see?

sfllaw@nyancat:~/go-sql-error-bug$ go run -trimpath main.go
err is ErrInvalid: sql: Scan error on column index 0, name "\"世界\"": not an ASCII string

What did you see instead?

sfllaw@nyancat:~/go-sql-error-bug$ go run -trimpath main.go
panic: unexpected error: sql: Scan error on column index 0, name "\"世界\"": not an ASCII string

goroutine 1 [running]:
main.main()
	github.com/sfllaw/go-sql-error-bug@/main.go:30 +0x2f0
exit status 2
sfllaw added a commit to sfllaw/go-sql-error-bug that referenced this issue Mar 26, 2020
database/sql.Rows.Scan does not use the %w verb when wrapping errors
@andybons
Copy link
Member

@andybons andybons commented Mar 27, 2020

Now that 1.12 is no longer supported, I think we can make this change. What do you think, @jba?

@andybons andybons added this to the Unplanned milestone Mar 27, 2020
@jba
Copy link
Contributor

@jba jba commented Mar 27, 2020

I'm for it. But this is technically breaking change, since anyone since 1.13 who was doing errors.Is on one of these errors could now get a different result. I don't know if that's an OK sort of change.

@andybons
Copy link
Member

@andybons andybons commented Mar 28, 2020

Hm. I’m not a fan of breaking changes. Will follow up. This needs more investigation.

@neild
Copy link
Contributor

@neild neild commented Mar 29, 2020

I don’t think this is a breaking change. It’s a behavioral change, and any behavioral change is of course subject to Hyrum’s Law, but I don’t believe this changes the documented properties of the package’s errors.

@sfllaw
Copy link
Author

@sfllaw sfllaw commented Mar 29, 2020

@jba @andybons: I’m not sure how this would be interpreted as a breaking change. Because fmt.Errorf returns an *errors.errorString, the only real way to test for equality would be to call its Error method, which will result in the exact same string because of how the %w verb is defined.

If developers are currently using errors.Is or errors.As to check for a wrapped sql.Scanner error, then their code is currently broken because they will never return true. I am unsure if we should really be supporting the use case where Is or As always fails. I argue that changing the fmt.Errorf call to use %w is actually a non-breaking change, because this code has never worked with Is or As.

In fact, with the introduction of errors.Is and errors.As, I was surprised to find that this error was not wrapped. If I had not written comprehensive tests, I may never have caught this and my error handling code would have never handled this correctly.

Someone might argue that these errors shouldn’t be wrapped because this is exposing the internal details of the Scan method: Errors and package APIs. That doesn’t hold water, since the caller of rows.Scan knows which pointers are being passed in and therefore expects certain errors while scanning. If the errors are repackaged without wrapping, then the caller can only examine the Error string heuristicly. The workaround I’m currently using checks strings.Contains(err.Error(), ErrInvalid.Error()) which is kind of terrible.

@muhlemmer
Copy link
Contributor

@muhlemmer muhlemmer commented Aug 10, 2020

I'm agreeing with @sfllaw's conclusion. errors.Is and errors.As can't be used against the returned error. Only string level equality can ben tested. If existing code is using string level equality testing, it won't break by switching to the %w verb:

var pgErr = errors.New("playground")

func main() {
	strErr := fmt.Errorf("Hello, %v", pgErr)
	if strings.Contains(strErr.Error(), pgErr.Error()) {
		fmt.Println(strErr)
	}
	
	wrpErr := fmt.Errorf("Hello, %w", pgErr)
	if strings.Contains(wrpErr.Error(), pgErr.Error()) {
		fmt.Println(wrpErr)
	}
	
	if errors.Is(wrpErr, pgErr) {
		fmt.Println(wrpErr)
	}
}

https://play.golang.org/p/uxpnS97ASZf

One of the affected use cases would be implementers of the sql.Scanner interface, like myself and the OP:

var (
	errJSONEmpty  = errors.New("JSON result empty")
	errJSONAssert = errors.New("JSON assertion error")
)

func (s *jsonScanner) Scan(v interface{}) (err error) {
	if v == nil {
		return errJSONEmpty
	}

	js, ok := v.([]byte)
	if !ok {
		return errJSONAssert
	}

	if s.Value, err = s.p.ParseBytes(js); err != nil {
		return fmt.Errorf("json Scan: %w", err)
	}

	return nil
}

Using the above pattern, I would expect to can do something like:

       	var js jsonScanner

	err = rt.Tx.QueryRowContext(rt.Ctx, query, args...).Scan(&js)
	switch {
	case err == nil:
	case errors.Is(err, errEmptyJSON):
		return []*shop.Article{}, nil
	default:
		return nil, status.Error(codes.Internal, errDB)
	}
@andybons andybons changed the title database/sql.Rows.Scan does not use the %w verb when wrapping errors database/sql: Rows.Scan does not use the %w verb when wrapping errors Aug 10, 2020
@andybons
Copy link
Member

@andybons andybons commented Aug 10, 2020

@neild @jba what do y’all think? Should we do this early in the 1.16 cycle and see what breaks?

@neild
Copy link
Contributor

@neild neild commented Aug 10, 2020

I think we should do this.

I expect that this will break some tests using reflect.DeepEqual on errors returned by Rows.Scan. This is fine, but is best to do early in cycle.

I don't expect any breakage from code using errors.Is, since as @sfllaw and others note this would never have worked in the first place. Even if such code exists, I do not believe this change is a violation of the compatibility guarantee since it makes no backwards-incompatible changes to the documented properties of database/sql's errors.

@andybons
Copy link
Member

@andybons andybons commented Aug 10, 2020

sgtm. marking as early-in-cycle

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 12, 2020

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@muhlemmer
Copy link
Contributor

@muhlemmer muhlemmer commented Aug 13, 2020

I'm working now on a CL, should submit in the next hour or so.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248337 mentions this issue: database/sql: use the %w error verb in Rows.Scan

@sfllaw
Copy link
Author

@sfllaw sfllaw commented Aug 13, 2020

@muhlemmer Suggestion: we should document how the Scanner's error propagates through the stack.

@muhlemmer
Copy link
Contributor

@muhlemmer muhlemmer commented Aug 13, 2020

@gopherbot gopherbot closed this in a20cb4c Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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