ReviewOS

also looking at this

stacks/bun-query-builder

fix(orm): give a declared column type the storage type it asks for

#1106
Merged glennmichael123 wants to merge fix/1094-integer-column-type into main
5 files +322 -33
10 days ago

Closes #1094.

createTableFromModel understood exactly two attribute types — number and boolean — and sent everything else to its TEXT default. So type: 'integer', the spelling used in this library's own docs and model examples, produced a TEXT column.

Why it is worse than a wrong column type

Nothing errors. Integers store and read back as strings, and under SQLite's text collation '10' < '9':

WHERE n > 9   ->  []          # rows n=9 and n=10 exist; "ten" is correct
typeof(n)     ->  'text'

Range filters and ORDER BY on an integer attribute return the wrong rows, quietly. Measured through the ORM, before → after:

declaredbeforeafter
integerTEXTINTEGER
bigintTEXTINTEGER
floatTEXTREAL
decimalTEXTREAL
number, booleanINTEGERINTEGER
string, text, date, datetime, jsonTEXTTEXT
ticker_id declared stringINTEGERTEXT

The actual defect is a disagreement, not an entry

The migration path (SQLiteDriver.getColumnType) already handled every one of these correctly. So a model migrated into one column type and was created as another — two mappings for one question.

Fixing only the ORM's table would leave that shape in place, so both paths now read one mapping in column-types.ts, and the driver delegates to it, keeping only the enum CHECK — which needs a column name and a quoter — of its own. There is a test asserting the two agree for every canonical type; that is the one that keeps this fixed.

The *_id heuristic

Also made conditional on the declared type being numeric, which is what the migration path already did (drivers/sqlite.ts, pinned by test/sqlite-string-id-columns.test.ts). A declared string id — tickers, wallet addresses, hashes — was being coerced to INTEGER, so '007' stored as 7.

An attribute that declares no type at all keeps the old INTEGER default, so nothing relying on the bare heuristic changes shape.

A test was already papering over this

test/orm.or-where-precedence.test.ts:70 carried a Number() coercion with a comment naming this exact bug:

Number(): type: 'integer' currently generates a TEXT column in createTableFromModel, so this reads back as '1'.

It asserts directly now.

Checks

  • typecheck clean; pickier reports one pre-existing brace-style warning at orm.ts:422 (:421 on main — moved by one import line)
  • bun test — 1962 pass, 4 skip, 2 fail

Both failures reproduce on clean main with none of these changes present; I checked before attributing them:

  • CLI > version prints package version — stale local bin/qbx, a gitignored binary rebuilt by CI
  • migration locking (#1067) > serialises concurrent runs — passes in isolation, fails only in the full suite, timing-sensitive. Pre-existing and worth its own look; CI has been green on it.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.