diff --git a/modules/lagoon/README.md b/modules/lagoon/README.md index f625d42..206e3fd 100644 --- a/modules/lagoon/README.md +++ b/modules/lagoon/README.md @@ -14,7 +14,7 @@ Postgres data layer: the shared GORM connection, per-plugin migrations, model he - One shared pool: `lagoon.Open`, `lagoon.Use` and `lagoon.OpenFromApp` return a `*sql.DB` and a `*gorm.DB` built on that same pool; `lagoon.Publish` makes both available on the `backpack.App`. - Database-ready hooks: `lagoon.OnDatabase` runs a callback with the pool and GORM handle as soon as the database is published, immediately when it already is, otherwise when `lagoon.Publish` runs. Plugins register GORM callbacks through it from Boot, which runs before the `serve` command publishes the database. -- After-commit work: `lagoon.Transaction` runs a function in a transaction and then the callbacks registered with `lagoon.AfterCommit`, in order, only after the commit succeeds; a nested `lagoon.Transaction` is a savepoint whose callbacks are dropped with it when it fails. A single-statement write for which GORM opens its own transaction runs its `lagoon.AfterCommit` callbacks from the `lagoon:after_commit` GORM callback (`lagoon.AfterCommitCallback`) once GORM commits, and never when the write fails. Outside both, including inside a plain GORM `Transaction`, `lagoon.AfterCommit` runs the callback immediately. A panicking callback is logged and never turns a committed write into an error. +- After-commit work: `lagoon.Transaction` runs a function in a transaction and then the callbacks registered with `lagoon.AfterCommit`, in order, only after the commit succeeds; a nested `lagoon.Transaction` is a savepoint whose callbacks are dropped with it when it fails. A single-statement write for which GORM opens its own transaction runs its `lagoon.AfterCommit` callbacks from the `lagoon:after_commit` GORM callback (`lagoon.AfterCommitCallback`) once GORM commits, and never when the write fails. Outside both, including inside a plain GORM `Transaction`, `lagoon.AfterCommit` runs the callback immediately on that transaction's connection. The handle a callback receives always has an empty statement on the connection its work belongs to, so a query through it, even one that starts with `WithContext`, never continues from the written model's statement. A panicking callback is logged and never turns a committed write into an error. - Database check at connect time: `lagoon.CheckLocale` refuses a database whose default collation is not the ICU `pl-PL` locale, so ordering matches the database default without per-query `COLLATE`. - Per-plugin migrations: `lagoon.Migrate` runs the framework's `system_files` set (`attach.Migrations`), backend admin identity set (`lagoon.BackendAdminMigrations`) and job-queue set (`lagoon.QueueMigrations`: River's schema pinned at `lagoon.RiverSchemaVersion`, then the `lagoon.JobsTable` record table, under the `lagoon.QueueHistoryID` history), then every `pact.HasMigrations` set in plugin activation order, each in its own `summer_migrations_` history table (`lagoon.HistoryTableName`). `lagoon.RollbackLast` and `lagoon.Status` cover rollback and history. - Mass assignment: `lagoon.Fill` copies only allow-listed keys onto a model by GORM column name and silently drops the rest, logging each dropped key once outside production. A `json.Number` (from a decoder using `UseNumber`) fills integer, unsigned and float fields. A value that does not fit its column (a fraction, an exponent or an overflow for an integer field, or a value of the wrong type) is a `lagoon.FillTypeError` naming the key, so a caller can answer it as a validation failure on that field. `lagoon.HasFillable` and `lagoon.HasHidden` are the Go forms of `$fillable` and `$hidden`. diff --git a/modules/lagoon/transaction.go b/modules/lagoon/transaction.go index 929f8d7..254d60b 100644 --- a/modules/lagoon/transaction.go +++ b/modules/lagoon/transaction.go @@ -70,7 +70,7 @@ func Transaction(ctx context.Context, gdb *gorm.DB, fn func(ctx context.Context, }); err != nil { return err } - runAfterCommit(ctx, gdb.Session(&gorm.Session{NewDB: true, Context: ctx}), buf.take()) + runAfterCommit(ctx, cleanHandle(gdb, ctx), buf.take()) return nil } @@ -80,7 +80,13 @@ func Transaction(ctx context.Context, gdb *gorm.DB, fn func(ctx context.Context, // example from a GORM create callback) it runs after that commit through the // AfterCommitCallback callback, and not at all when the write fails. // Anywhere else, including inside a plain gorm Transaction, fn runs -// immediately with db. +// immediately on db's connection. +// +// The handle fn receives always has an empty statement on the connection +// the work belongs to (the pool after a commit, the open transaction +// inside a plain gorm Transaction), whatever handle AfterCommit was called +// with. Queries through it never continue from the written model's +// statement, even when they start with WithContext. func AfterCommit(ctx context.Context, db *gorm.DB, fn func(ctx context.Context, db *gorm.DB)) { if fn == nil { return @@ -108,7 +114,7 @@ func AfterCommit(ctx context.Context, db *gorm.DB, fn func(ctx context.Context, return } } - runAfterCommit(ctx, db, []func(context.Context, *gorm.DB){fn}) + runAfterCommit(ctx, cleanHandle(db, ctx), []func(context.Context, *gorm.DB){fn}) } func bufferFrom(ctx context.Context, db *gorm.DB) *afterCommitBuffer { @@ -143,7 +149,21 @@ func flushStatementAfterCommit(db *gorm.DB) { if ctx == nil { ctx = context.Background() } - runAfterCommit(ctx, db.Session(&gorm.Session{NewDB: true, Context: ctx}), fns) + runAfterCommit(ctx, cleanHandle(db, ctx), fns) +} + +// cleanHandle returns a handle on db's connection with an empty statement. +// db.Session with NewDB and a Context is not enough on a callback's +// handle: the Context makes it clone the write's statement (model, table, +// clauses), and a later WithContext on the result continues from that +// clone, so a query would run against the written model's table. Clauses() +// starts a fresh statement on the same connection, and the final Session +// makes the next chained call start from it again. A nil db stays nil. +func cleanHandle(db *gorm.DB, ctx context.Context) *gorm.DB { + if db == nil { + return nil + } + return db.Session(&gorm.Session{NewDB: true, Context: ctx}).Clauses().Session(&gorm.Session{NewDB: true}) } func runAfterCommit(ctx context.Context, db *gorm.DB, fns []func(context.Context, *gorm.DB)) { diff --git a/modules/lagoon/transaction_test.go b/modules/lagoon/transaction_test.go index 709e65d..7efa193 100644 --- a/modules/lagoon/transaction_test.go +++ b/modules/lagoon/transaction_test.go @@ -18,6 +18,13 @@ type acItem struct { func (acItem) TableName() string { return "lagoon_ac_items" } +type acOther struct { + ID uint `gorm:"column:id;primaryKey"` + Label string `gorm:"column:label"` +} + +func (acOther) TableName() string { return "lagoon_ac_others" } + // recorder collects after-commit callback names and whether the row they // were registered for was visible to another connection when they ran. type recorder struct { @@ -188,8 +195,11 @@ func TestTransactionAfterCommit(t *testing.T) { var got *gorm.DB err := gdb.WithContext(ctx).Transaction(func(tx *gorm.DB) error { AfterCommit(ctx, tx, func(_ context.Context, d *gorm.DB) { got = d }) - if got != tx { - t.Error("callback did not run immediately with the tx handle") + if got == nil { + t.Fatal("callback did not run immediately") + } + if got.Statement.ConnPool != tx.Statement.ConnPool { + t.Error("callback did not run on the transaction's connection") } return nil }) @@ -198,6 +208,65 @@ func TestTransactionAfterCommit(t *testing.T) { } }) + // The handle a callback receives has an empty statement on the write's + // connection: a query through it (with WithContext, as application code + // writes it) must not continue from the written model's statement. + t.Run("callback_handle_has_a_clean_statement", func(t *testing.T) { + if err := gdb.Exec(`CREATE TABLE lagoon_ac_others (id SERIAL PRIMARY KEY, label TEXT NOT NULL)`).Error; err != nil { + t.Fatal(err) + } + if err := gdb.Exec(`INSERT INTO lagoon_ac_others (label) VALUES ('other')`).Error; err != nil { + t.Fatal(err) + } + g3, err := Use(ctx, db) + if err != nil { + t.Fatal(err) + } + var mu sync.Mutex + labels := map[string]string{} + if err := g3.Callback().Create().Before("gorm:create").Register("lagoon_test:clean_handle", func(stmt *gorm.DB) { + item, ok := stmt.Statement.Dest.(*acItem) + if !ok { + return + } + name := item.Name + AfterCommit(stmt.Statement.Context, stmt, func(ctx context.Context, d *gorm.DB) { + var o acOther + label := "" + if err := d.WithContext(ctx).Take(&o).Error; err != nil { + label = "error: " + err.Error() + } else { + label = o.Label + } + mu.Lock() + labels[name] = label + mu.Unlock() + }) + }); err != nil { + t.Fatal(err) + } + if err := g3.WithContext(ctx).Create(&acItem{Name: "clean-implicit"}).Error; err != nil { + t.Fatal(err) + } + if err := g3.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + return tx.Create(&acItem{Name: "clean-plain-tx"}).Error + }); err != nil { + t.Fatal(err) + } + if err := Transaction(ctx, g3, func(ctx context.Context, tx *gorm.DB) error { + return tx.WithContext(ctx).Create(&acItem{Name: "clean-lagoon-tx"}).Error + }); err != nil { + t.Fatal(err) + } + mu.Lock() + defer mu.Unlock() + for _, name := range []string{"clean-implicit", "clean-plain-tx", "clean-lagoon-tx"} { + if got := labels[name]; got != "other" { + t.Errorf("%s: query through the callback handle read %q, want the lagoon_ac_others row \"other\"", name, got) + } + } + }) + t.Run("outside_transaction_runs_now", func(t *testing.T) { ran := false AfterCommit(ctx, gdb, func(context.Context, *gorm.DB) { ran = true })