feat(postgrest): add the public transport seam over HTTPTypes - #1271
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 24, 2026 08:32
fc714de to
6468104
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 24, 2026 08:38
6468104 to
a281c58
Compare
Coverage Report for CI Build 32831869802Warning No base build found for commit Coverage: 86.13%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 25, 2026 08:19
a281c58 to
3fe60d9
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 25, 2026 08:46
3fe60d9 to
0342e8f
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 25, 2026 09:08
0342e8f to
fb9f24c
Compare
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 25, 2026 09:17
fb9f24c to
654a25e
Compare
…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.
## Tests
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.
`PostgrestTransport` is how a caller takes over sending: a timeout, an
injected header, logging, canned responses in a test — what `fetch:` provides
today.
```swift
public protocol PostgrestTransport: Sendable {
func send(_ request: HTTPTypes.HTTPRequest, body: Data?) async throws
-> (Data, HTTPTypes.HTTPResponse)
}
```
Nothing `package`-scoped appears in that signature, which is the whole reason
it can be public. Spec §4.7 records why the alternatives lose: promoting
`HTTPRuntime` contradicts an explicit in-source comment and would put
`HTTPError` under ADR 0001, and a URLSession-only seam drops interception that
standalone `PostgrestClient` users have today.
## The pipeline still speaks HTTPRuntime
Per §4.7, `HTTPRuntime` is reused rather than replaced. `PostgrestTransport`
is a façade over it, not a parallel implementation:
- no custom transport supplied → `HTTPRuntime.URLSessionTransport` is used
directly, and nothing converts
- a custom transport supplied → `PostgrestTransportBridge` adapts it to
`HTTPRuntime.HTTPTransport`
So the conversion is a cost only callers who take over the transport pay.
`URLSessionPostgrestTransport` is the shipped conformance, implemented over
`HTTPRuntime.URLSessionTransport` rather than duplicating the URLSession
handling. It exists mainly to be delegated to: a transport that only wants to
observe or adjust a request should hold one and forward, which keeps the URL
assembly and its encoding rules in one place.
## Three conversion traps, each with a test
**The path must not be decoded.** `URL.path` and `URLComponents.path` both
percent-decode, so reading either would hand the transport `select=*` where
the wire carries `select=%2A`. The bridge reads `percentEncodedPath` and
`percentEncodedQuery`, and rebuilds a URL by concatenation rather than through
`URLComponents`, which would re-encode.
**A file-backed body is refused, not buffered.** Reading the file in would
defeat the reason `HTTPBody.file` exists. PostgREST never produces one, so
this throws `unsupportedRequestBody` rather than quietly working.
**Streaming fails loudly.** A `PostgrestTransport` returns a buffered
`(Data, HTTPResponse)`. Buffering the whole body and handing back a one-chunk
stream would look like streaming while defeating the point of it.
Header names keep the casing they were written with — `rawName`, not
`canonicalName`, which lowercases for HTTP/2 — and repeated names merge
case-insensitively, joined with `", "` per RFC 9110 section 5.3.
## A curl snapshot cannot check this
`sendsTheQueryStringExactlyAsGiven` asserts the recorded `URLRequest.url`
directly. Written first with `Mock.snapshotRequest`, it recorded
`?select=*&to=eq.+16505555555` while the wire carried the escaped spelling:
that renderer sorts the query items and re-encodes them through
`URLComponents`, normalizing `%2A` to `*` and `%2B` to `+` — the exact
regression the test exists to catch. Worth knowing before SDK-1566 and
SDK-1568 lean on the same helpers.
## Not in this commit
The plan's Task 3 also deletes the hand-rolled retry loop and re-checks
`db.retry`. Both need an execution path on the new core, and the typed API
still executes through `Legacy/`. They belong with SDK-1568, which rebuilds
the wrappers, and SDK-1572, which builds the client that holds a transport.
`Sources/PostgREST/Legacy/` is untouched.
18 tests. Full suite: 1291 passing.
`PostgrestTransport` underpins every `database.*` capability rather than belonging to one — it is how a caller takes over sending for any of them — so it goes under `supporting_symbols`, alongside the other cross-cutting PostgREST contracts.
grdsdev
force-pushed
the
guilhermesouza/sdk-1565-featpostgrest-add-the-public-transport-seam-over-httptypes
branch
from
August 25, 2026 09:25
654a25e to
c843481
Compare
Contributor
|
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-1565. Stage 2 task 3, stacked on #1270.
Settles the open question spec §7 says must be settled before stage 2: "How does a caller inject a transport?"
Nothing
package-scoped appears in that signature, which is the whole reason it can be public. The two alternatives §4.7 lists lose for concrete reasons: promotingHTTPRuntimecontradicts an explicit in-source comment (HTTPMethod.swift: "NEVER exposed as public SDK surface") and would putHTTPErrorunder ADR 0001 along with typed throws,HTTPResponseStream,ProgressHandlerandHTTPBody; a URLSession-only seam drops the arbitrary interception standalonePostgrestClientusers have today.The pipeline still speaks HTTPRuntime
Per §4.7,
HTTPRuntimeis reused rather than replaced. This is a façade over it, not a parallel implementation:HTTPRuntime.URLSessionTransportused directly — nothing convertsPostgrestTransportBridgeadapts it toHTTPRuntime.HTTPTransportSo the conversion cost is paid only by callers who take over the transport.
URLSessionPostgrestTransportis the shipped conformance, implemented overHTTPRuntime.URLSessionTransportrather than duplicating the URLSession handling. It exists mainly to be delegated to — a transport that only wants to observe or adjust should hold one and forward, which keeps URL assembly and its encoding rules in one place.Three conversion traps, each with a test
The path must not be decoded.
URL.pathandURLComponents.pathboth percent-decode, so reading either hands the transportselect=*where the wire carriesselect=%2A. The bridge readspercentEncodedPath/percentEncodedQueryand rebuilds a URL by concatenation rather than throughURLComponents, which would re-encode. This is the same hazard #1270 fixed one layer down; it would have been reintroduced here.A file-backed body is refused, not buffered. Reading the file in would defeat the reason
HTTPBody.fileexists. PostgREST never produces one, so this throwsunsupportedRequestBodyrather than quietly working.Streaming fails loudly. A
PostgrestTransportreturns a buffered(Data, HTTPResponse). Buffering the whole body and handing back a one-chunk stream would look like streaming while defeating the point of it.Header names keep the casing they were written with —
rawName, notcanonicalName, which lowercases for HTTP/2 — and repeated names merge case-insensitively, joined with", "per RFC 9110 §5.3.A curl snapshot cannot check the thing that matters
sendsTheQueryStringExactlyAsGivenasserts the recordedURLRequest.urldirectly, and the comment says why. Written first withMock.snapshotRequest, it recordedwhile the wire carried
?select=%2A&to=eq.%2B16505555555. That renderer sorts the query items and re-encodes them throughURLComponents, normalizing%2Aback to*and%2Bback to+— precisely the regression the test exists to catch. This is the hazard I flagged on #1270 forHTTPRuntimeTestHelpers.curlCommand, now confirmed forTestHelpers'._curltoo. Worth knowing before SDK-1566 and SDK-1568 lean on those helpers.The test also proves the strict escaping survives a real
URLSession, not just a string assertion — it goes out through Mocker and comes back byte-identical.Not in this PR
The plan's Task 3 also deletes the hand-rolled retry loop and re-checks
db.retry. Both need an execution path on the new core, and the typed API still executes throughLegacy/. They belong with SDK-1568, which rebuilds the wrappers, and SDK-1572, which builds the client that holds a transport. Nothing wires a transport into a client yet, so this PR is the seam plus its bridge.Sources/PostgREST/Legacy/is untouched.Verification
swift test— 1291 tests in 135 suites pass (18 new)./scripts/format.sh,./scripts/spell-check.sh— clean./scripts/test-docs.sh— clean, no broken DocC linkssdk-compliance.yaml— 5 new symbols undersupporting_symbols, in its own commit