ReviewOS

stacks/bun-query-builder

executeMigration discards its dir argument, and cannot run a corpus spread across directories

#1137
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Two things, one small and one that blocks a feature. The small one is a bug on its own terms; the second is the reason it came up.

1. The dir argument is declared, documented, and discarded

dist/actions/migrate.d.ts declares it and carries a comment explaining why it is retained:

// because it is public API and every caller passes it.
// eslint-disable-next-line pickier/no-unused-vars
export declare function executeMigration(dir?: string): Promise<boolean>;

The compiled body never reads it. It resolves the directory itself from the global config, passing undefined where the caller's value would go (dist/chunk-vw813qrt.js, 0.2.60):

At(void 0, _.migrationDir)

So executeMigration('/somewhere/else') silently runs config.migrationDir instead. The eslint-disable is arguably the tell: the parameter is unused because the implementation stopped honouring it, not because callers stopped passing it.

It is currently masking a caller bug. Stacks passes the models directory to it:

await qbExecuteMigration(modelsDir)   // storage/framework/core/database/src/migrations.ts:1597

That has always been wrong and has never mattered, because the value is thrown away. Anyone who fixes the signature honouring without checking callers will surface bugs like this one, so it is worth doing deliberately rather than as a drive-by.

2. The thing that actually blocks: one corpus, several directories

Stacks is adding Laravel-style package auto-discovery, where bun add loghq brings that package's models, routes and migrations. A package ships its own hand-written SQL, so the corpus for a run is the application's database/migrations plus each installed package's.

executeMigration enumerates exactly one directory:

readdirSync(dir).filter(f => f.endsWith('.sql')).sort()

Calling it once per directory does not work. Ordering is decided entirely by the sorted basename, and a package's tables carry foreign keys into the application's (user_id, team_id) and never the reverse. Sorting within each directory separately loses the cross-directory order, and REFERENCES "users" on a table created before users fails on Postgres and MySQL while SQLite tolerates it, so the failure mode is green locally and red on deploy.

The only workaround on our side is to copy every package's .sql into the application's corpus before running, under the migration lock. That means writing files the user did not author into their git-tracked database/migrations, keeping them in sync when the package upgrades, and defending them from the local generator's own pruning passes. All of it exists to work around one directory being one string.

Proposed

export declare function executeMigration(dirs?: string | string[]): Promise<boolean>
  • Honour the argument when passed; fall back to config.migrationDir when not, so every existing caller keeps its current behaviour.
  • With a list, enumerate all of them and sort globally by basename, so run order is still the ordinal and callers can interleave sources deliberately.
  • The ledger already keys on the bare basename, so a caller spreading files across directories has to keep basenames unique. Worth stating in the docs; a duplicate basename across two directories would otherwise record one and silently skip the other.

Happy to send the PR if the shape looks right. The single-argument fix alone would be an improvement, but it does not solve the multi-directory case, which is the one that matters here.

Found while building package discovery in stacksjs/stacks.

Sign in to comment on this issue.