Skip to content

Commit e14d516

Browse files
kriszypclaude
andcommitted
fix(databases): preserve runtime-only relationship attrs on resetDatabases() (RE-7)
The removal loop in initStores() was dropping runtime-only attributes — like relationship attrs added via GraphQL `@relationship` — because table()'s persistence loop intentionally `continue`s past them (databases.ts:1138), so they appear in `existingAttributes` (table.attributes) but not in the `attributes` list rebuilt from attributesDbi. updatedAttributes() then stripped the resolver/search support, breaking nested GraphQL queries after any resetDatabases() / hot-reload — the symptom that showed up in CI as 8 failing `handles query by nested attribute` assertions in graphql-querying-test and the 404/501 RESTProperties failures. Also fixes a splice-while-iterating bug in the same loop: splicing the array being walked by `for...of` skipped the next element, so two adjacent removals (e.g. `breed` and `age` in the regression test) only dropped one. The loop now collects into `toRemove` and applies the splices after iteration. Adds a unit test that injects a relationship attr and asserts it survives a resetDatabases() call, and extends the removal test's comment to explain why the two-adjacent-removal pattern is intentional. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 86d89be commit e14d516

2 files changed

Lines changed: 47 additions & 6 deletions

File tree

resources/databases.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,10 @@ function initStores(
638638
logger.error(`Error trying to update attribute`, attribute, existingAttributes, indices, error);
639639
}
640640
}
641+
// Collect removals first; splicing while iterating `existingAttributes` skips adjacent
642+
// elements, which would silently leave stale fields behind when two or more were dropped
643+
// in the same reload.
644+
const toRemove = [];
641645
for (const existingAttribute of existingAttributes) {
642646
const attribute = attributes.find((attribute) => attribute.name === existingAttribute.name);
643647
if (!attribute) {
@@ -658,15 +662,22 @@ function initStores(
658662
}
659663
if (existingAttribute.indexed) {
660664
// we only remove attributes if they were indexed, in order to support dropAttribute that removes dynamic indexed attributes
661-
existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1);
662-
attributesUpdated = true;
665+
toRemove.push(existingAttribute);
663666
} else if (!existingAttribute.isPrimaryKey) {
664-
// Remove non-indexed attributes that are no longer present in the persisted schema.
665-
existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1);
666-
attributesUpdated = true;
667+
// Skip runtime-only attributes (e.g. relationship attrs — table()'s persistence loop
668+
// `continue`s past them at line 1138). They are present in `existingAttributes` but
669+
// never in the `attributes` list rebuilt from attributesDbi; removing them would drop
670+
// the resolver/search support added by updatedAttributes(). Computed attrs ARE
671+
// persisted, so only `relationship` is excluded here.
672+
if (existingAttribute.relationship) continue;
673+
toRemove.push(existingAttribute);
667674
}
668675
}
669676
}
677+
for (const existingAttribute of toRemove) {
678+
existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1);
679+
attributesUpdated = true;
680+
}
670681
if (table && !recreateForEngineChange) {
671682
if (attributesUpdated) {
672683
table.schemaVersion++;

unitTests/resources/schemaMigrationFragility.test.js

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,10 @@ describe('schema-migration fragility: non-indexed attributes missing from table.
450450
// table() updates in-memory Table.attributes directly (databases.ts:997), so after the
451451
// call above the in-memory state is already [id, name]. Re-create the stale main-thread
452452
// view — still holding the old [id, name, breed, age] — so that the removal loop in
453-
// initStores() actually needs to drop breed and age.
453+
// initStores() actually needs to drop breed and age. Two adjacent removals (breed AND
454+
// age) also guard against a regression of the splice-while-iterating bug: splicing the
455+
// array being iterated by `for...of` skipped the next element, so the loop must collect
456+
// removals first and apply them after the iteration.
454457
const tblForRemoval = getDatabases()[DB]?.[TABLE];
455458
tblForRemoval.attributes.splice(
456459
0,
@@ -467,6 +470,33 @@ describe('schema-migration fragility: non-indexed attributes missing from table.
467470
assert.ok(!attrNames.includes('breed'), `"breed" should be removed, got: ${attrNames}`);
468471
assert.ok(!attrNames.includes('age'), `"age" should be removed, got: ${attrNames}`);
469472
});
473+
474+
it('preserves runtime-only relationship attributes across resetDatabases()', () => {
475+
// Relationship attrs are runtime-only — table()'s persistence loop skips them
476+
// (databases.ts:1138, `if (attribute.relationship) continue`), so they are present in
477+
// table.attributes but never written to attributesDbi. After resetDatabases(),
478+
// initStores() rebuilds `attributes` from attributesDbi only — so the relationship attr
479+
// won't appear there. The removal loop must not drop it; otherwise updatedAttributes()
480+
// would strip the resolver/search support and downstream GraphQL nested queries return
481+
// undefined (the integration symptom: graphql-querying-test "handles query by nested
482+
// attribute" assertions).
483+
table({
484+
table: TABLE,
485+
database: DB,
486+
attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'name' }],
487+
});
488+
const tblWithRel = getDatabases()[DB]?.[TABLE];
489+
// Inject a runtime-only relationship attribute, mirroring what graphql.ts does after
490+
// parsing a `@relationship` directive — these never round-trip through attributesDbi.
491+
tblWithRel.attributes.push({ name: 'related', relationship: { from: 'relatedId' } });
492+
resetDatabases();
493+
const tbl = getDatabases()[DB]?.[TABLE];
494+
const attrNames = tbl.attributes.map((a) => a.name);
495+
assert.ok(
496+
attrNames.includes('related'),
497+
`relationship attribute "related" should survive resetDatabases() but was dropped — got: ${attrNames}`
498+
);
499+
});
470500
});
471501

472502
describe('schema-migration fragility: stale store reused after LMDB to RocksDB engine migration (F4)', () => {

0 commit comments

Comments
 (0)