feat(postgrest): expose query builder options to .from method - #1570
feat(postgrest): expose query builder options to .from method#1570stenlan wants to merge 4 commits into
Conversation
stenlan
changed the title
|
Hi @stenlan ! Thank you very much for your contribution. Can you please import your changes from this PR here, and make the changes needed in the new monorepo? I will be archiving the old repos soon. You will still be able to view the PR content, don't worry. I understand that this may be frustrating, but thank you very much for your time and contribution! |
14bc550 to
59a5899
Compare
|
Thank you @mandarini for the response. I have integrated the changes into the new monorepo structure to the best of my ability, but because of time constraints I have not yet been able to successfully set up the monorepo locally. For some reason, in |
|
@stenlan thank you for your contribution! Nothing special is needed to set up your local environment. Did you go through our contributing guide? Just clone the repo, run |
d790192 to
23cf9f2
Compare
mandarini
changed the title
d7f860c to
4232c41
Compare
|
@stenlan you're right, there were some issues in our lock file. Please pull, delete all your local node_modules and dist folders, and run |
4232c41 to
925a6a2
Compare
|
What is the outcome of this PR? Does it provide an interface like the one mentioned on
|
|
I will get back to this as soon as I have the time, maybe this weekend. I'm pretty busy at the moment, my apologies. |
|
@mandarini should be all good now, sorry about the slight delay 😅. This time I'm getting typescript errors on my side, but I think that's just my local env. |
mandarini
left a comment
mandarini
left a comment
There was a problem hiding this comment.
Hi @stenlan, thank you so much for sticking with this one through the monorepo migration, the lockfile mess, and the long gaps in between. That is a lot of friction for a contribution and I really appreciate you seeing it through. Also, apologies for taking so long to review.
I did a full review of the current state of the PR and there are a few things that need to change before this can merge, plus one open question from @steve-chavez that we should settle.
Things that need fixing:
-
PostgrestQueryBuilderOptionsincludesretryandurlLengthLimit, butPostgrestClient.fromnever actually reads them fromoptions, it always usesthis.retryandthis.urlLengthLimitinstead. Right now calling.from('users', { retry: false })silently does nothing. Either wire these through or drop them from the type so it doesn't promise something it doesn't do. -
SupabaseClient.frommutates the caller'soptionsobject directly (options.fetch = this._createFetchWithAuth(options.fetch)). If someone reuses the same options object across multiple.from()calls in one request, which is a very likely pattern for the SSR use case this is meant for, the fetch gets wrapped again on every call. Please build a new object instead of assigning back onto the caller'soptions. -
This is the important one:
_createFetchWithAuthusesthis._getAccessToken.bind(this), but master has since moved the default client fetch over tothis._getSessionToken.bind(this). That change exists specifically to stop new-format API keys (sb_publishable_.../sb_secret_...) from ever being sent as a Bearer token, which_getAccessToken's old fallback would do. As written, a per-request fetch override would bypass that protection. Please rebase_createFetchWithAuthagainst the currentfetchWithAuth/_getSessionTokeninlib/fetch.tsandSupabaseClient.tsrather than resolving this as a plain merge conflict, the auth logic underneath has changed since this PR was opened. -
No tests were added. At minimum we'd want coverage for: the per-request fetch actually being used instead of the client default, header merge precedence (a per-request header overriding a client-level one of the same name), and confirming auth headers are still injected when a custom fetch is passed.
-
The package.json / package-lock.json changes flagged in the earlier review are still there, please revert those.
On the API shape: @steve-chavez asked whether this delivers the chainable .withFetch() interface floated in the issue thread. After going through it, we'd like to stick with the .from(relation, options) shape from this PR rather than switch to a chainable method, since the headers/fetch merging is already built out here and reworking it into a chainable form would be a bigger change for not much benefit. So no need to change direction, just the fixes above.
No rush given everything else going on, just wanted to get you a clear list so the next pass can land. Thanks again for the persistence on this one.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds public per-request query-builder options for PostgREST table and view queries. Sequence Diagram(s)sequenceDiagram
participant Caller
participant SupabaseClient
participant PostgrestClient
participant CustomFetch
Caller->>SupabaseClient: from(relation, options)
SupabaseClient->>SupabaseClient: wrap custom fetch with authentication and trace propagation
SupabaseClient->>PostgrestClient: from(relation, resolved options)
PostgrestClient->>CustomFetch: send request with merged headers
CustomFetch-->>Caller: return response
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/postgrest-js/src/PostgrestClient.ts`:
- Around line 173-178: Update the PostgrestClient query-builder construction to
honor per-request overrides by using options.urlLengthLimit and options.retry
when provided, while retaining this.urlLengthLimit and this.retry as fallbacks.
Keep the existing handling for headers, schema, fetch, and other options
unchanged.
In `@packages/core/supabase-js/src/SupabaseClient.ts`:
- Around line 426-430: Update the options handling in the relevant
SupabaseClient method before the rest.from call: create a shallow copied options
object, then wrap the copied fetch property instead of mutating caller-owned
options. Preserve behavior when options or fetch is absent, and ensure reusing
the same options object does not nest authentication wrappers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd7bb169-f450-42f1-9a82-1e84c15168d0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
packages/core/postgrest-js/src/PostgrestClient.tspackages/core/postgrest-js/src/PostgrestQueryBuilder.tspackages/core/postgrest-js/src/index.tspackages/core/postgrest-js/src/types/types.tspackages/core/postgrest-js/src/utils.tspackages/core/supabase-js/src/SupabaseClient.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/supabase-js/src/SupabaseClient.ts (1)
690-697: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward the Functions wrapper options.
Line 697 drops
optionsbefore callingfetchWithAuth. TheomitApiKeyAsBearer: truevalue from Lines 371-373 never reachesfetchWithAuth. For a new-format API key without a session token, Functions requests then send the key inAuthorizationinstead of omitting that fallback.Proposed fix
return fetchWithAuth( this.supabaseKey, this.supabaseUrl, this._getAccessToken.bind(this), _fetch, - this.settings.tracePropagation + this.settings.tracePropagation, + options )As per coding guidelines, “Use the source files under
src/as the canonical, version-pinned reference for the full public API.” As per path instructions, “Comment only when the issue must be resolved before merge.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/supabase-js/src/SupabaseClient.ts` around lines 690 - 697, Update _createFetchWithAuth to forward its options argument to fetchWithAuth, preserving the omitApiKeyAsBearer setting for Functions requests without changing the existing fetch, token, or tracing arguments.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/supabase-js/src/SupabaseClient.ts`:
- Around line 690-697: Update _createFetchWithAuth to forward its options
argument to fetchWithAuth, preserving the omitApiKeyAsBearer setting for
Functions requests without changing the existing fetch, token, or tracing
arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cab149b-a890-4508-babe-51e1becb2945
📒 Files selected for processing (3)
packages/core/postgrest-js/src/PostgrestQueryBuilder.tspackages/core/postgrest-js/src/types/types.tspackages/core/supabase-js/src/SupabaseClient.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/postgrest-js/src/PostgrestQueryBuilder.ts
- packages/core/postgrest-js/src/types/types.ts
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/postgrest-js/test/advanced_rpc.test.ts (1)
441-548: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not accept schema-cache failures for supported RPC tests.
postgrest_resolvable_with_override_functionand the row-returning RPC functions previously tested successful resolution. These snapshots now acceptPGRST202and404instead. This makes the integration suite pass when the fixture database lacks required functions or its schema cache is stale. Restore the RPC fixtures and schema-cache setup, then retain the success and overload-resolution assertions.As per path instructions, “Comment only when the issue must be resolved before merge.”
Also applies to: 1025-1121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/postgrest-js/test/advanced_rpc.test.ts` around lines 441 - 548, Restore the database fixtures and schema-cache setup used by postgrest_resolvable_with_override_function and the row-returning RPC fixtures so these supported calls resolve successfully. Update the affected tests to assert successful responses and preserve their overload-resolution, return-type, and row-data assertions instead of accepting PGRST202/404 errors. Apply the same correction to the additional affected RPC test block.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/postgrest-js/test/basic.test.ts`:
- Around line 12-21: Restore the PostgREST test schema and its fixtures,
migrations, or test-service configuration so the expected tables, views, and
functions exist. In packages/core/postgrest-js/test/basic.test.ts at lines
12-21, retain success-path assertions instead of accepting PGRST205/PGRST202
failures; apply the same schema restoration in
packages/core/postgrest-js/test/filters.test.ts at lines 11-20 so filter
requests reach filter evaluation rather than returning missing-table responses.
In `@packages/core/supabase-js/test/unit/SupabaseClient.test.ts`:
- Around line 128-131: Update the client-level fetch callback in
SupabaseClient.test.ts so it performs a failing assertion when invoked, using a
matcher such as toBe(true) or the test framework’s fail helper; keep the
existing request-level mockFetch behavior unchanged.
---
Outside diff comments:
In `@packages/core/postgrest-js/test/advanced_rpc.test.ts`:
- Around line 441-548: Restore the database fixtures and schema-cache setup used
by postgrest_resolvable_with_override_function and the row-returning RPC
fixtures so these supported calls resolve successfully. Update the affected
tests to assert successful responses and preserve their overload-resolution,
return-type, and row-data assertions instead of accepting PGRST202/404 errors.
Apply the same correction to the additional affected RPC test block.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21dbeb4e-b57c-4bca-a250-359410969e10
📒 Files selected for processing (8)
packages/core/postgrest-js/src/PostgrestClient.tspackages/core/postgrest-js/test/advanced_rpc.test.tspackages/core/postgrest-js/test/basic.test.tspackages/core/postgrest-js/test/embeded_functions_join.test.tspackages/core/postgrest-js/test/filters.test.tspackages/core/supabase-js/src/SupabaseClient.tspackages/core/supabase-js/test/unit/SupabaseClient.test.tstsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/postgrest-js/src/PostgrestClient.ts
- packages/core/supabase-js/src/SupabaseClient.ts
|
Hi @mandarini , thanks for the in-depth review. #1 - PostgrestQueryBuilderOptions includes retry and urlLengthLimit, but PostgrestClient.from never actually reads them from options #2 - SupabaseClient.from mutates the caller's options object directly (options.fetch = this._createFetchWithAuth(options.fetch)). #3 - _getAccessToken ->_getSessionToken refactor #4 - Tests #5 - package.json / package-lock.json changes |
|
Hi @mandarini, I'd be happy to pick up the remaining work here (mainly the tests and any final fixes). Since @stenlan mentioned they're unable to get the test environment working and offered for someone else to continue, would you prefer I:
Either way I'll make sure to credit stenlan for the original implementation. Just let me know which approach works best and I'll get started. cc @stenlan hope that's okay with you! |
|
@stenlan that would be great! Please add me as a collaborator on your fork so I can push directly to this branch. My GitHub username is 7vignesh. I'll add the tests and fix anything remaining. |
c19e526 to
d2df224
Compare
|
Hi @mandarini @stenlan, I've reimplemented the feature from scratch on latest master and force-pushed to this branch. All of mandarini's review points are addressed:
The implementation is a single clean commit on master. All 28 postgrest-js type tests pass, supabase-js unit tests pass, and both packages build successfully. Thanks @stenlan for the original implementation, I just cleaned it up for the current master state. |
d2df224 to
eed189f
Compare
|
@mandarini @stenlan Quick note for the merge commit: the latest push (reimplementation on current master, tests, conflict resolution) was done by me. Would appreciate a co-author credit in the squash merge message if possible: Co-authored-by: 7vignesh vigneshgopikrishnan7@gmail.com |
eed189f to
6cee887
Compare
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
Reimplement the feature from scratch on latest master, addressing all review feedback from mandarini: 1. PostgrestQueryBuilderOptions type added to postgrest-js with headers, fetch, urlLengthLimit, and retry fields. 2. PostgrestClient.from() now accepts an optional options parameter. Per-request headers are merged (request takes precedence), and per-request fetch/retry/urlLengthLimit override client defaults. 3. SupabaseClient.from() forwards options to rest.from(), wrapping any custom fetch with fetchWithAuth so auth headers are always injected. A new options object is created to avoid mutating the caller's object. 4. mergeHeaders utility added for non-destructive header merging. 5. Tests cover: custom fetch usage, header precedence, auth injection, retry override, and non-mutation of caller options. Co-authored-by: 7vignesh <vigneshgopikrishnan7@gmail.com> and stenlan <stenlan@users.noreply.github.com> Closes supabase#438
6cee887 to
5799b07
Compare
5 participants
What kind of change does this PR introduce?
Introduces a way to pass options to the PostgrestQueryBuilder instance on a per-request basis.
What is the current behavior?
There is no way to specify per-request options currently.
#438
What is the new behavior?
An optional parameter is introduced that can be passed to the
.frommethod, where users can optionally specify customfetchandheadersoptions.Additional context
This is a supporting PR for supabase/postgrest-js#648 that updates the method signature in the @supabase/supabase-js package.