fix(postgrest): make query builders immutable (#1208)#1523
Open
CAN-Aidoo wants to merge 1 commit into
Open
Conversation
Every chain method on the postgrest builders (BaseFilterRequestBuilder, BaseSelectRequestBuilder, BaseRPCRequestBuilder, and the four Async*RequestBuilder classes) mutated self.request.params/headers and returned self. Sharing a base query across multiple executions leaked conditions between calls — a builder reused for pagination or multi-branch filtering accumulated params instead of producing independent requests. Introduce a module-level _clone_request() helper and a _clone() method on each builder. Every mutating method now returns a fresh instance with an independent RequestConfig. Methods that return a different builder type (single, maybe_single, csv, text_search, explain) clone the request before handing it off. Also fixes a latent bug in BaseFilterRequestBuilder.match() where the loop used self.eq instead of chaining through the accumulator. Add test_builder_immutability and test_pagination_immutability reproducing the exact scenarios from the issue, plus a parameterized test_select_builder_base_is_untouched covering 15 chain methods. Rename test_max_affected_returns_self to reflect the new contract. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix #1208 by making every postgrest query builder chain method return a new instance instead of mutating shared
RequestConfigstate.Before (bug):
Same class of bug affects pagination via reused
.range()/.limit()/.offset()— the request URL ends up with?offset=0&offset=250&…&limit=251&limit=251…(see the DDoerner comment on the issue).Approach
_clone_request(request)helper that returns a freshRequestConfigwith copies of the mutable pieces (Headers,QueryParams)._clone(self)onBaseFilterRequestBuilder(inherited byBaseSelectRequestBuilder→BaseRPCRequestBuilder) and on the four standaloneAsync*RequestBuilderclasses.new = self._clone(); …mutate new…; return new.AsyncSelectRequestBuilder.{single,maybe_single,csv,text_search,explain}), clone the request first and hand it to the new builder — never mutateself.request.Also fixes a latent bug in
BaseFilterRequestBuilder.match()where the loop usedself.eq(...)instead of chaining through the accumulator.Coverage vs. prior PRs
#1385 (open): introduced the
_clone()idea but only fornot_,filter,or_,match— leaves ~20 mutating methods still broken (order/limit/offset/range/max_affected/select/single/csv/explain/text_search/retry, plus every RPC builder method). This PR extends the pattern to all of them.#1521 (closed): used a class decorator (
_make_immutable) that monkey-patched every public method. This PR uses explicit per-method_clone()calls instead — reviewable, doesn't break IDE navigation, preservesSelf-typing for mypy.Breaking change callout
@silentworks noted on 2026-03-30 that this fix is scheduled for v3 because it's a breaking change. Users relying on
builder is builder.eq(...)identity, or on mutating a builder in place and re-executing, will see behavior change. Suggest labeling for v3 (or gating behind an opt-in flag if you'd prefer to ship it in v2).I asked in this comment on #1208 whether you'd prefer this against
mainor av3branch — happy to retarget if needed.Test plan
test_builder_immutability— reproduces the exacteq → in_scenario from the issue.test_pagination_immutability— reproduces the DDoerner.range()pagination scenario.test_select_builder_base_is_untouched— parameterized over 15 chain methods (limit,offset,order,range,select,eq,in_,filter,or_,max_affected,match,not_,single,maybe_single,csv) asserting the base builder's params and headers are unchanged after each call.test_max_affected_returns_new_instance— renamed fromtest_max_affected_returns_self, updated assertion to reflect the new immutable contract.uv run --package postgrest pytest src/postgrest/tests/_async src/postgrest/tests/_sync --ignore=…integration.py --ignore=…test_client.py).make postgrest.mypyclean.uv run ruff check src/postgrestclean.Closes #1208