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.

Sign up for GitHub

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

Jump to bottom

database/sql: use %w verb for valuer errors to enable errors.Is/As #64707

Closed
joshgarrett-hellofresh opened this issue Dec 13, 2023 · 6 comments
Closed

database/sql: use %w verb for valuer errors to enable errors.Is/As #64707

joshgarrett-hellofresh opened this issue Dec 13, 2023 · 6 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.

Comments

@joshgarrett-hellofresh
Copy link

joshgarrett-hellofresh commented Dec 13, 2023 •
edited by mauri870

Go version

go version go1.21.4 darwin/arm64

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

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/gofer/Library/Caches/go-build'
GOENV='/Users/gofer/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/gofer/go/pkg/mod'
GOOS='darwin'
GOPATH='/Users/gofer/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.21.4/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.21.4/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/cf/314tnd5d22129h1z7srmcklr0000gn/T/go-build479648600=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

package main

import (
	"context"
	"database/sql"
	"database/sql/driver"
	"errors"
)

type Valuer struct {
	field []byte
}

func (v Valuer) Value() (driver.Value, error) {
	if len(v.field) == 0 {
		// return an error that we hope to be able to handle later
		return nil, errEmpty
	}
	return v, nil
}

// errEmpty is returned when an empty value is found
var errEmpty = errors.New("empty value")

func main() {
	db, _ := sql.Open("pgx", dsn)
	_, err := db.ExecContext(
		context.Background(),
		`INSERT INTO some_table (some_bytea_column) VALUES ($1)`,
		&Valuer{nil},
	)
	// errors.Is doesn't work b/c %v verb was used for error instead of %w
	if !errors.Is(err, errEmpty) {
		panic(err)
	}
}

What did you expect to see?

errors.Is should be able to detect sentinel errors returned by valuer. error will need to be properly wrapped by fmt.Errorf with %w verb instead of %v, similar to scanner

What did you see instead?

valuer errors are wrapped by fmt.Errorf with %v verb
@mauri870
Copy link
Member

Edited your issue to add a code block.

@mauri870 mauri870 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 14, 2023
@mauri870
Copy link
Member

mauri870 commented Dec 14, 2023 •
edited

I don't think this can cause any issues, so feel free to send a CL. Thanks!

@mauri870 mauri870 added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 14, 2023
@aimuz aimuz mentioned this issue Dec 15, 2023
Closed
aimuz added a commit to aimuz/go that referenced this issue Dec 15, 2023
@aimuz
database/sql: wrap errors with %w in driverArgsConnLocked
3f21a21 
Use fmt.Errorf's %w verb to wrap errors in driverArgsConnLocked,
which allows for easier unwrapping and checking of error types.

Add tests in sql_test.go to ensure that Stmt.Exec and Stmt.Query
correctly wrap underlying Valuer errors, adhering to the new change.

Fixes golang#64707.
@gopherbot
Copy link

Change https://go.dev/cl/550116 mentions this issue: database/sql: wrap errors with %w in driverArgsConnLocked

@callthingsoff
Copy link
Contributor

Related issue #44635, and CL https://go-review.googlesource.com/c/go/+/333990

@aimuz
Copy link
Contributor

aimuz commented Feb 19, 2024

@mauri870 Can you help me review CL? https://go.dev/cl/550116

aimuz added a commit to aimuz/go that referenced this issue Feb 20, 2024
@aimuz
database/sql: wrap errors with %w in driverArgsConnLocked
c496858 
Use fmt.Errorf's %w verb to wrap errors in driverArgsConnLocked,
which allows for easier unwrapping and checking of error types.

Add tests in sql_test.go to ensure that Stmt.Exec and Stmt.Query
correctly wrap underlying Valuer errors, adhering to the new change.

Fixes golang#64707.
aimuz added a commit to aimuz/go that referenced this issue Feb 21, 2024
@aimuz
database/sql: wrap errors with %w in driverArgsConnLocked
0df367e 
Use fmt.Errorf's %w verb to wrap errors in driverArgsConnLocked,
which allows for easier unwrapping and checking of error types.

Add tests in sql_test.go to ensure that Stmt.Exec and Stmt.Query
correctly wrap underlying Valuer errors, adhering to the new change.

Fixes golang#64707.
aimuz added a commit to aimuz/go that referenced this issue Mar 4, 2024
@aimuz
doc/go1.23: document database/sql wrap errors
a4d8eca 
For golang#64707
For golang#65614

Signed-off-by: aimuz <mr.imuz@gmail.com>
@aimuz aimuz mentioned this issue Mar 4, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/568755 mentions this issue: doc/go1.23: document database/sql wrap errors

gopherbot pushed a commit that referenced this issue Mar 4, 2024
@aimuz @gopherbot
doc/go1.23: document database/sql wrap errors
c273418 
For #64707.
For #65614.

Change-Id: Ib07ac67d7652bc7c9e1363f70637938c7bb4bc72
GitHub-Last-Rev: a4d8eca
GitHub-Pull-Request: #66089
Reviewed-on: https://go-review.googlesource.com/c/go/+/568755
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

database/sql: wrap errors with %w in driverArgsConnLocked aimuz/go
5 participants
@callthingsoff @gopherbot @mauri870 @aimuz @joshgarrett-hellofresh