Repository navigation
fix: v0.19.1 — ILike, error classification by driver code, Transactional rollback errors - #64
Merged
Merged
Conversation
v0.19.0 turned LikeExp into an interface and dropped its exported Like field. v0.18 callers wrote exp.Like = "ILIKE" (the field was documented as LIKE, NOT LIKE or ILIKE); that no longer compiled and had no replacement, and the migration guide did not mention it. ILike and its NotILike/OrILike/OrNotILike variants return LikeExp, so escaping, Match and EscapeChars work as for Like. Rendering is per dialect: PostgreSQL uses ILIKE. MySQL and SQLite have no ILIKE and their LIKE is not reliably case-insensitive (MySQL follows the column collation, *_bin is case-sensitive; SQLite folds ASCII only and honors case_sensitive_like), so both sides are folded by the server: LOWER(col) LIKE LOWER(?). The v0.18 escape hatch emitted ILIKE on every dialect, which MySQL and SQLite reject. Integration tests run on a case-sensitive column (PostgreSQL, MySQL utf8mb4_bin) and cover AND/OR/NOT forms, escaped wildcards, Match, qualified columns, transactions, non-ASCII per dialect, and an invalid expression that must fail instead of deleting every row. README: the suffix example used Match(false, true), which is a prefix match; fixed and a prefix example added.
…g it When f returned an error, Transactional and TransactionalTx called tx.Rollback and ignored its result. A failed rollback (lost connection) leaves the caller unaware that the transaction's fate is unknown. ozzo-dbx returns both errors; Relica now does too, with errors.Join so errors.Is still matches the original error and the rollback error. sql.ErrTxDone is skipped: the transaction was already ended by f or by a canceled context, nothing was left to roll back. Transactional now delegates to TransactionalTx(ctx, nil, f) instead of duplicating it. The always-skipped TestTransactional_CommitError is now a real test (driver whose Commit fails). Godoc: Rollback after Commit returns sql.ErrTxDone (it said no-op); Transactional never joins a transaction carried in ctx, and f must not end tx itself.
…examples
Public docs taught the mistakes found in projects using Relica:
- Set(map{"balance": "balance - ?"}) in GETTING_STARTED, BEST_PRACTICES
and the sqlx guide binds the text as the new value (SQLite stores it
in an INTEGER column); now relica.NewExp("balance - ?", amount).
- README row locking ran ForUpdate/ForShare/SkipLocked on *DB, where the
SELECT autocommits and the lock is released at once; now inside
Transactional on tx.
- README, TROUBLESHOOTING, GORM tip, SECURITY and godoc deferred
tx.Rollback before checking Begin's error (nil-pointer panic) or
ignored Execute/Commit errors (partial commits).
- BEST_PRACTICES repository held *relica.DB and dropped ctx, so it could
not join a transaction and request deadlines never reached SQL.
- "Rollback after Commit is a no-op": it returns sql.ErrTxDone.
AGENTS.md Transactions is now a decision guide: rules, which API for
which situation, a locked transfer, manual Begin, dbcontext wiring
(open once, repositories call With(ctx), service owns the boundary),
context and isolation semantics, SQLite specifics (_txlock=immediate),
a rollback test that fails for a With(ctx) ignoring the tx, and a
symptom/cause/fix table. llms.txt points agents to it.
All new snippets were compiled and run on SQLite against this branch.
IsUniqueViolation, IsForeignKeyViolation, IsNotNullViolation and IsCheckViolation matched substrings of err.Error(). They returned false on a PostgreSQL server with non-English lc_messages, missed MariaDB CHECK errors (4025) and MySQL NOT NULL errors 1263/1364. The helpers now read the code the driver attached, anywhere in the error tree (Unwrap chains and errors.Join), first code wins as with errors.As: - PostgreSQL SQLSTATE through SQLState() (pgx v5, lib/pq >= v1.10.9) or the Code field of lib/pq < v1.10.9; - MySQL/MariaDB error number and SQLSTATE of go-sql-driver/mysql MySQLError; - SQLite extended result code of modernc.org/sqlite and mattn/go-sqlite3. Reflection is bound to those exact package paths and type names; no driver is imported. The message is matched only when the tree holds no code. New helpers for retry decisions: IsDeadlock (40P01, 1213), IsSerializationFailure (40001, SQLITE_BUSY_SNAPSHOT) and IsLockTimeout (55P03, 1205, 3572, SQLITE_BUSY, SQLITE_LOCKED). Tests: look-alike driver shapes for every code; integration on PostgreSQL through lib/pq and pgx, MySQL and SQLite, each class provoked for real (deadlock, serialization failure, lock timeout, NOWAIT, localized server); lib/pq v1.2.0 and mattn/go-sqlite3 in a separate module test/drivercompat, run in the SQLite CI job.
- AGENTS.md: rule 12 (retry the whole transaction), RetryTx helper with full-jitter backoff and context, what to retry per error class, error helper code table, three more rows in the real-project mistakes table. - ADVANCED_PATTERNS: the retry example called an undefined isDeadlock, ignored the Commit error and the context; replaced with RetryTx. - BEST_PRACTICES: eight repository examples called r.db.Select()/r.db.Model() on the dbcontext type, which has neither method and would bypass the caller's transaction; they now use r.db.With(ctx). The UpdateChanged note no longer claims to prevent lost updates. - README, TROUBLESHOOTING, SECURITY, sqlx and GORM guides (GORM has TranslateError sentinels since v1.25), llms.txt: helpers read driver codes; db.Model(&x).Insert() returns only error. - MIGRATION_GUIDE: v0.19.0 -> v0.19.1 section. CHANGELOG: [0.19.1]. Every changed snippet was compiled against the public API; RetryTx was run against SQLite (rolled-back attempts, non-retryable error, exhausted attempts, context deadline).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…CA_TEST_DB The integration jobs used -run Postgres / -run MySQL / -run SQLite. -run is case-sensitive and matches top-level names only, so the 16 tests named *_PostgreSQL never ran in CI (transactions, Executor, Model API, ILike, error classification, ...), and neither did any multi-database test (*_AllDatabases, IrisMX, WrapDB), whose databases are subtests. Each job now runs every test with RELICA_TEST_DB=postgres|mysql|sqlite. The Setup helpers and the tests that open a database themselves skip the databases not selected. When the selected database's container does not start, the test fails instead of skipping, so a broken Docker host cannot turn the job green. testcontainers panics without a Docker host (Windows); the panic is turned into that error, so local runs without Docker skip cleanly. Running the SQLite part of the multi-database tests exposed a broken test: TestWrapDB_ConnectionPoolSettings ran concurrent queries on ':memory:', where every pool connection has its own empty database. getDSN now uses a file with busy_timeout for SQLite and the DSN recorded by Setup for PostgreSQL/MySQL instead of hard-coded localhost DSNs.
Insert, Upsert, Update, Delete, BatchInsert and BatchUpdate quoted the table name as one identifier: db.Insert("app.users", ...) produced INSERT INTO "app.users", which PostgreSQL rejects with 'relation "app.users" does not exist' (MySQL and SQLite likewise). SELECT already quoted per part.
A new quoteTableName quotes every dot-separated part and, unlike quoteColumn, passes nothing through unquoted. DB.QuoteTableName uses it too.
Found when the PostgreSQL integration suite (TestSchemaQualifiedTable_PostgreSQL) first ran in CI.
PostgreSQL resolves a CASE whose results are all untyped parameters to text. Update(...).Set({"n": CaseWhen().When(cond, 9).Else(0)}) therefore failed on an integer column, and OrderBySub(CaseWhen()...) sorted 10 before 2 — while the same CASE with literals works.
When every non-NULL result is a Go number, or every one is a bool, the parameters are cast to the type a literal would have: INTEGER, BIGINT beyond int32, NUMERIC for floats and for uint64 beyond int64, BOOLEAN. Nothing is cast when a result is an Expression (it types the CASE already, a cast could conflict), when results are strings (untyped like string literals) or mixed. WHEN values of a simple CASE take the column's type and are not cast. MySQL and SQLite are unchanged.
Found when TestPublicTypes_PostgreSQL first ran in CI; adds an integration test that fails if the CASE sorts as text.
…tabases on failure TestWrapDB_IrisMXUseCase wrote the string "unread" into the INT status column. SQLite accepts that (type affinity); PostgreSQL and MySQL strict mode reject it. The test now writes 1. With the default fail-fast, the failing postgres job cancelled the mysql job, so its result was never known. Both matrices now use fail-fast: false.
…params before HAVING OrderBy columns were always rendered before OrderByExpr and OrderBySub expressions, whatever the call order, so OrderBySub(...).OrderBy(...) sorted by the column first. GroupBy had the same split. GroupByExpr and GroupBySub parameters were appended after Having's, although GROUP BY precedes HAVING in the statement: with parameters in both clauses each was bound to the other's placeholder and the query returned wrong rows without an error. A unit test asserted that order. ORDER BY and GROUP BY items now share one ordered list per clause, and the SQL and its parameters come from the same pass. Count keeps only the GROUP BY columns, as before.
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.
Fixes for gaps in v0.19.0 and in the error helpers, released together as v0.19.1.
Case-insensitive LIKE — v0.19.0 made
LikeExpan interface, so code that setexp.Like = "ILIKE"stopped compiling and had no replacement. Addsrelica.ILike,NotILike,OrILike,OrNotILike:ILIKEon PostgreSQL,LOWER(col) LIKE LOWER(?)on MySQL and SQLite, with the same wildcard escaping asLike.Error classification by driver code —
IsUniqueViolation,IsForeignKeyViolation,IsNotNullViolationandIsCheckViolationmatchederr.Error()text. That fails on a PostgreSQL server with non-Englishlc_messages, and it missed MariaDB CHECK (4025) and MySQL 1263/1364. The helpers now read the code anywhere in the error tree (%wchains,errors.Join; the first code wins, as witherrors.As):SQLState()(pgx v5, lib/pq >= v1.10.9), or theCodefield of older lib/pqMySQLErrorReflection is limited to those exact package paths and type names. No driver is imported, so the main module still has zero dependencies. Text matching is used only when the error carries no code.
New:
IsDeadlock(40P01, 1213),IsSerializationFailure(40001,SQLITE_BUSY_SNAPSHOT),IsLockTimeout(55P03, 1205, 3572,SQLITE_BUSY,SQLITE_LOCKED).Transactional reports a failed rollback — when
ffailed and the rollback also failed, the rollback error was dropped. It is now returned aserrors.Join(err, rollbackErr).sql.ErrTxDoneis not reported.Docs — AGENTS.md has a full "Transactions" section for people and coding agents: rules, which API to use when, the dbcontext pattern, context and isolation, SQLite specifics, a rollback test, a
RetryTxhelper, and mistakes seen in real projects. Wrong examples fixed across 11 files:"balance - ?"bound as a string;ForUpdateoutside a transaction;defer tx.Rollback()before theBeginerror check;Commiterrors;r.db.Select()on the dbcontext type);_, err := db.Model(&x).Insert();isDeadlock.CI ran only part of the integration suite. The jobs selected tests with
-run Postgres|MySQL|SQLite. That filter is case-sensitive and only sees top-level names, so 16*_PostgreSQLtests never ran in CI. Neither did any multi-database test (*_AllDatabases, IrisMX, WrapDB), because there the databases are subtests. Each job now runs the whole suite withRELICA_TEST_DB=postgres|mysql|sqlite. Other databases are skipped. If the selected database's container does not start, the job fails instead of skipping. The matrix usesfail-fast: false. The first full run found two library bugs, fixed here with failing tests first:app.usersas one identifier ("app.users"); SELECT was already correct. Every part is now quoted, with nothing passed through raw.text.Set({"n": CaseWhen().When(c, 9).Else(0)})therefore failed on integer columns, andOrderBySub(CaseWhen()...)sorted 10 before 2. When every result is a Go number, or every result is a bool, the parameters are now cast the way a literal would be typed (INTEGER/BIGINT/NUMERIC/BOOLEAN). Expression and string results are unchanged.It also found two broken tests: concurrent queries on SQLite
:memory:, and a string written to an INT column.ORDER BY and GROUP BY follow the call order.
OrderBycolumns were always rendered beforeOrderByExpr/OrderBySubexpressions, soOrderBySub(priority).OrderBy("created_at")sorted bycreated_atfirst. TheGroupByfamily behaved the same way. Fixing this exposed a worse bug:GroupByExpr/GroupBySubparameters were bound afterHaving's, although GROUP BY comes first in the statement. With parameters in both clauses,GroupByExpr("CASE WHEN price > ? ...", 60).Having("COUNT(*) >= ?", 2)ran asprice > 2andCOUNT(*) >= 60and returned wrong rows without an error. An existing unit test asserted that order.Each clause now keeps one ordered list of items, and the SQL and its parameters are produced in a single pass.
Countstill keeps only the GROUP BY columns, as before; a test pins this.Also included (already on main): timer-dependent tests use
testing/synctest(#63).Nothing stops compiling. Behaviour changes are listed in MIGRATION_GUIDE "v0.19.0 → v0.19.1"; code that calls
OrderBySub/OrderByExprbeforeOrderBynow sorts the way it reads. Error helpers now return true in more cases (localized servers, MariaDB, MySQL strict mode). When an error carries a code, its message is no longer consulted.Verification:
Unit tests check every code per class and per driver shape, with exactly one class matching each. They also cover the error tree, typed nil, a changed driver shape (falls back to text), and unregistered look-alike types (not read).
Integration tests provoke each class for real: constraints (also through
Transactionaland the Model API), deadlock with opposite-order locks, serialization failure, lock timeout, NOWAIT, and a localized PostgreSQL. They run on PostgreSQL through both lib/pq and pgx (pgx added totest/go.mod), on MySQL 8.0 and on SQLite (file, WAL).New CI step: a
test/drivercompatmodule for lib/pq v1.2.0 and mattn/go-sqlite3 (cgo).Mutation checks: breaking the binding to modernc, lib/pq or mattn makes the integration or drivercompat tests fail.
ORDER BY / GROUP BY: unit tests with interleaved calls on all three dialects check both the SQL and the parameter order. They fail on the previous code. Two integration subtests (an ORDER BY where the order changes the result; GROUP BY + HAVING parameters) fail on the previous code on SQLite and pass with the fix.
A SQLSTATE 23505 error with a German message returns
IsUniqueViolation = falseon main and true on this branch.Locally: unit tests, lint, SQLite integration, and drivercompat (mattn under Linux with gcc).
CI, checked in the job logs (not only the job status):
Every skip is "another database selected", except four documented SQLite cases: no deadlock detection, ASCII-case-insensitive
LIKE, and twoTxOptionssubtests covered byTxOptions_SQLite. There are no Docker skips.The localized-server test fell back to its second path on
postgres:15-alpine, which has no German messages: it ran the real driver error with a German message. The log records which path ran.