Skip to content

Commit 3f42920

Browse files
claude[bot]claude
andauthored
fix(driver-sql): keep the logger receiver at the nine detach-then-call sites — a class-based host logger no longer turns a durability warning into a TypeError (#12821)
* fix(driver-sql): keep the logger receiver at the nine detach-then-call sites `(this.logger.error ?? this.logger.warn)(…)` evaluates to the bare function and then calls it, so the call runs with `this === undefined`. The eight `error ?? warn` sites now call `logDurabilityFailure()` — the property-access helper this class already had three lines from the docblock that explained it. The ninth is `info ?? warn` and reports a reconcile that SUCCEEDED, so it keeps its level with an in-place property-access spelling rather than being escalated to the durability channel. The `:4290` docblock is rewritten: its closing paragraph explained why the inline sites were left alone, next to code that no longer uses that shape. Part of #12792 * test(driver-sql): pin the logger receiver with class-based doubles and a shape scan Three sections. ⓪ asserts the doubles — and `@objectstack/core`'s real `ObjectLogger` — are receiver-sensitive, and that a closure double is not, so the file cannot go quietly decorative. ①/② drive the real reconcile and the real declared-index sync (real sqlite, real duplicate rows) against a class-based logger and against a warn-only one. ③ is a structural pin over `sql-driver.ts`: an AST walk for all four detach shapes, with a control sample proving it fires — including on a fallback split across lines, which the two single-line counts on this card could not see. Part of #12792 * predict: reverting ONE site to the detached spelling must turn the pins red Prediction, committed before the mutation so it cannot be written after the fact. Reverting the durability site in `syncDeclaredIndexes` to `(this.logger.error ?? this.logger.warn)(…)` must produce, against the class-based double: ① §2 red — the detached call throws `TypeError: Cannot read properties of undefined (reading 'record')`, escaping `syncDeclaredIndexes`, so the `resolves.toBeUndefined()` assertion rejects instead. The mirror of the production shape, where `ObjectLogger.error` reaches `this.writeErrorLike`. ② §2's warn-only case red for the same reason (the FALLBACK leg detaches too). ③ §2's real-ObjectLogger case red — the platform logger, same path. ④ §3 red — the structural scan reports one `parenthesized-callee` finding. ⑤ §0 and §1 stay GREEN: they do not touch this site, so a red there would mean the mutation was not the thing measured. Direction: RED. Not "fewer diagnostics" and not a reversal — the assertions are behavioural and the scan is a direct count of the shape. Part of #12792 * predict: reverting the INFO site must turn §1 red — and in a different shape The second ablation, because §1's assertions are about a different site on a different channel and the first one does not cover them. Reverting the `info` site in `reconcileAndWarnDrift` to `(this.logger.info ?? this.logger.warn)(…)` must produce: ① §1 red — but NOT as an escaping TypeError. That site sits inside the reconcile's own `try`, so the throw is CAUGHT and re-reported as `[schema-drift] dev auto-reconcile failed … — falling back to warning`. So the failure is `infos` empty AND a bogus failure line present: a reconcile that really happened, announced as a failure. ② §1's warn-only case red the same way (the fallback leg detaches too). ③ §3 red — one more `parenthesized-callee` finding. ④ §0 and §2 stay GREEN. Direction: RED, via SWALLOW-AND-MISLABEL rather than a propagating throw. That asymmetry is the point of running this one separately. Part of #12792 * test(driver-sql): assert the reconcile's whole log transcript, not just the line wanted The info site's defect has two halves and the second one is a line that must NOT be present: the site sits inside the reconcile's own `try`, so a detached call is caught and re-reported as `dev auto-reconcile failed`. Asserting the full transcript puts both halves in one diff instead of hiding the mislabel behind an earlier expectation that fails first. Part of #12792 * chore: changeset for the driver-sql logger receiver fix Part of #12792 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c4ecf0c commit 3f42920

3 files changed

Lines changed: 627 additions & 13 deletions

File tree

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): keep the logger's receiver at the nine detach-then-call sites — a class-based host logger no longer turns a durability warning into a `TypeError` (#12792)
6+
7+
Nine sites in `sql-driver.ts` picked a log channel by **extracting** the method before
8+
calling it — eight on the durability channel and one on `info`:
9+
10+
```ts
11+
(this.logger.error ?? this.logger.warn)(msg, meta); // 8 sites
12+
(this.logger.info ?? this.logger.warn)(msg); // 1 site
13+
```
14+
15+
`a.b` in *call position* passes `a` as the receiver; `(a.b ?? c.d)(…)` evaluates to the
16+
bare function first, so the call runs with `this === undefined`. A plain-closure logger
17+
does not read `this` and survives it — which is why no suite ever went red, since this
18+
class's own default sink and every test double in the package are closures.
19+
`@objectstack/core`'s `ObjectLogger` is a real class with prototype methods and no
20+
constructor binding — `error`/`fatal` reach for `this.writeErrorLike`,
21+
`debug`/`info`/`warn` for `this.write` — so a host that injects one got:
22+
23+
```
24+
TypeError: Cannot read properties of undefined (reading 'writeErrorLike')
25+
at error (packages/core/src/logger.ts:414:14)
26+
at SqlDriver.syncDeclaredIndexes (packages/drivers/driver-sql/src/sql-driver.ts)
27+
```
28+
29+
The asymmetry that makes it worth fixing rather than noting: these particular lines
30+
report **durability degradation and schema drift** — the channel that exists to be loud
31+
when a constraint the metadata claims is enforced is not. A throw there converts the one
32+
signal into silence plus an unrelated crash, and where the site sits inside the
33+
reconcile's own `try` the throw is swallowed and re-reported as
34+
`dev auto-reconcile failed` — a reconcile that really happened, announced as a failure,
35+
with the post-reconcile re-detect skipped so the next warning describes a state that is
36+
no longer true.
37+
38+
The eight `error ?? warn` sites now call `logDurabilityFailure()` — the property-access
39+
helper this class already had, three lines from the docblock that explains it. The ninth
40+
is `info ?? warn` and reports a reconcile that **succeeded**, so it keeps its level with
41+
an in-place property-access spelling rather than being escalated onto the durability
42+
channel; escalating a functional report to `error` is the over-application AGENTS.md
43+
names as what makes `error` unreadable in the first place.
44+
45+
**Why `patch`.** No export, signature, accepted input or rejected input changes, and no
46+
message text changes. A host whose logger is a plain closure object sees byte-identical
47+
behaviour — that shape worked before and is pinned unchanged. The one behaviour a
48+
consumer could observe is a subclass that overrides the `protected`
49+
`logDurabilityFailure`: eight more calls now route through its override. That method is
50+
already this class's declared verb for the durability channel and the fallback semantics
51+
at those sites are unchanged (`error` when the sink has one, else `warn`), so more calls
52+
honouring the override is the documented intent rather than a break.
53+
54+
Also measured and recorded rather than assumed: **nothing composes an `ObjectLogger`
55+
into `SqlDriver` today**. The plugin's `onEnable` builds `new SqlDriver(config)` and
56+
never passes the kernel's logger, the constructor reads no `logger` key, and every
57+
`driver.logger = …` assignment in the repo is a test or a testkit; the one production
58+
seam that *can* install one is `SqliteWasmDriver`'s constructor, inherited straight into
59+
this class, and no caller passes it yet. So these were latent, not live — which decides
60+
urgency, not whether: a call that runs with `this === undefined` is a defect whatever
61+
today's wiring happens to tolerate.
62+
63+
The regression pin (`logger-receiver-detach.test.ts`) uses **class-based** logger doubles
64+
whose channels dispatch through `this`, drives the real reconcile and the real
65+
declared-index sync against real SQLite, and adds a structural AST scan over
66+
`sql-driver.ts` for all four detach spellings — including the two a single-line regex
67+
cannot see, which is how this file's count was twice taken as a floor.

0 commit comments

Comments
 (0)