ReviewOS

also looking at this

stacks/bun-query-builder

fix(orm): make createModel models findable, so relations read the declared table

#1104
Merged glennmichael123 wants to merge fix/1093-relation-declared-table into main
3 files +244 -3
10 days ago

Closes #1093.

Root cause

defineModel registers into the global model registry (model.ts:193). createModel never did — there is not one registerModel call in orm.ts. Every relation lookup goes through getModelFromRegistry, so a createModel model was invisible to it and each caller fell back to its own guess.

That is also why CI never caught it: the repo's own passing Postgres integration test builds its models with defineModel.

Note the line the issue points at was never the bug — relatedModel?.getTable?.() || toTableName(name) has been correct since March. The || fallback was firing because the left side was always undefined.

Two failures, not one

1. Wrong table. The table was guessed from the model name, ignoring table. Where no table exists at the guessed name, that is the loud "no such table" crash the issue reports. Where one does — and the guess is the conventional name, so this is a plausible accident — the relation silently reads the wrong table:

with("widget") -> [{ id: 1, owner_id: 1, label: "DECOY" }]     # before
with("widget") -> [{ id: 1, owner_id: 1, label: "REAL"  }]     # after

2. A hidden column leaks through with(). Eager-load hydration fell back to the parent's definition (orm.ts ×4, || this._definition), so related rows were shaped by the wrong model:

direct queryvia with()
beforetoken redactedtoken: "SUPER-SECRET" returned
afterredactedredacted

This second one is the more serious of the pair, for a reason that is easy to miss: it does not require an unusual table name. The model in that test is called Hidsecret with table hidsecrets — exactly the conventional guess — so the table half cannot fire and the user never sees a "no such table" warning. Anyone whose naming is ordinary gets no signal that their relations are being resolved by guesswork.

The fix

Register createModel models in a module-local map that getModelFromRegistry consults after the public registry. That is the single chokepoint all the lookups already share — four table resolutions and four hydration fallbacks — so both failures close with one change.

Deliberately not in the public registry. getAllModels(), getModelRegistry() and hasModel() keep reporting exactly what defineModel put there, so nothing downstream starts seeing new entries and two models sharing a name cannot clobber each other's public entry. There is a test pinning that surface as unchanged — it passes before and after, which is the point.

Also drops the memoized relation resolutions when a model registers and when clearModelRegistry() runs. That memo is keyed per model+relation pair and was never invalidated, so a resolution computed before a model existed kept its guessed table — making the answer depend on registration order and leaking stale resolutions across test files.

Tests

test/orm.relation-declared-table.test.ts — 4 tests. Three fail without the src change; the fourth is the public-surface invariant that must pass both ways.

The decoy table matters: a test that only asserts "no crash" would have passed while the relation returned the wrong rows.

Checks

  • bun test — 1959 pass, 4 skip, 1 fail; typecheck clean
  • pickier reports one brace-style warning at orm.ts:464, which is pre-existing — it sits at :421 on main and moved only because this PR adds 43 lines above it. Untouched.

The one test failure is CLI > version prints package version, unrelated and local-only: bin/qbx is a gitignored binary compiled at build time and a stale local copy reports an old version. CI rebuilds it.

🤖 Generated with Claude Code

3 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.