Skip to content

fix: translate errors from the sqlite3 error type instead of JSON - #247

Open
davidpavlovschi wants to merge 1 commit into
go-gorm:masterfrom
davidpavlovschi:fix-typed-error-translation
Open

fix: translate errors from the sqlite3 error type instead of JSON#247
davidpavlovschi wants to merge 1 commit into
go-gorm:masterfrom
davidpavlovschi:fix-typed-error-translation

Conversation

@davidpavlovschi

Copy link
Copy Markdown

I maintain glebarez/sqlite, the pure-Go fork of
this driver. I have been running a parity audit between the two, and I committed in
glebarez#152 to sending anything backend-agnostic here first rather than letting
the fork drift. This is the first of those.

Translate currently round-trips the error through encoding/json and reads the
extended result code off the decoded object. That has two problems.

It matches on field names rather than on type. Any error carrying Code,
ExtendedCode and SystemErrno fields gets translated even when it comes from a
completely unrelated package. I added a test type whose message is "unrelated error
from another library" and today Translate turns it into gorm.ErrDuplicatedKey.

It also stops recognising a real sqlite error the moment something wraps it. json.Marshal
on a wrapped error produces {}, so the extended code reads back as 0. Any callback,
hook or plugin that adds context to the driver error before it reaches AddError loses
translation silently.

This reads the extended result code from sqlite3.Error through errors.As, so the
whole error chain is searched and only real sqlite errors match. Both sqlite3.Error
and *sqlite3.Error are probed, so the pointer case that worked before keeps working.

About the comment saying the go-sqlite3 error type is avoided because it needs cgo: I
checked, and that reason is correct. sqlite3.Error is declared in a file that imports
C, so it does not exist under CGO_ENABLED=0, and a plain type switch would break that
build. So Translate is split by build tag. The non-cgo build returns the error
untouched, which is the right answer there: without cgo the driver cannot open a
connection at all, so no sqlite error ever reaches the function. That also removes the
false positives on that build, where the JSON heuristic could only ever misfire.

ErrMessage is exported, so I kept it and marked it deprecated rather than deleting it.

One compatibility note. If someone points this Dialector at a pure-Go sqlite driver via
DriverName and builds with CGO_ENABLED=0, translation becomes a no-op. That is not a
regression: modernc.org/sqlite's error type has unexported fields, so json.Marshal
already produced {} and the old code did not translate it either.

There was no test for Translate before this, so all seven tests are new. Two of them
fail against the old implementation and pass against the new one. The other five pass
against both, which is what shows existing behaviour is unchanged.

go test ./... goes from 70 passing to 77, no failures. gofmt, go vet,
CGO_ENABLED=0 go build ./... and CGO_ENABLED=0 go vet ./... are all clean.

Translate round-tripped the error through encoding/json and read the
extended result code off the resulting object. That has two problems.

It matches on field names rather than on type, so any error carrying
Code/ExtendedCode/SystemErrno fields is translated even when it comes
from an unrelated package, and a real sqlite error stops being
recognised as soon as a callback or plugin wraps it.

Read the extended result code from sqlite3.Error through errors.As so
the whole error chain is searched and only sqlite errors match.

sqlite3.Error is declared in a file that imports C, so Translate is
split by build tag. Without cgo the driver cannot open a connection at
all, so the non-cgo build returns the error untouched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant