fix: preserve original FK value when identity lookup misses#1738
Conversation
GetTableRowsUseCase was overwriting null and orphaned foreign key values
with `{}` because the merge step's else branch assigned an empty object
whenever the identity lookup map missed. Null FKs (filtered out before
the identity query) and orphaned FKs (referenced row deleted) both
trigger this path, silently destroying the original cell value.
Fall back to the original cell value instead, so null stays null and
orphaned ids are preserved as primitives.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThese changes address foreign-key handling in the table rows API when FK metadata exists but the FK value is NULL or orphaned. The logic now preserves the original FK value instead of substituting an empty object, with a corresponding end-to-end test validating this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes FK cell-value corruption in GetTableRowsUseCase when the identity lookup doesn’t return a match (e.g., NULL FKs filtered out of the identity query, or orphaned FK values), ensuring the original FK value is preserved instead of being replaced with {}.
Changes:
- Update FK identity-merge logic to fall back to the original row value when the lookup map misses.
- Add a Postgres E2E test covering both NULL FK and orphaned FK scenarios to prevent regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| backend/src/entities/table/use-cases/get-table-rows.use.case.ts | Preserves original FK value on identity lookup miss (keeps null and orphaned IDs intact). |
| backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts | Adds regression test ensuring NULL/orphaned FK values aren’t converted to empty objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts (1)
255-332: Consider cleaning upFKBugFix_Parent/FKBugFix_Childafter the test.The test creates two custom tables and a
NOT VALIDFK constraint but never drops them. The leadingdropTableIfExistsmakes reruns idempotent, but the artifacts persist in the shared test DB after the suite completes and will appear in any later test that enumerates tables on the same connection (e.g.GET /connection/tables/:connectionId - Should list all complex test tables in connectionat line 1004 — currently safe because it only asserts inclusion, but the coupling is fragile).♻️ Proposed cleanup
// Bug: previously these were assigned `{}`. Both should preserve the original value. t.is(nullRow.parent_id, null, 'null FK must remain null, not be converted to {}'); t.is(orphanRow.parent_id, orphanedParentId, 'orphaned FK must keep its raw value, not become {}'); + + await knex.schema.dropTableIfExists(childTableName); + await knex.schema.dropTableIfExists(parentTableName); }, );For stronger guarantees against test failures leaking state, wrap the body in
try { … } finally { await knex.schema.dropTableIfExists(childTableName); await knex.schema.dropTableIfExists(parentTableName); }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts` around lines 255 - 332, Wrap the test body in a try { … } finally { … } using the existing knex and the parentTableName/childTableName variables so the cleanup always runs; in the finally block call await knex.schema.dropTableIfExists(childTableName) and await knex.schema.dropTableIfExists(parentTableName) (or additionally drop the FK constraint via knex.raw if you prefer) to remove FKBugFix_Child and FKBugFix_Parent regardless of test success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts`:
- Around line 255-332: Wrap the test body in a try { … } finally { … } using the
existing knex and the parentTableName/childTableName variables so the cleanup
always runs; in the finally block call await
knex.schema.dropTableIfExists(childTableName) and await
knex.schema.dropTableIfExists(parentTableName) (or additionally drop the FK
constraint via knex.raw if you prefer) to remove FKBugFix_Child and
FKBugFix_Parent regardless of test success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec742a4c-e6e8-4a24-b8d5-c72b0836113a
📒 Files selected for processing (2)
backend/src/entities/table/use-cases/get-table-rows.use.case.tsbackend/test/ava-tests/complex-table-tests/complex-postgres-table-e2e.test.ts
GetTableRowsUseCase was overwriting null and orphaned foreign key values with
{}because the merge step's else branch assigned an empty object whenever the identity lookup map missed. Null FKs (filtered out before the identity query) and orphaned FKs (referenced row deleted) both trigger this path, silently destroying the original cell value.Fall back to the original cell value instead, so null stays null and orphaned ids are preserved as primitives.
Summary by CodeRabbit