test(model): use non-aggregate terminals in forUpdate chain specs - #3375
Conversation
The two forUpdate() chain-entry specs added by PR #3368 used .count() as the terminal. Postgres and CockroachDB reject aggregate functions combined with FOR UPDATE, so every postgres/cockroachdb leg in the compat matrix failed with 'FOR UPDATE is not allowed with aggregate functions'. The specs pin chain-entry dispatch, not locking semantics, so a non-aggregate .get() terminal asserts the same behavior on every engine. Verified locally on lucee7 + cockroachdb (previously failing leg): 4775 pass / 0 fail / 0 error; lucee7 + sqlite: 4763 pass / 0 fail / 0 error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <petera@pai.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR swaps the .count() terminal for a non-aggregate .get() in the two forUpdate() chain-entry specs in queryBuilderSpec.cfc, because Postgres and CockroachDB reject SELECT COUNT(*) ... FOR UPDATE (FOR UPDATE is not allowed with aggregate functions), which was zeroing out those legs across the whole compat matrix. The specs pin chain-entry dispatch, not locking semantics, so a non-aggregate terminal asserts the same behavior while being legal on every supported engine. Verdict: approve.
Correctness. The substitution is sound. QueryBuilder.get() delegates to findAll() (vendor/wheels/model/query/QueryBuilder.cfc:327-341), which returns a query object, so expect(result.recordcount).toBe(1) is the right assertion shape — and it mirrors the pre-existing distinct() chain-entry test three cases up (queryBuilderSpec.cfc:272-277), which already uses exactly .get() + expect(result.recordcount).toBe(1). Both edited specs still exercise the intended path: chain entry from the model, and transition from a scope chain into the builder. The FOR UPDATE clause is still emitted by the builder; only the terminal aggregate is removed, so the cross-engine SQL restriction is genuinely dodged rather than masked.
Tests. Test-only change; no production code touched. The added comments document why the terminal must be non-aggregate — the useful bit for the next person who reaches for .count() here.
Commits. test(model): use non-aggregate terminals in forUpdate chain specs conforms to commitlint.config.js (type test, scope model, subject not ALL-CAPS, header <= 100 chars) and describes the why.
No cross-engine, convention, or security concerns. Nice tight fix.
What
PR #3368's two
forUpdate()chain-entry specs used.count()as the chain terminal. Postgres and CockroachDB rejectSELECT COUNT(*) ... FOR UPDATE(`FOR UPDATE is not allowed with aggregate functions`), which broke the postgres and cockroachdb legs on every engine in the compat matrix (first surfaced by dispatch run 30972608328 on the #3365 branch after it picked up today's merges).The specs pin chain-entry dispatch, not locking semantics, so switching to a non-aggregate
.get()terminal asserts the same behavior and is legal on all supported engines.Test evidence
Run locally against the previously failing combination (lucee7 container + cockroachdb):
Spec-only change; unblocks the #3302 chain (Refs #3302, refs #3368).
🤖 Generated with Claude Code