diff --git a/changelog.d/4429-request-cache-cross-model.fixed.md b/changelog.d/4429-request-cache-cross-model.fixed.md new file mode 100644 index 0000000000..79a1be45ad --- /dev/null +++ b/changelog.d/4429-request-cache-cross-model.fixed.md @@ -0,0 +1 @@ +- The per-request finder cache (`cacheQueriesDuringRequest`) is now cleared across every model on any ORM write and on a transaction rollback, so a cached `include=` query no longer returns a joined model's pre-write columns — or a rolled-back transaction's phantom rows — for the rest of the request (#4429) diff --git a/vendor/wheels/global/cache.cfm b/vendor/wheels/global/cache.cfm index c9909b3b78..a9e35ada21 100644 --- a/vendor/wheels/global/cache.cfm +++ b/vendor/wheels/global/cache.cfm @@ -542,6 +542,12 @@ * forgetCachedQueries("Post") clears just the Post model's slot, by name (works anywhere) * forgetCachedQueries(all = true) clears every model's cached results this request * + * This is only needed after RAW writes (`queryExecute(...)`) — ORM writes (create / update / delete + * and the `*All` / `*ByKey` forms) already clear the whole request cache themselves (#4429). After a + * raw write, prefer `all = true` when the tables you wrote are joined by other models' `include=` + * queries: a cross-model result is cached in the *base* model's slot, so the single-model named form + * misses it. The named form is for a raw single-table write whose table no other cached query joins. + * * Called as a method on a model instance it scopes to that model automatically, because only a model * carries `variables.wheels.class.modelName` (Controller / view / job / base Global do not). Called * outside a model with neither a `modelName` nor `all`, it throws rather than silently wiping every @@ -589,8 +595,7 @@ if (arguments.all) { StructDelete(request.wheels, "$queryCache"); } else { - // Empty just this model's slot, keeping the key — the same shape $clearRequestCache - // leaves behind, so a re-query repopulates it in place. + // Empty just this model's slot, keeping the key, so a re-query repopulates it in place. request.wheels["$queryCache"][local.slot] = {}; } } diff --git a/vendor/wheels/model/miscellaneous.cfm b/vendor/wheels/model/miscellaneous.cfm index 091dd0a3a1..a2672df787 100644 --- a/vendor/wheels/model/miscellaneous.cfm +++ b/vendor/wheels/model/miscellaneous.cfm @@ -21,11 +21,22 @@ } /** - * Deletes all queries stored during the request for this model. + * Clears the ENTIRE per-request query cache — every model's slot, not just this model's (#4429). + * + * A finder's result is cached in the base model's slot keyed by its SQL, so a query that joins + * another model through `include=` lives in the base model's slot. Clearing only the writing + * model's slot left such a cross-model query returning this model's pre-write columns for the rest + * of the request. Every ORM write path funnels through here (create / update / delete / *All / + * *ByKey / the #4397 soft-delete UPDATE via $runDeleteStatement), so clearing all of it here means + * no write path can forget the cross-model case. The cache is request-scoped, so re-running the + * affected reads after a write is both correct and cheap. Raw (non-ORM) writes are the caller's job + * via `forgetCachedQueries(all = true)`. */ public void function $clearRequestCache() { - $ensureRequestQueryCache(); - request.wheels["$queryCache"][variables.wheels.class.modelName] = {}; + if (!StructKeyExists(request, "wheels")) { + request.wheels = {}; + } + request.wheels["$queryCache"] = {}; } /** diff --git a/vendor/wheels/model/transactions.cfm b/vendor/wheels/model/transactions.cfm index 849bf7c03c..0acf17ff98 100644 --- a/vendor/wheels/model/transactions.cfm +++ b/vendor/wheels/model/transactions.cfm @@ -167,6 +167,12 @@ ) { $resolveTransactionCallbacks(connection = arguments.connectionArgs, type = "afterRollback"); } + if (arguments.closeTransaction) { + // The transaction rolled back (the body threw). Reads made inside it may have cached + // uncommitted rows, so drop the whole request query cache — otherwise a later read in + // this request returns phantom data that never committed (#4429). + $clearRequestCache(); + } rethrow; } // Transaction block closed without an exception: fire afterCommit on commit, or @@ -182,6 +188,12 @@ connection = arguments.connectionArgs, type = local.ctx.txnState.rolledBack ? "afterRollback" : "afterCommit" ); + if (local.ctx.txnState.rolledBack) { + // A non-exception rollback (the method returned false, or transaction = "rollback"): + // same phantom-read risk as the throw path above, so drop the request query cache. + // A commit does not need this — its reads are of committed rows (#4429). + $clearRequestCache(); + } } return local.ctx.rv; } @@ -414,11 +426,16 @@ // A rollback that itself fails is logged, never allowed to replace the // method's exception, which is what the caller needs to see. $rollbackToSavepointQuietly(name = local.savepoint, connection = local.connectionArgs, mark = local.mark); + // The savepoint unit's reads may have cached rows it just rolled back; clear the request + // cache so the outer transaction, which carries on, does not serve them as phantoms (#4429). + $clearRequestCache(); rethrow; } // Same failure test as the commit branch: a numeric count (0-row bulk op) is not a failure. if (!IsBoolean(local.rv) || (!IsNumeric(local.rv) && !local.rv)) { $rollbackToSavepoint(name = local.savepoint, connection = local.connectionArgs, mark = local.mark); + // Same phantom-read risk as the exception path above (#4429). + $clearRequestCache(); } if (!IsBoolean(local.rv)) { Throw( diff --git a/vendor/wheels/tests/_assets/models/Author.cfc b/vendor/wheels/tests/_assets/models/Author.cfc index 6a819c2d6d..19a549ded7 100644 --- a/vendor/wheels/tests/_assets/models/Author.cfc +++ b/vendor/wheels/tests/_assets/models/Author.cfc @@ -19,4 +19,34 @@ component extends="Model" { return true; } + // #4429 rollback-cache probe: inside a transaction, write (uncommitted) then read — caching the + // uncommitted row — and return false so the transaction rolls back. The spec then checks that the + // phantom row is not still served from the request cache after the rollback. + public boolean function $cacheRollbackProbe4429() { + this.update(firstName = "Phantom4429", transaction = "none"); + model("Author").findByKey(key = this.id); + return false; + } + + // #4429 throw-path rollback probe: write + read (caching the uncommitted row) then THROW, so the + // transaction rolls back through the exception branch rather than the returned-false branch. + public boolean function $throwRollbackProbe4429() { + this.update(firstName = "PhantomThrow4429", transaction = "none"); + model("Author").findByKey(key = this.id); + Throw(type = "Test.Rollback4429", message = "rollback the throw-path probe"); + } + + // #4429 savepoint-rollback probe: the inner unit (run as a savepoint) writes + reads the uncommitted + // row then returns false so only the savepoint rolls back; the outer transaction carries on. + public boolean function $savepointInner4429() { + this.update(firstName = "PhantomSavepoint4429", transaction = "none"); + model("Author").findByKey(key = this.id); + return false; + } + + public boolean function $savepointOuter4429() { + this.invokeWithTransaction(method = "$savepointInner4429", transaction = "savepoint"); + return true; + } + } diff --git a/vendor/wheels/tests/specs/model/requestQueryCacheSpec.cfc b/vendor/wheels/tests/specs/model/requestQueryCacheSpec.cfc index 6c6cd54979..028021a534 100644 --- a/vendor/wheels/tests/specs/model/requestQueryCacheSpec.cfc +++ b/vendor/wheels/tests/specs/model/requestQueryCacheSpec.cfc @@ -36,7 +36,11 @@ component extends="wheels.WheelsTest" { it("does not store query results when cacheQueriesDuringRequest is disabled", () => { application.wheels.cacheQueriesDuringRequest = false; model("author").findAll(where = "lastName = 'Djurner'"); - expect(StructCount(request.wheels["$queryCache"]["author"])).toBe(0); + // beforeEach's $clearRequestCache now empties the whole $queryCache (#4429), so with the + // cache disabled the author slot may be absent rather than present-but-empty; either way + // nothing was cached for author. + var cache = request.wheels["$queryCache"]; + expect(StructKeyExists(cache, "author") ? StructCount(cache["author"]) : 0).toBe(0); }) it("findEach does not accumulate per-batch queries in the request cache", () => { diff --git a/vendor/wheels/tests/specs/model/requestQueryCacheTenantCollisionSpec.cfc b/vendor/wheels/tests/specs/model/requestQueryCacheTenantCollisionSpec.cfc index c4b97139bc..4dc1a87767 100644 --- a/vendor/wheels/tests/specs/model/requestQueryCacheTenantCollisionSpec.cfc +++ b/vendor/wheels/tests/specs/model/requestQueryCacheTenantCollisionSpec.cfc @@ -100,7 +100,7 @@ component extends="wheels.WheelsTest" { expect(g.$tenantDataSource()).toBe(application.wheels.dataSourceName); }) - it("clears only its own model's cache, leaving sibling models untouched", () => { + it("clears every model's cache, not just its own, so cross-model include= results can't go stale", () => { application.wheels.cacheQueriesDuringRequest = true; model("author").findAll(where = "lastName = 'Djurner'"); model("Tenant").findAll(where = "lastName = 'Djurner'"); @@ -108,8 +108,10 @@ component extends="wheels.WheelsTest" { model("Tenant").$clearRequestCache(); - expect(StructCount(request.wheels["$queryCache"]["Tenant"])).toBe(0); - expect(StructCount(request.wheels["$queryCache"]["author"])).toBe(1); + // #4429: $clearRequestCache now empties the whole request cache, siblings included — a query + // that joins another model via include= is cached under the base model, so a write to one + // model has to clear the others or they keep serving its pre-write rows. + expect(StructIsEmpty(request.wheels["$queryCache"])).toBeTrue(); }) }) diff --git a/vendor/wheels/tests/specs/requestcache/RequestCacheInvalidationSpec.cfc b/vendor/wheels/tests/specs/requestcache/RequestCacheInvalidationSpec.cfc new file mode 100644 index 0000000000..3f9596675d --- /dev/null +++ b/vendor/wheels/tests/specs/requestcache/RequestCacheInvalidationSpec.cfc @@ -0,0 +1,117 @@ +/** + * #4429: the per-request finder cache (cacheQueriesDuringRequest) must be invalidated across models + * on any ORM write, and on a transaction rollback — not just in the writing model's own slot. + * + * The core runner forces cacheQueriesDuringRequest OFF (tests/runner.cfm), so these specs turn it ON + * themselves. The setting is read at call time in model/read.cfm, so a per-group flip works. beforeEach + * flips it on; afterEach restores it AND restores the one post + author these specs mutate, and wipes + * request.wheels["$queryCache"] — all of that runs even when a spec fails, so a failing spec can't leak + * a mutated row or a cached row into the next spec (the whole suite runs in one request). Catch writes + * would use `var state` (cross-engine invariant 11); none are needed here. + */ +component extends="wheels.WheelsTest" { + + function beforeAll() { + var wo = application.wo; + var post = wo.model("Post").findOne(where = "authorid IS NOT NULL", order = "id", returnAs = "query"); + variables.postId = post.id; + variables.postTitle = post.title; + variables.authorId = post.authorid; + variables.authorFirstName = wo.model("Author").findByKey(variables.authorId).firstName; + } + + function run() { + + describe("request query cache invalidation (4429)", () => { + + beforeEach(() => { + variables.g = application.wo; + variables.cacheWas = application.wheels.cacheQueriesDuringRequest; + application.wheels.cacheQueriesDuringRequest = true; + $wipeRequestQueryCache(); + }); + + afterEach(() => { + // Restore the rows the specs mutate, unconditionally, so a failed spec leaves no pollution. + g.model("Post").findByKey(variables.postId).update(title = variables.postTitle); + g.model("Author").findByKey(variables.authorId).update(firstName = variables.authorFirstName); + application.wheels.cacheQueriesDuringRequest = variables.cacheWas; + $wipeRequestQueryCache(); + }); + + it("serves a repeated read from cache when no ORM write intervenes (cache really is on)", () => { + var first = g.model("Post").findAll(where = "id = #variables.postId#", returnAs = "query").title; + // A RAW write bypasses ORM invalidation, so with the cache ON the next read is still the + // cached value — proof the cache is active (and why raw writes need forgetCachedQueries). + $rawPostTitle(variables.postId, "RawTitle4429"); + var second = g.model("Post").findAll(where = "id = #variables.postId#", returnAs = "query").title; + expect(second).toBe(first); + expect(second).notToBe("RawTitle4429"); + }); + + it("an ORM write to a model clears that model's own cached finder", () => { + g.model("Post").findAll(where = "id = #variables.postId#", returnAs = "query"); + g.model("Post").findByKey(variables.postId).update(title = "Changed4429a"); + expect(g.model("Post").findAll(where = "id = #variables.postId#", returnAs = "query").title).toBe("Changed4429a"); + }); + + it("an ORM write to an INCLUDED model clears a base model's include= cached query", () => { + var sel = "c_o_r_e_posts.id, c_o_r_e_authors.firstName AS authorName4429"; + // Cache the Post finder that joins Author (lives in the Post slot, keyed by this SQL). + g.model("Post").findAll(where = "c_o_r_e_posts.id = #variables.postId#", include = "author", select = sel, returnAs = "query"); + // An ORM write to the INCLUDED model. Pre-fix this cleared only the Author slot, so the + // Post slot kept serving the pre-write author column for the rest of the request. + g.model("Author").findByKey(variables.authorId).update(firstName = "Changed4429b"); + var after = g.model("Post").findAll(where = "c_o_r_e_posts.id = #variables.postId#", include = "author", select = sel, returnAs = "query").authorName4429; + expect(after).toBe("Changed4429b"); + }); + + it("a transaction rollback clears reads cached inside it (no phantom rows)", () => { + // The probe writes then reads inside the transaction (caching the uncommitted row), then + // returns false to roll back. Pre-fix the phantom row stayed cached after the rollback. + g.model("Author").findByKey(variables.authorId).invokeWithTransaction(method = "$cacheRollbackProbe4429", transaction = "rollback"); + expect(g.model("Author").findByKey(variables.authorId).firstName).toBe(variables.authorFirstName); + }); + + it("a rollback through the exception (throw) path clears cached reads too", () => { + // The probe throws instead of returning false, so the transaction rolls back through the + // begin-catch branch of $runInTransaction, which must clear the cache just like the other path. + var state = {threw = false}; + try { + g.model("Author").findByKey(variables.authorId).invokeWithTransaction(method = "$throwRollbackProbe4429", transaction = "commit"); + } catch (any e) { + state.threw = true; + } + expect(state.threw).toBeTrue(); + expect(g.model("Author").findByKey(variables.authorId).firstName).toBe(variables.authorFirstName); + }); + + it("a savepoint rollback clears reads cached inside the savepoint (no phantom), outer carries on", () => { + // The outer transaction commits; inside it a savepoint unit writes + reads (caching the + // uncommitted row) then rolls back. Pre-fix the savepoint's phantom row stayed cached while + // the outer transaction continued and committed. + g.model("Author").findByKey(variables.authorId).invokeWithTransaction(method = "$savepointOuter4429", transaction = "commit"); + expect(g.model("Author").findByKey(variables.authorId).firstName).toBe(variables.authorFirstName); + }); + + }); + + } + + private void function $wipeRequestQueryCache() { + if (StructKeyExists(request, "wheels")) { + request.wheels["$queryCache"] = {}; + } + } + + // id is untyped and bound as cf_sql_bigint: CockroachDB ids are 64-bit, which overflow + // cf_sql_integer (and lose precision through a `numeric` arg) on Adobe (#4437 review). + private void function $rawPostTitle(required any id, required string title) { + queryExecute( + "UPDATE c_o_r_e_posts SET title = :title WHERE id = :id", + {title = {value = arguments.title}, id = {value = arguments.id, cfsqltype = "cf_sql_bigint"}}, + {datasource = application.wheels.dataSourceName} + ); + } + +} diff --git a/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/caching.mdx b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/caching.mdx index 88775ce2b5..b198e3bfaf 100644 --- a/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/caching.mdx +++ b/web/sites/guides/src/content/docs/v4-2-0/digging-deeper/caching.mdx @@ -193,6 +193,19 @@ Query caching is ideal when: - Per-request result variation is acceptable (a 5-minute-old revenue number is fine on a dashboard). - The view output depends on other per-request data (current user, feature flags) so whole-action caching won't work. +## The per-request query cache + +Separate from `findAll(cache=N)` above, Wheels keeps a second, much shorter-lived query cache that is **on by default**: `cacheQueriesDuringRequest`. Within a **single request**, the first time a finder runs a given SQL statement its result is remembered, and an identical finder later in the same request returns that result instead of hitting the database again. It is what keeps a view that calls the same finder in a loop — or across several partials — from running the query many times. The cache lives in `request` scope, so it never outlives the request and is never shared between requests, visitors, or nodes. + +You rarely interact with it directly, because Wheels keeps it correct for you: + +- **Any ORM write clears the whole per-request cache.** `create()`, `update()`, `delete()`, `save()`, and the bulk forms (`updateAll` / `deleteAll` / `updateByKey` / `deleteByKey` / `insertAll` / `upsertAll`) drop every cached finder result for the rest of the request — not just the written model's. This matters for `include=`: a query that joins another model is cached under the base model, so a write to the included model has to clear it too, or the base finder would keep returning the joined model's pre-write columns. +- **A transaction rollback clears it too.** A finder that ran inside a transaction can cache rows the transaction then rolled back; clearing on rollback keeps those phantom rows from being served later in the request. +- **`reload=true` bypasses it for one call:** `model("Post").findByKey(key=id, reload=true)` always goes to the database. +- **After a raw write you clear it yourself.** A `queryExecute("UPDATE …")` is invisible to the ORM, so cached finders can go stale. Call `forgetCachedQueries(all=true)` after a raw write when the tables you changed are joined by other models' `include=` queries; the single-model form `forgetCachedQueries("Post")` is enough for a raw single-table write that no other cached query joins. The same applies to a **raw `transaction {}` block that rolls back** — the automatic clear only fires for ORM transactions (`model.transaction()` / a write's `transaction=` argument), so after rolling back a raw block call `forgetCachedQueries(all=true)` to drop any rows it cached. + +Turn the whole thing off with `set(cacheQueriesDuringRequest=false)` in `config/settings.cfm` if you want every finder to hit the database. + ## Cache your own data When the slow part is a value you compute yourself (a total, a call to another service, a struct built from several queries), cache it with `appCacheFetch()`. It returns the cached value, or calls your function, caches what it returns and returns that: