Conversation
Contributor
|
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)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds Sequence Diagram(s)sequenceDiagram
participant Caller
participant HTTPRequestBuilder
participant QueryEncoding
participant URLComponents
Caller->>HTTPRequestBuilder: setQuery name and value
HTTPRequestBuilder->>HTTPRequestBuilder: replace or append query item
Caller->>HTTPRequestBuilder: build request
HTTPRequestBuilder->>QueryEncoding: render query items
QueryEncoding-->>HTTPRequestBuilder: encoded query string
HTTPRequestBuilder->>URLComponents: set percentEncodedQuery
Merge Risk: ⚪ Minimal · up to The PR tightens query encoding and adds localized query replacement behavior without introducing current production callers, new dependencies, or identified correctness, security, or availability issues. No actionable merge-blocking risk remains beyond normal checks and review. 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 |
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 22, 2026 10:22
72c03ec to
b4a9955
Compare
Contributor
|
The following capabilities are marked
The following capabilities are marked
These may have been renamed, removed, or never registered. Please update the capability matrix. |
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 22, 2026 10:38
b4a9955 to
b78e635
Compare
grdsdev
changed the title
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 24, 2026 08:32
3a15b20 to
051134d
Compare
Coverage Report for CI Build 33060279490Coverage decreased (-0.2%) to 86.941%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions17 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 24, 2026 08:38
051134d to
f8a62d6
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 25, 2026 08:19
f8a62d6 to
00fc225
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 25, 2026 08:46
00fc225 to
57f3d11
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 25, 2026 09:08
57f3d11 to
5654169
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 25, 2026 09:17
5654169 to
3c6554b
Compare
Base automatically changed from
guilhermesouza/sdk-1563-refactorpostgrest-move-todays-builders-under-legacy
to
main
August 25, 2026 09:25
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 25, 2026 09:25
3c6554b to
ee53a2f
Compare
Contributor
|
…tems
`HTTPRequestBuilder.build()` assigned `URLComponents.queryItems`, whose
encoding leaves `+` literal. A server that form-decodes the query string
reads a literal `+` as a space, so
received_at=gt.2023-03-23T15:50:30.511743+00:00
to=eq.+16505555555
arrive with a space where the UTC offset should be, and without a country
code. Neither fails loudly; both just match the wrong rows.
`QueryEncoding` escapes everything RFC 3986 reserves, keeping only `?` and
`/`, which section 3.4 permits inside a query — leaving those readable in a
log without changing how they parse. It renders the whole query in one pass
so escaping happens exactly once, at build time, on both names and values.
Items are still stored unescaped, so an existing `%` cannot be double-encoded.
This is the same character set `Helpers`' `sbURLQueryAllowed` already applies
on PostgREST's legacy path, which is what makes the port below an equality
rather than an approximation.
Nothing in production imports `HTTPRuntime` yet — only its own tests and
`HTTPRuntimeTestHelpers` — so there is no shipped behavior to preserve.
PostgREST v3 is its first consumer, and the generated clients inherit the fix
before they land.
13 tests, including 30 of the 32 requests PostgREST records under
`Tests/PostgRESTTests/__Snapshots__/BuildURLRequestTests/`, re-expressed
against `HTTPRequestBuilder`. Those are the most varied real query values in
the repository: JSON objects, range literals, `like` patterns, a timestamptz
offset, a non-ASCII string, a leading `+`. All 30 now build byte-identical
URLs to the legacy builder.
The expected strings are not copied from the snapshot files. Those are
written by swift-snapshot-testing's `.curl` strategy, which sorts the query
items by name and re-encodes them through `URLComponents.queryItems` — so
`select=%2A` is recorded as `select=*` and insertion order is lost. Each
string is the real `URLRequest.url` the legacy builder produces, captured by
running the same chains through a fetch handler that prints it.
Two cases are tested separately. The 25-operator case reads better generated
than transcribed. And `rpc call with get and params` cannot be an exact-order
assertion at all: `PostgrestClient.rpc(_:params:get:)` iterates a
`JSONValue` object, so it emits `index=2&array=…` on one run and
`array=…&index=2` on the next — two captures of the same chain disagreed.
The snapshot never caught it because `.curl` sorts before recording.
Parameter order does not change what PostgREST returns, so it is not a wire
bug, but it does make the request unreproducible in a log or a cache key.
`HTTPRuntime` is a target, not a library product, and every symbol in it is
`package`. No public API changes, so no compliance or migration entry.
The note named `PostgrestRequest` as what replaces `PostgrestRequestBuilder`. That type is not being built — spec §4.7 says to reuse `HTTPRuntime` rather than invent a parallel request model, and `HTTPRequestBuilder` already provides the ordered repeated-key query encoding it was meant to add.
`addQuery` always appends, which is right for lists (`?k=a&k=b`) and wrong for the parameters a server reads once. PostgREST v3 needs `select`, `order`, `limit`, `offset`, `on_conflict` and `columns` set rather than accumulated, or a chain that touches one twice leaves two behind and lets the server pick. `setQuery` replaces the first item using that name, in place, so the items around it keep their position. Only the first match: a repeated name is either a list or, in PostgREST's case, a conjunction on one column, and replacing every match would silently collapse `id=gt.1&id=lt.9` into one condition. A `nil` value is ignored, matching `addQuery`, so the same argument behaves the same way in both. 4 tests, including the repeated-name case and the in-place ordering.
…ttp-types rebase Rebasing onto main pulled in #1282's HTTPRequest/HTTPMethod → swift-http-types migration, which QueryEncodingTests predates: `HTTPMethod` no longer exists, `HTTPRequest.Method`/`.url` need their defining modules imported directly, and `.url` is now optional (HTTPTypesFoundation's computed property), matching the pattern already used in HTTPRequestBuilderTests.
grdsdev
force-pushed
the
guilhermesouza/sdk-1564-featpostgrest-add-the-postgrestrequest-value-model
branch
from
August 27, 2026 09:49
ee53a2f to
3b18c78
Compare
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
2 participants
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.
Closes SDK-1564, but not as originally scoped. Stacked on #1267.
Why
SDK-1564 asked for a new
PostgrestRequestvalue model. Spec §4.7 says the opposite — reuseHTTPRuntime, don't invent a transport.HTTPRuntimealready covers everything the task needed:HTTPRequest,HTTPRequestBuilder.addQuery(repeated keys),Prefer-directive merging, and request inspection viaHTTPTransportStub. SoPostgrestRequestis dropped; what's left here is the one real bug inHTTPRuntime.The fix
HTTPRequestBuilder.build()usedURLComponents.queryItems, whose encoding leaves+literal — a form-decoding server reads that as a space, silently corrupting timestamps and phone numbers.QueryEncodingnow percent-encodes everything RFC 3986 reserves except?//(permitted inside a query, and kept for log readability).No production code imports
HTTPRuntimeyet, so there's no shipped behavior to preserve — PostgREST v3 is its first consumer.Also included
setQuery: replaces a single-valued query item in place instead of appending. Needed for SDK-1568 —select,order,limit, etc. should be set once, not accumulated.PostgrestTransportover the spec's recommended reuse — same call this PR made, worth revisiting together.Tests
30 of PostgREST's 32 recorded wire-format snapshots, re-expressed against
HTTPRequestBuilderand asserted against the realURLRequest.url— not the snapshot's.curlstring, which sorts and re-encodes the query and would hide this exact bug. All 30 match the legacy builder byte-for-byte.Verification
swift test— full suite passes./scripts/format.sh,./scripts/spell-check.sh— cleanHTTPRuntimeispackage-only, so nosdk-compliance.yaml/ migration entry needed