Repository navigation
fix(postgres): support cross-schema foreign keys in related records and navigation (#1473) - #1478
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| title: currentTab.database | ||
| ? `${fk.ref_table} (${currentTab.database}${targetSchema ? `.${targetSchema}` : ''})` | ||
| : fk.ref_table, | ||
| : (targetSchema ? `${fk.ref_table} (${targetSchema})` : fk.ref_table), |
There was a problem hiding this comment.
WARNING: Tab title changes for all schema-aware drivers, not just cross-schema FKs
When currentTab.database is unset, targetSchema is now always truthy for Postgres (it falls back to currentTab.schema), so every FK navigation produces a title like organizations (public) instead of organizations. This is a UI behaviour change beyond the cross-schema fix, and may break title-based tab dedup/lookup or existing tests. Consider adding the schema suffix only when fk.ref_schema differs from currentTab.schema. Also the added parentheses around the ternary are redundant.
| }); | ||
| const columns = renderIdentifiers(ordered.map((c) => c.name)); | ||
| const refColumns = renderIdentifiers(ordered.map((c) => c.ref_column)); | ||
| const refTableRef = group.ref_schema |
There was a problem hiding this comment.
SUGGESTION: Generated DDL now schema-qualifies same-schema references
The Postgres driver always returns ref_schema (even for same-schema FKs), so generateCreateTableSQL will now emit REFERENCES "public"."tbl" for every FK while the created table itself (CREATE TABLE "tbl") stays unqualified. This is valid SQL but changes existing generated output and is inconsistent. Consider qualifying only when ref_schema differs from the table's own schema, or document the intended behaviour.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of
The Files Reviewed (4 files)
Previous Review Summary (commit 3ed8d05)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3ed8d05)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (20 files)
Reviewed by claude-sonnet-5.5 · Input: 252 · Output: 1.6K · Cached: 210.6K Review guidance: REVIEW.md from base branch |
…nd navigation (TabularisDB#1473) Signed-off-by: markankamaransheogarat-sys <279850453+markankamaransheogarat-sys@users.noreply.github.com>
3ed8d05 to
b25cbf6
Compare
1-DARK
left a comment
1-DARK
left a comment
There was a problem hiding this comment.
Thanks for the fix, @markankamaransheogarat-sys ! The core change looks good: ref_schema is mapped from the existing foreign_schema_name column, and the UI falls back cleanly with fk.ref_schema ?? schema.
I checked locally that your new useReferencedRecord test fails on main (it queries "billing"."organizations") and passes on this branch. Typecheck, lint, cargo check --no-default-features, and the full Vitest suite also pass.
One change needed before merge:
In packages/web-ui/src/pages/Editor.tsx (L3272–3274), the tab title for foreign-key navigation dropped the schema:
${fk.ref_table} (${db}.${schema}) became ${fk.ref_table} (${db}).
With cross-schema foreign keys, core.organizations and billing.organizations now open as two tabs that are both titled organizations (mydb).
Could you restore the previous expression? It already uses your new targetSchema:
? `${fk.ref_table} (${currentTab.database}${targetSchema ? `.${targetSchema}` : ''})`Optional / nice to have:
- The description mentions
sqlGeneratorchanges and tests, but they aren't in the diff. Could you update the description or add the change?generateForeignKeyConstraintsstill emits an unqualifiedREFERENCES. That's existing behaviour, so a follow-up is fine. - A small test for the schema and title produced by
handleForeignKeyNavigatewould help prevent this regression. - FYI: the reporter uses the external PostgreSQL plugin, which will also need to send
ref_schema.
|
Thanks a lot @markankamaransheogarat-sys for the time you put into this. #1474 fixes the same issue with the same backend change and was opened a few hours earlier, so I've merged that one and I'm closing this as a duplicate. Your catch on the New Row modal FK dropdown was a good one, and it's being picked up as a follow-up to #1474. Thanks again, and see you on the next one! |
Fixes #1473
Problem
When a table in one PostgreSQL schema (e.g.
billing.invoices) has a foreign key referencing a table in another schema (e.g.core.organizations), the Related Records panel and Open referenced table in new tab look forbilling.organizationsinstead ofcore.organizations, failing with:ERROR: relation "billing.organizations" does not exist.Root Cause
models::ForeignKeyinsrc-taurilacked aref_schemafield, droppingref_nsp.nspname::text AS foreign_schema_namewhich was already returned by the PostgreSQL driver query.useReferencedRecord,Editor.tsx,NewRowModal,DataGrid,sqlGenerator) relied solely on the current tab's active schema when resolving referenced tables instead of checking iffk.ref_schemawas provided.Solution
pub ref_schema: Option<String>toForeignKeyinmodels.rs(with#[serde(skip_serializing_if = "Option::is_none")]).foreign_schema_nametoref_schemainget_foreign_keysandget_all_foreign_keys_batchin the PostgreSQL driver.foreign_keys.rsandparity_foreign_keys.rs.ref_schema?: string | nulltoForeignKeytype.useReferencedRecord.ts, resolvedtargetSchema = fk.ref_schema ?? schemawhen quoting the table and in theexecute_queryRPC payload.Editor.tsx(handleForeignKeyNavigate), settargetSchema = activeCapabilities?.schemas ? (fk.ref_schema ?? currentTab.schema) : undefined.ref_schema.useReferencedRecordandsqlGenerator.Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.