fix: translate errors from the sqlite3 error type instead of JSON - #247
Open
davidpavlovschi wants to merge 1 commit into
Open
fix: translate errors from the sqlite3 error type instead of JSON#247davidpavlovschi wants to merge 1 commit into
davidpavlovschi wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Translatecurrently round-trips the error throughencoding/jsonand reads theextended result code off the decoded object. That has two problems.
It matches on field names rather than on type. Any error carrying
Code,ExtendedCodeandSystemErrnofields gets translated even when it comes from acompletely unrelated package. I added a test type whose message is "unrelated error
from another library" and today
Translateturns it intogorm.ErrDuplicatedKey.It also stops recognising a real sqlite error the moment something wraps it.
json.Marshalon a wrapped error produces
{}, so the extended code reads back as0. Any callback,hook or plugin that adds context to the driver error before it reaches
AddErrorlosestranslation silently.
This reads the extended result code from
sqlite3.Errorthrougherrors.As, so thewhole error chain is searched and only real sqlite errors match. Both
sqlite3.Errorand
*sqlite3.Errorare 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.Erroris declared in a file that importsC, so it does not exist under
CGO_ENABLED=0, and a plain type switch would break thatbuild. So
Translateis split by build tag. The non-cgo build returns the erroruntouched, 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.
ErrMessageis 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
DriverNameand builds withCGO_ENABLED=0, translation becomes a no-op. That is not aregression:
modernc.org/sqlite's error type has unexported fields, sojson.Marshalalready produced
{}and the old code did not translate it either.There was no test for
Translatebefore this, so all seven tests are new. Two of themfail 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 ./...andCGO_ENABLED=0 go vet ./...are all clean.