Repository navigation
release: v1.7.0 - #117
Merged
Merged
release: v1.7.0#117
Conversation
…part MysqlBuilder.Upsert appended ON DUPLICATE KEY UPDATE to q.sql after NewQuery had already computed rawSQL and placeholders, so the server received a plain INSERT and an existing key failed with a duplicate-key error. The same bug was fixed for PostgreSQL in #70. Upsert now rebuilds the query from the final SQL. Also: - PostgreSQL Upsert without constraint columns emitted ON CONFLICT DO UPDATE without a target, which PostgreSQL rejects; it now sets LastError. - Constraint columns are left out of the update part on every database. On MySQL a row matched through another unique index keeps its own key. - An empty update part sets LastError instead of producing invalid SQL. The shared logic lives in BaseBuilder.upsert. MySQL integration tests cover the update path, expression values and a second unique key.
SqliteBuilder.Upsert generates INSERT ... ON CONFLICT (...) DO UPDATE SET ... (SQLite 3.24.0+). Before, SQLite fell through to BaseBuilder and returned "Upsert is not supported". The conflict target is required, as on PostgreSQL.
db.Model(&m).Upsert(attrs...) inserts the model or updates the row with the same primary key, on MySQL, PostgreSQL and SQLite (#95). The primary key is the conflict target and is never updated; attrs and Exclude choose the fields as for Insert and Update, and excluded fields are neither inserted nor updated. A zero single-column primary key is left out of the insertion so auto-increment works. The primary key field is not filled afterwards. Unit tests record the executed SQL per dialect; MySQL integration tests cover a PK-only table, a second unique key and a composite unique key with Exclude.
GetTableName checked TableModel only on the value it was given, and on a
zero element value for slices. A TableName method with a pointer receiver was
therefore ignored for T{}, []T and &[]T (e.g. All(&rows) without From()),
and the snake-case type name was used instead. It is now called on a new zero
value when *T implements TableModel.
ForUpdate(), ForShare() and ForUpdateSkipLocked() append FOR UPDATE, FOR SHARE or FOR UPDATE SKIP LOCKED after ORDER BY/LIMIT/OFFSET, with the same names as Relica. The clause comes from the optional LockQueryBuilder interface, implemented by BaseQueryBuilder, so custom QueryBuilder implementations keep compiling; one without it gets LastError. Build sets LastError when the lock is combined with UNION, GROUP BY or DISTINCT, when the executor is a *sql.DB (outside a transaction the lock is released at once), and for SQL Server and Oracle. SQLite has no row locks, so the clause is left out there. MySQL integration tests prove the locks: a second FOR UPDATE times out, SKIP LOCKED skips the locked row, two concurrent claimers get disjoint rows, and a write waits for a shared lock.
… receivers README: builder and model Upsert, a Locking Reads section, and the corrected note on TableName receivers. CHANGELOG: Unreleased entries.
A lock built from a DB is now rejected on every database, SQLite included. Outside a transaction the lock is released when the statement ends, so it protects nothing: two job-queue workers using ForUpdateSkipLocked could claim the same row. Rejecting it on SQLite as well lets a test suite that runs on SQLite catch the mistake. Inside a transaction SQLite still leaves the clause out without an error. The error names the method, e.g. "ForUpdateSkipLocked requires a transaction: outside one the lock is released when the statement ends".
Transactional and TransactionalContext turned sql.ErrTxDone from Commit into nil. That error also comes when database/sql has rolled the transaction back because the context was canceled while the callback ran, so the callback's writes were lost while the caller was told they were committed. Tx now records whether Commit or Rollback was called on it. If Commit returns ErrTxDone and the callback did not end the transaction itself, the context error is returned (ErrTxDone if the context is not done). A callback that commits or rolls back itself and returns nil still gets nil. Tests use a fake database/sql driver for both functions and the DB context path; a MySQL integration test checks that no row is committed.
Transactional returns Errors{err, rollbackErr} when the rollback also
fails. Errors had no Unwrap, so errors.Is/As on that result found neither
error. Add Unwrap() []error (Go 1.20 multi-error unwrapping).
LIKE values are escaped with a backslash (DefaultLikeEscape), which MySQL
and PostgreSQL use as the default escape character. SQLite has none, so
Like("name", "50%") searched for a literal backslash and never matched
"50% off". LikeExp.Build now appends ESCAPE '\' on SQLite when escaping
is on; MySQL must not get it, as '\' is an unterminated literal there.
Also document that Escape() with no arguments makes _ and % wildcards.
Reported by the Relica review (Relica fixed the same inherited bug in
v0.19.0).
Expression values are rendered in both the INSERT and the UPDATE part, so an expression reading the existing row (hits + 1) fails; on PostgreSQL and SQLite a NULL in a conflict column never conflicts. Both from the Relica review; update-side expressions are tracked for a later release.
Add the new sections of the English README to the Russian one: Upsert in data manipulation queries, ModelQuery.Upsert, Locking Reads, TableName receivers, and the Upsert limits. Both READMEs now also say how LIKE values are escaped and that Escape() with no arguments turns it off.
Move the Unreleased changes to the 1.7.0 section and update the roadmap: v1.7.0 completed items, Upsert removed from the next steps.
MySQL's UNIQUE indexes allow several NULLs as PostgreSQL's and SQLite's do, so the Upsert note applies to every database, not only PostgreSQL and SQLite. Add a MySQL integration test: two upserts with a NULL in a UNIQUE column insert two rows.
The first run on MySQL 8 showed two wrong expectations: upserting only Name for an existing customer fails because email is NOT NULL without a default (the insert part is checked before the conflict), and AccountPeriod.Created scans NULL after Exclude, so it is now a sql.NullString. The partial case now passes Name and Email, and a new assertion checks that leaving email out fails and changes nothing.
The database checks NOT NULL constraints on the insert part before it detects the conflict, so a field list must include every NOT NULL column without a default even when the row exists. Observed on MySQL 8.0.34 and SQLite.
The update part of Upsert bound every value a second time (col={:pN}, or
the Expression rendered again). That is a different statement from the
insert: on MySQL 8.0.34 a BEFORE INSERT trigger that lower-cases email
stored the lower-cased value on insert but the raw value on update, and an
expression such as a sequence or a non-deterministic function was
evaluated twice, the second value being stored.
The update part now copies the proposed row: col=EXCLUDED.col on
PostgreSQL and SQLite, col=VALUES(col) on MySQL. VALUES() is deprecated
since MySQL 8.0.20 but is the only form that works on MySQL 5.7, 8, 9 and
MariaDB. Each value is now bound once.
Tests: SQL and placeholder lists for all three dialects; MySQL integration
tests for the trigger case and for single evaluation (a stored function
counting calls in a session variable); SQLite tests on modernc.org/sqlite
v1.36.1 (added to the integration module, Go 1.21+) with a Go scalar
function counting calls.
README and README-ru: the SQL examples show EXCLUDED/VALUES(), and the Upsert notes say that BEFORE INSERT triggers apply, expressions are evaluated once, and VALUES() on MySQL is deprecated but portable.
Runs the integration suite against mysql:innovation (the newest release line) and prints the server version. It is not a required check: it shows early when a newer server breaks something we rely on, such as VALUES() in ON DUPLICATE KEY UPDATE, deprecated since MySQL 8.0.20. The required jobs are unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #117 +/- ##
==========================================
+ Coverage 86.45% 91.30% +4.85%
==========================================
Files 16 16
Lines 1514 1588 +74
==========================================
+ Hits 1309 1450 +141
+ Misses 155 97 -58
+ Partials 50 41 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
This release makes
Upsert()actually work on MySQL, adds model-level upserts (#95) and locking reads, and fixes a transaction bug that could silently lose writes.A few things worth calling out before upgrading:
Transactional()/TransactionalContext(): if the context was canceled while the callback ran, the transaction was rolled back but the call returnednil. It now returns the context error. If your code gotnilin that situation, those writes were in fact lost.Upsert()used to send only theINSERTpart to the server, so updating an existing key never worked. On all databases the update part now copies the proposed row (EXCLUDED.col/VALUES(col)) and leaves the constraint columns out. On PostgreSQL this meansBEFORE INSERTtriggers now apply on the update path too, and expression values are evaluated once.ForUpdate(),ForShare(),ForUpdateSkipLocked()) must be built from a transaction (tx.Select()); building one from aDBreturns an error, on SQLite too.TableName()with a pointer receiver is now used for values and slices (e.g.All(&rows)withoutFrom()). Code that silently depended on the snake-case fallback will query a different table.Changelog
Added
ModelQuery.Upsert(attrs...): inserts a model or updates the row with the same primary key, on MySQL, PostgreSQL and SQLite (#95). Fields are chosen as forInsert()/Update(); fields passed toExclude()are neither inserted nor updated; the primary key is never updated; a zero single-column primary key is left out of the insertion (auto-increment). The primary key field is not filled after the callUpsert()for SQLite (INSERT ... ON CONFLICT (...) DO UPDATE SET ..., SQLite 3.24.0+)SelectQuery.ForUpdate(),ForShare()andForUpdateSkipLocked()appendFOR UPDATE,FOR SHAREorFOR UPDATE SKIP LOCKEDafterORDER BY/LIMIT/OFFSET. A locking read must be built from a transaction (tx.Select()): from aDBthe query returns an error on every database, SQLite included, since the lock would be released as soon as the statement ends. Inside a transaction the clause is left out for SQLite (no row locks); an error for SQL Server and Oracle, and when combined withUnion(),GroupBy()orDistinct(). OptionalLockQueryBuilderinterface for custom query builders (not added toQueryBuilder, so existing implementations keep compiling)Errors.Unwrap() []error, soerrors.Isanderrors.Assee each error of the list (e.g. the callback's error whenTransactional()also failed to roll back)Fixed
Transactional()andTransactionalContext()returnednilwhen the context was canceled while the callback ran. database/sql had already rolled the transaction back, so the callback's writes were lost while the caller was told they were committed. They now return the context error (context.Canceled/context.DeadlineExceeded). A callback that ends the transaction itself withtx.Commit()ortx.Rollback()and returnsnilstill getsnilUpsert()executed a plainINSERT: theON DUPLICATE KEY UPDATEclause and its parameters were shown bySQL()but never sent to the server, so an existing key failed with a duplicate-key error (the same bug was fixed for PostgreSQL in #70)Upsert()without constraint columns generatedON CONFLICT DO UPDATEwithout a conflict target, which PostgreSQL rejects; it now returns an error before executing (SQLite likewise)Upsert()with no column left to update besides the constraint columns generated an empty update clause; it now returns an error before executingGetTableName()(the defaultDB.TableMapper) ignored aTableName()method with a pointer receiver when given a struct value or a slice of struct values, e.g.db.Select().All(&[]Customer{})withoutFrom(), and fell back to the snake-case type name. Code that relied on the fallback name will now query the table returned byTableName()Like()and the other LIKE expressions never matched an escaped%or_(e.g.Like("name", "50%")did not match "50% off"): the values are escaped with a backslash, but SQLite has no default LIKE escape character. AnESCAPE '\'clause is now added on SQLite only (MySQL and PostgreSQL use the backslash by default)Changed
Upsert()leaves the constraint columns out of the update part on every database. On MySQLconstraintswas ignored before: now a row matched through another unique index keeps its own values in the constraint columns (e.g. its primary key). On PostgreSQL the conflict columns were assigned their own values, a no-op except forUPDATE OFtriggers and case-insensitive collationsUpsert()(andModelQuery.Upsert()) now update the existing row from the proposed row —col=EXCLUDED.colon PostgreSQL and SQLite,col=VALUES(col)on MySQL — instead of binding every value a second time. The update stores exactly what the insert would have stored, and each value is sent once. PostgreSQL behaviour change:BEFORE INSERTtriggers now apply on the update path too (before, the update stored the values as passed), and an expression value such asnextval(...),random()or a function with side effects is evaluated once instead of twice (before, the second evaluation was stored and a sequence lost a value). On MySQL,VALUES()is deprecated since 8.0.20 (warning 1287) but is the only form that works on MySQL 5.7, 8 and 9 and on MariaDBNot included
go-sql-driver/mysql(v1.10.1),filippo.io/edwards25519(v1.2.0) andstretchr/objx(v0.8.0) require Go 1.24+, while ozzo-dbx still supports Go 1.22 (CI runs 1.22 and 1.23). They will come with a minimum Go version bump.hits = hits + 1) andDO NOTHING; batch upsert.Testing
database/sqldriver.Exclude(), aNULLin a unique column, aBEFORE INSERTtrigger, an expression evaluated once), locking reads (a secondFOR UPDATEtimes out,SKIP LOCKEDskips locked rows, two concurrent workers claim disjoint rows), a canceled transaction commits nothing, and LIKE with an escaped%.modernc.org/sqlite(pure Go, test-only dependency of theintegrationmodule): an upsert expression is evaluated once per conflicting row.mysql:innovation), to catch an eventual removal ofVALUES().Closes #95