diff --git a/docs/backend/admin-controllers.md b/docs/backend/admin-controllers.md index 321210e..6a39edd 100644 --- a/docs/backend/admin-controllers.md +++ b/docs/backend/admin-controllers.md @@ -217,6 +217,8 @@ Behaviour overrides such as `formBeforeCreate` or `listExtendQuery` become optio | `pact.FormBeforeDelete`, `pact.FormAfterDelete` | Around a delete, inside its transaction. | | `pact.DropdownOptionsProvider` | Supplies the options of a `dropdown` field that names a method. | +A hook or scope that has to read the database during a write should use the transaction the write runs in, `cabana.TxFromContext(ctx)`, rather than the application pool: it then sees the write's own snapshot and takes no second connection while the transaction holds row locks. The handle is valid only until the hook returns, and the list route, which runs no transaction, reports none. + Scope reads and writes with `pact.ListExtendQuery` and `pact.FormExtendQuery` rather than checking in a hook: the scope then applies to every route, including relation and action routes. ## Toolbar actions diff --git a/modules/cabana/README.md b/modules/cabana/README.md index 5bae324..0ac7f1b 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -153,6 +153,7 @@ func (p *Plugin) AdminFS() fs.FS { return adminFS } | `cabana.AdminRelationContractProvider` / `cabana.RelationContract` | Controller-supplied bindings for relation managers. | | `cabana.BackendUser` / `cabana.BackendUserRole` / `cabana.BackendUsers` | GORM models of the backend user tables and the principal loader used by the guard. | | `cabana.Allows` | Checks a principal against required permission codes. | +| `cabana.TxFromContext` | The transaction a write route is running in, from the context of a lifecycle hook or scope. | | `cabana.WriteData` / `cabana.WriteError` / `cabana.WriteErrorDetails` | Write the admin success and error envelopes. | | `cabana.ValidationError` / `cabana.ListValidationError` | Field-level `validation_failed` errors. A `pact.AdminAction` may return a `cabana.ValidationError` to answer 422. | | `cabana.AdminActionRequest` | Body of an action route: optional `record_id` and the widget's `values`. Unknown keys are refused. | diff --git a/modules/cabana/crud.go b/modules/cabana/crud.go index 0d37382..688a94d 100644 --- a/modules/cabana/crud.go +++ b/modules/cabana/crud.go @@ -152,6 +152,7 @@ func (s CRUDService) Delete(ctx context.Context, cc *CompiledController, id any) } var result BulkResult err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + ctx = withTx(ctx, tx) target, err := newWritableModel(cc) if err != nil { return err @@ -199,6 +200,7 @@ func (s CRUDService) BulkDelete(ctx context.Context, cc *CompiledController, in } var result BulkResult err = lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + ctx = withTx(ctx, tx) if err := ctx.Err(); err != nil { return lifecycleFailure(cc, err) } @@ -307,6 +309,7 @@ func (s CRUDService) save(ctx context.Context, cc *CompiledController, id any, i } var result RecordResult err = lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + ctx = withTx(ctx, tx) target, err := newWritableModel(cc) if err != nil { return err diff --git a/modules/cabana/crud_lifecycle_test.go b/modules/cabana/crud_lifecycle_test.go index e500267..acd1c3f 100644 --- a/modules/cabana/crud_lifecycle_test.go +++ b/modules/cabana/crud_lifecycle_test.go @@ -26,6 +26,9 @@ type hookController struct { fail string queries atomic.Int32 allocs atomic.Int32 + // txHooks counts the Before hooks that found the write's transaction on + // their context through TxFromContext. + txHooks atomic.Int32 } func (c *hookController) ID() string { return "acme.demo.records" } @@ -58,6 +61,7 @@ func (c *hookController) FormExtendQuery(ctx context.Context, db *gorm.DB) *gorm } func (c *hookController) FormBeforeCreate(ctx context.Context, model any) error { recordHook(ctx, "form_before_create") + c.noteTx(ctx) if c.trackScope { if row, ok := model.(*crudRow); ok { if scope, ok := ctx.Value(scopeKey{}).(uint); ok { @@ -73,6 +77,7 @@ func (c *hookController) FormAfterCreate(ctx context.Context, model any) error { } func (c *hookController) FormBeforeUpdate(ctx context.Context, model any) error { recordHook(ctx, "form_before_update") + c.noteTx(ctx) return c.hookErr("form_before_update") } func (c *hookController) FormAfterUpdate(ctx context.Context, model any) error { @@ -81,12 +86,18 @@ func (c *hookController) FormAfterUpdate(ctx context.Context, model any) error { } func (c *hookController) FormBeforeDelete(ctx context.Context, model any) error { recordHook(ctx, "form_before_delete") + c.noteTx(ctx) return c.hookErr("form_before_delete") } func (c *hookController) FormAfterDelete(ctx context.Context, model any) error { recordHook(ctx, "form_after_delete") return c.hookErr("form_after_delete") } +func (c *hookController) noteTx(ctx context.Context) { + if _, ok := TxFromContext(ctx); ok { + c.txHooks.Add(1) + } +} func (c *hookController) hookErr(name string) error { if c != nil && c.fail == name { return errHookBoom @@ -558,3 +569,31 @@ func TestCRUDOperationsFollowDeclarations(t *testing.T) { t.Fatalf("declared bulk delete=%d %s", got.Code, got.Body.String()) } } + +// TestHooksReceiveTheWriteTransaction pins WR-19: lifecycle hooks can reach the +// transaction their write runs in through TxFromContext, so a hook's own reads +// share the write's snapshot and connection instead of using the app pool. +func TestHooksReceiveTheWriteTransaction(t *testing.T) { + _, httpSvc, _, _, hooks := hookFixture(t) + ctx := principalCtx(hooks, superUser()) + created := crudCall(httpSvc, http.MethodPost, "", []byte(`{"name":"Ada"}`), ctx) + if created.Code != http.StatusCreated { + t.Fatalf("create=%d %s", created.Code, created.Body.String()) + } + id := uintString(decodeData(t, created.Body.Bytes())["id"]) + if updated := crudCall(httpSvc, http.MethodPut, id, []byte(`{"name":"Bea"}`), ctx); updated.Code != http.StatusOK { + t.Fatalf("update=%d %s", updated.Code, updated.Body.String()) + } + if deleted := crudCall(httpSvc, http.MethodDelete, id, nil, ctx); deleted.Code != http.StatusOK { + t.Fatalf("delete=%d %s", deleted.Code, deleted.Body.String()) + } + if got := hooks.txHooks.Load(); got != 3 { + t.Fatalf("Before hooks that saw the transaction = %d, want 3 (create, update, delete)", got) + } + if _, ok := TxFromContext(context.Background()); ok { + t.Fatal("a bare context reported a transaction") + } + if _, ok := TxFromContext(nil); ok { //nolint:staticcheck // a nil context must not panic + t.Fatal("a nil context reported a transaction") + } +} diff --git a/modules/cabana/relation.go b/modules/cabana/relation.go index b9084cc..4021cdf 100644 --- a/modules/cabana/relation.go +++ b/modules/cabana/relation.go @@ -633,6 +633,7 @@ func (s RelationService) Link(ctx context.Context, cc *CompiledController, relat } var result RelationMutationResult err = lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + ctx = withTx(ctx, tx) parent, err := newWritableModel(cc) if err != nil { return err @@ -702,6 +703,7 @@ func (s RelationService) Unlink(ctx context.Context, cc *CompiledController, rel } var result RelationMutationResult err = lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error { + ctx = withTx(ctx, tx) parent, err := newWritableModel(cc) if err != nil { return err diff --git a/modules/cabana/tx_context.go b/modules/cabana/tx_context.go new file mode 100644 index 0000000..791edde --- /dev/null +++ b/modules/cabana/tx_context.go @@ -0,0 +1,33 @@ +package cabana + +import ( + "context" + + "gorm.io/gorm" +) + +type txContextKey struct{} + +// withTx returns ctx carrying the transaction a write is running in. +func withTx(ctx context.Context, tx *gorm.DB) context.Context { + if tx == nil { + return ctx + } + return context.WithValue(ctx, txContextKey{}, tx) +} + +// TxFromContext returns the database transaction a write route is running in, +// for the context handed to a lifecycle hook (pact.FormBeforeCreate and the +// other Form and Relation hooks) or a scope (pact.FormExtendQuery) that runs +// inside it. A hook that has to read the database should use this handle +// instead of the application pool: it then sees the same snapshot as the write +// and does not take a second connection while the transaction holds row locks. +// The handle is valid only until the hook returns. The second result is false +// on routes that do not run in a transaction, such as the list. +func TxFromContext(ctx context.Context) (*gorm.DB, bool) { + if ctx == nil { + return nil, false + } + tx, ok := ctx.Value(txContextKey{}).(*gorm.DB) + return tx, ok && tx != nil +}