fix(postgrest): add typed column inference for order() with referencedTable - #2445
fix(postgrest): add typed column inference for order() with referencedTable#24457vignesh wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates Postgrest-js TypeScript typings so .order() can infer valid column names when ordering via a referencedTable (and the deprecated foreignTable) based on the schema.
Changes:
- Add
TablesAndViews-based overloads for.order()to type column names forreferencedTable. - Add matching typed overloads for deprecated
foreignTable. - Extend d.ts tests to cover typed ordering on referenced tables and expected type errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/postgrest-js/src/PostgrestTransformBuilder.ts | Adds schema-aware .order() overloads for referencedTable/foreignTable to type referenced-table columns. |
| packages/core/postgrest-js/test/index.test-d.ts | Adds type-level tests verifying typed referenced-table ordering and invalid-column failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| order<ReferencedTable extends string>( | ||
| column: string, | ||
| options?: { ascending?: boolean; nullsFirst?: boolean; referencedTable?: string } | ||
| options?: { | ||
| ascending?: boolean | ||
| nullsFirst?: boolean | ||
| referencedTable?: ReferencedTable extends keyof TablesAndViews<Schema> | ||
| ? never | ||
| : ReferencedTable | ||
| } |
| order<ReferencedTable extends string>( | ||
| column: string, | ||
| options?: { ascending?: boolean; nullsFirst?: boolean; foreignTable?: string } | ||
| options?: { | ||
| ascending?: boolean | ||
| nullsFirst?: boolean | ||
| foreignTable?: ReferencedTable extends keyof TablesAndViews<Schema> ? never : ReferencedTable | ||
| } |
| order< | ||
| ReferencedTable extends string & keyof TablesAndViews<Schema>, | ||
| ColumnName extends string & keyof TablesAndViews<Schema>[ReferencedTable]['Row'], | ||
| >( | ||
| column: ColumnName, | ||
| options: { ascending?: boolean; nullsFirst?: boolean; referencedTable: ReferencedTable } | ||
| ): this |
| order< | ||
| ReferencedTable extends string & keyof TablesAndViews<Schema>, | ||
| ColumnName extends string & keyof TablesAndViews<Schema>[ReferencedTable]['Row'], | ||
| >( | ||
| column: ColumnName, | ||
| options: { ascending?: boolean; nullsFirst?: boolean; foreignTable: ReferencedTable } | ||
| ): this |
|
Hi @7vignesh, thank you so much for contributing to Supabase! This is a nice fix for a real gap. Passing While testing it locally I ran into one case that regresses: if the column name comes from a variable instead of a string literal, and const sortColumn: string = getSortColumn()
postgrest
.from('users')
.select('messages(*)')
.order(sortColumn, { referencedTable: 'messages', ascending: false })This compiles fine on master today but fails on this branch with "No overload matches this call." Since the column is a plain Would you be able to take a look at that case? Happy to help think through the overload signatures if that's useful. Thank you again for contributing, it's contributions like yours that help make our tools better for everyone. |
mandarini
left a comment
mandarini
left a comment
There was a problem hiding this comment.
Thanks, see my comment above!
…dTable When referencedTable or foreignTable is a known table/view, the column param is now constrained to that table's Row. Provides autocomplete and compile-time error checking. Closes supabase#971
Use conditional type in order() overloads so that: - Valid column literals get autocomplete and type checking - Wide string types (from variables) are accepted - Invalid column literals still produce compile-time errors This fixes the regression where a non-literal column (e.g. from a prop or user selection) with a known referencedTable would fail with 'No overload matches this call.' Addresses reviewer feedback on supabase#2445.
65f8ce8 to
d8dccf0
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Assessment against linked issues
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 |
|
Hi @mandarini, thanks for the detailed feedback! I've addressed the regression case you identified. The fix uses a conditional type in the
// This now compiles correctly:
const sortColumn: string = getSortColumn()
postgrest
.from('users')
.select('messages(*)')
.order(sortColumn, { referencedTable: 'messages', ascending: false })I've also added test cases for both Branch has been rebased on latest master as well. |
mandarini
left a comment
mandarini
left a comment
There was a problem hiding this comment.
Thanks for turning around the dynamic-column fix so quickly!! So, while digging a bit further I found one more edge case worth addressing before we merge. I checked how PostgREST resolves the <relation>.order= param, and when an embed is aliased (e.g. .select('messages:channels(*)')), PostgREST matches on the alias, not the real table name. Right now TablesAndViews<Schema> only knows real table/view names, so if an alias happens to coincide with an unrelated real table in the schema, the typed overload will validate the column against that unrelated table's Row instead of the table that's actually embedded. That could either reject a valid column or silently accept one that's wrong at runtime.
Could you add a test that exercises an aliased referencedTable (something like .select('archived:messages(*)').order('col', { referencedTable: 'archived' })) so we can see how it behaves today, and add a short note in the JSDoc that aliased referenced tables fall back to unchecked string (or aren't safe if the alias collides with a real table name)? Given that fully resolving aliases would need more plumbing than this PR should take on, documenting the limitation seems like the right scope for now.
Separately, since the referencedTable and foreignTable overloads duplicate the same three-branch conditional type, would it be worth factoring that into a shared helper type? It would cut the overload count roughly in half and make it much easier to maintain going forward.
Thanks again for sticking with this one through a couple of rounds, it's making the fix noticeably more solid.
…cs and tests - Factor the conditional column type into a shared OrderColumnForTable helper, reducing duplication across referencedTable and foreignTable overloads. - Add JSDoc note explaining that aliased referencedTable names that do not match a real table/view fall through to unchecked string, and that collisions with unrelated real tables should use a string variable to bypass checking. - Add tests for aliased referencedTable (unknown alias falls through to string overload). Addresses reviewer feedback on supabase#2445.
|
Hi @mandarini, all three points addressed in the latest push:
All 28 tstyche type test files pass and the build is green. |
Description
What changed?
Added new typed overloads to the
order()method inPostgrestTransformBuilderthat resolve the referenced table's column names from theSchematype whenreferencedTable(or deprecatedforeignTable) is a known table/view.Overload resolution now works in 3 tiers:
referencedTable→ autocomplete shows columns from the parent table (unchanged)referencedTableis a known table/view → autocomplete shows columns from that table'sRow, invalid columns produce a compile-time errorreferencedTableis an unknown string → falls through tocolumn: string(unchanged, for dynamic/alias use cases)The catch-all overload uses a conditional
nevertype to prevent it from matching when the referenced table is known, forcing TypeScript to use the typed overload instead.Why was this change needed?
When users passed
referencedTable: 'messages'to.order(), TypeScript offered no autocomplete for the referenced table's columns and silently accepted invalid column names. This caused runtime errors that could have been caught at compile time.This is the TypeScript typing portion of the issue — runtime behavior was already correct (clarified by maintainers in the issue thread).
Closes #971
📸 Screenshots/Examples
After: type error on invalid column for referenced table
After: valid column passes without error
Breaking changes
The catch-all
stringoverload still exists for dynamic table names or unknown aliases, so existing code that passes arbitrary strings continues to compile.Checklist
fix(postgrest): add typed column inference for order() with referencedTablepnpm nx formatto ensure consistent code formattingtest/index.test-d.ts)Additional notes
TablesAndViewsfrom the existing select-query-parser types no new type utilities were introduced.order()is untouched.postgrest-jsand downstreamsupabase-js.