Skip to content

Commit 2427b79

Browse files
fix(database): keep package models out of the migration generator's scope (#2428)
#2427 merged package models into the set the generator diffs, which is the wrong side of the framework. A package owns its tables through the hand-written SQL it ships, so a package model in scope means `generate:migrations` emits a second CREATE TABLE for a table that already has one. That duplicate is not inert. `preprocessSqliteMigrations` prunes duplicate create-table files by keeping the earliest filename and calling `deleteMigration()` on the rest, and once package migrations are staged into the corpus the file it deletes can be the package's own. The plan's one standing constraint on this work is that the generator must never delete a package's migration, and the merged version walked straight into it. Package models now report through `excludedTables` instead, which already means "something else owns this table" and already feeds `protectedTables`, so the generator knows the table exists and has no authority to drop it. That is the same mechanism excluded framework defaults use, for the same reason. The collision guards are unchanged and still worth having: two packages shipping one model name, or a package shipping a model the framework owns, both still stop the run. Being a global is a separate question this does not answer. A package model is still absent from `globalThis`, because that comes from the auto-import barrel in @stacksjs/server rather than from here. That wiring is its own change, and it carries its own hazards: discovery runs after the barrel is built on boot, and the barrel's staleness check is mtime-based, which a package manager preserving tarball mtimes will not trip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 93f55b3 commit 2427b79

2 files changed

Lines changed: 26 additions & 15 deletions

File tree

storage/framework/core/database/src/model-sources.ts

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ export function resolveModelSources(options: {
317317

318318
assertPackageModelsAreUsable(packaged, framework)
319319

320-
if (user.length === 0 && framework.length === 0 && packaged.length === 0)
320+
if (user.length === 0 && framework.length === 0)
321321
return null
322322

323323
const merge = options.includeFrameworkDefaults ?? shouldIncludeFrameworkDefaults()
@@ -345,26 +345,32 @@ export function resolveModelSources(options: {
345345
shadowed.push({ name: model.name, userFile: model.file, frameworkFile: replaced.file })
346346
}
347347

348-
// Precedence, lowest to highest: framework defaults, then packages, then
349-
// userland. A package's models are MERGED rather than treated as a fallback,
350-
// because an application that installed the package asked for its schema.
351-
// Userland still wins over both, which is how an app customises either.
348+
// Packages are deliberately NOT here. A package owns its tables through the
349+
// hand-written SQL it ships, so putting its models in the generator's scope
350+
// would emit a second CREATE TABLE for each one. Those duplicates then meet
351+
// the earliest-filename-wins pruning in `preprocessSqliteMigrations`, which
352+
// deletes the loser - and the loser can be the package's own file, which is
353+
// the one thing a package's migrations must never suffer.
354+
//
355+
// They are reported through `excludedTables` instead, so the generator knows
356+
// those tables exist and has no authority to drop them. Being a global is a
357+
// separate question, answered by the auto-import barrel rather than here.
352358
const byName = new Map<string, ModelSource>()
353359
for (const model of contributing) byName.set(model.name, model)
354-
for (const model of packaged) byName.set(model.name, model)
355360
for (const model of user) byName.set(model.name, model)
356361

357362
const models = [...byName.values()].sort((a, b) => a.name.localeCompare(b.name))
358363

359364
const roots: string[] = []
360365
if (user.length > 0)
361366
roots.push(userRoot)
362-
for (const root of new Set(packaged.map(model => model.file.replace(/\/[^/]+$/, ''))))
363-
roots.push(root)
364367
if (contributing.length > 0)
365368
roots.push(frameworkRoot)
366369

367-
const excludedTables = [...new Set(excluded.map(declaredTableName))].sort()
370+
// A package's tables are out of the generator's scope for the same reason an
371+
// excluded framework default's are: something else owns them, and "not mine
372+
// to generate" is not "safe to drop".
373+
const excludedTables = [...new Set([...excluded, ...packaged].map(declaredTableName))].sort()
368374

369375
// Fast path: a single root whose models are all top level needs no staging,
370376
// so the common userland-only project keeps reading its own directory.

storage/framework/core/database/tests/package-models.test.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ describe('package model roots', () => {
9898
})
9999

100100
describe('resolving models with packages installed', () => {
101-
test('a package contributes its models alongside the application own', () => {
101+
test('a package model stays OUT of the generator scope, and its table is protected', () => {
102102
const root = project()
103103
try {
104104
const userRoot = models(join(root, 'app/Models'), ['Post'])
@@ -111,14 +111,18 @@ describe('resolving models with packages installed', () => {
111111
packageRoots: [{ package: 'loghq', dir: pkg }],
112112
})
113113

114-
// Userland has models, so the framework defaults stay out (#2220). The
115-
// package's models come in regardless, because the app asked for them.
116-
expect(resolved?.models.map(m => m.name).sort()).toEqual(['LogEntry', 'Post'])
114+
// The package owns log_entries through the SQL it ships, so generating a
115+
// second CREATE TABLE for it would produce a duplicate that the pruning
116+
// resolves by deleting a file, possibly the package's own.
117+
expect(resolved?.models.map(m => m.name).sort()).toEqual(['Post'])
118+
119+
// But the generator must know the table exists, or it proposes a DROP.
120+
expect(resolved?.excludedTables).toContain('log_entries')
117121
}
118122
finally { rmSync(root, { recursive: true, force: true }) }
119123
})
120124

121-
test('userland still overrides a model a package shipped', () => {
125+
test('a userland model of the same name is the one that generates', () => {
122126
const root = project()
123127
try {
124128
const userRoot = models(join(root, 'app/Models'), ['LogEntry'])
@@ -154,7 +158,8 @@ describe('resolving models with packages installed', () => {
154158
// A package bringing models is not the app having models of its own, so
155159
// the defaults still stand in. Removing them here would take User away
156160
// from a fresh app the moment it installed anything.
157-
expect(resolved?.models.map(m => m.name).sort()).toEqual(['LogEntry', 'User'])
161+
expect(resolved?.models.map(m => m.name).sort()).toEqual(['User'])
162+
expect(resolved?.excludedTables).toContain('log_entries')
158163
}
159164
finally { rmSync(root, { recursive: true, force: true }) }
160165
})

0 commit comments

Comments
 (0)