perf: batch N+1 query loops in case-batch-update-service - #908
Merged
Merged
Conversation
The JSON-editor batch update path issued one database query per element
in several places instead of one batched query per operation. Batch:
- validateCreateParents: one findMany for all referenced parent ids
instead of one findUnique per create.
- validateUpdateParents: a single shared multi-root breadth-first sweep
(new getDescendantIdsForRoots in tree-traversal.ts) for circular-
reference checks, instead of one getDescendantIds walk per update.
- Level calculation in applyCreates/applyUpdates: parent {level,
elementType} fetched once per batch via a new fetchLevelInfo helper,
with a local map tracking levels recalculated earlier in the same
batch so intra-batch parent moves still resolve correctly.
- validateAssertionStatusChanges: the acting user's system-user flag is
resolved once per batch instead of once per create/update that sets
assertionStatus (element-service.ts's enforceAssertionStatusRules and
guardAssertionStatusWrite now take an optional pre-resolved flag).
- applyLinkEvidence/applyUnlinkEvidence: one createMany(skipDuplicates)
and one deleteMany(OR[...]) instead of a findFirst+create, or delete,
per link/unlink change.
applyDeletes and applyCreates' per-row element.create calls are left as
individual queries: deletes rely on delete() throwing on a missing row
to abort the transaction (deleteMany would silently no-op), and creates
must happen in parent-before-child order because of the self-referencing
foreign key.
All 20 existing case-batch-update-service tests, plus the full
integration suite (914 tests), pass unmodified.
…nce-idempotency gaps in batch update perf/n-plus-one-batching batched several previously-uncovered code paths in case-batch-update-service.ts. Adds coverage for the circular-reference check (validateUpdateParents' multi-root BFS), the intra-batch level-chaining map in applyUpdates, direct query-count assertions proving the N+1 batching (via a pg-level spy that observes tx-scoped queries, unlike a spy on the Prisma client), and evidence link/unlink idempotency under the new createMany/deleteMany implementation.
Fallow audit reportFound 2 findings. Details
Generated by fallow. |
Structural Quality gate fix round (fed real merged unit+integration
coverage):
- applyUpdates (cognitive complexity 20, threshold 15): extracted the
level-resolution branch into a named helper, resolveUpdateLevel, taking
the same pre-fetched per-batch maps (ownTypeById, parentInfoById,
recalculatedLevels) via a small UpdateLevelContext. Same resolution
rule, same order, same pinned test behaviour — only the shape changed.
- resolveParentLevel (CRAP 30.0, 0% coverage): diagnosed as live, not
dead. It's the level-resolution closure inside applyCreates, reachable
from applyBatchUpdate whenever a batch creates a PROPERTY_CLAIM with a
parentId — no existing test drove a batch CREATE through that shape
(existing PROPERTY_CLAIM fixtures were all pre-existing elements via
createTestElement, never a batch "create" change). Added one
integration test through the public applyBatchUpdate API that creates
a property claim under an existing element (the external-parent
branch) and a second claim under the first, created in the same batch
(the within-batch-parent branch), asserting both resulting levels.
Verified against real coverage (unit suite doesn't touch this
service-only file, so scoping to the integration test file's own
coverage run matches what CI's merged-coverage gate sees):
- applyUpdates: cyclomatic 14->3, cognitive 20->3
- resolveParentLevel: CRAP 30.0 (0% cov) -> 5.0 (high-tier cov)
- resolveUpdateLevel (new): cyclomatic 12, cognitive 8, CRAP 12.0 —
under every threshold, no new finding introduced
All 31 pre-existing case-batch-update-service tests pass unmodified;
32/32 total with the new test.
chrisdburr
added a commit
that referenced
this pull request
Aug 25, 2026
The previous refresh used locally generated coverage; CI's measured coverage differs enough at threshold edges to unbaseline findings on untouched code (updateElement CRAP 30.6 in CI vs <30 locally, PR #908). Saving from the merged-coverage artifact CI itself uploaded (run 32829923887, staging) makes baseline and head scores share one coverage source. Dupes and dead-code baselines refreshed on this machine in the same pass (both were stale, 0-match warnings in review).
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
1 participant
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.
What
Eliminates the remaining per-element (N+1) query loops in the JSON-editor batch-update path. For an N-element batch: parent validation N→1 query, circular-reference checks N walks→one shared multi-root BFS, level calculation up to 2N→2 queries, system-user check N→1, evidence link/unlink 2N→1 each.
Deliberately left per-row (with rationale in the commit): deletes (single-row delete() throwing on a missing row is what aborts the transaction) and element creates (self-referencing FK requires parent-before-child insert order).
The second half of the originating issue (case-study-service publish resolution) is obsolete — that service was removed with the case-study system retirement.
Changes
lib/services/case-batch-update-service.ts— batched validation/level/link pathslib/utils/tree-traversal.ts— newgetDescendantIdsForRoots(multi-root generalisation of the existing batched BFS)lib/services/element-service.ts—enforceAssertionStatusRules/guardAssertionStatusWriteaccept an optional pre-resolved system-user flag (backward-compatible; all other callers unchanged)src/__tests__/integration/case-batch-update-service.test.ts— 11 new tests: circular-reference validation (previously uncovered), intra-batch level chaining (incl. pinning its array-order dependency), query-count assertions (counted at the pg-driver seam so transaction-scoped queries are observed), evidence link/unlink idempotencyReview chain
applyUpdates, cyclomatic 14) with correctness pinned by tests