Make BundledHTTPRequest's init checks hold, and add Equatable - #38
Merged
Conversation
init() rejects a .get with a body and a url with no scheme, but both stored properties were publicly settable, so a caller could assign a body onto a .get or flip the method of one that had a body and reach a state init() rejects. Validation was advisory rather than structural. body is now let and request is private(set). Header fields cannot invalidate either check, so settingHeader(_:for:) keeps them settable through a narrow API. Two bugs found while writing the tests: init() called assert(false) before throwing missingScheme, so in a debug build that path trapped rather than surfacing the error, and could not be tested. HTTPRequest cannot represent a urn: or mailto: url - it drops everything after the scheme and reports url == nil. Such a request could never be sent, and any comparison through request.url called it equal to every other url of that shape. init() now rejects them with unrepresentableURL, and Equatable is synthesized so it compares scheme, authority and path structurally. Source-breaking: oauth4swift assigns headers in place at three sites and needs the companion change to settingHeader(_:for:). No other consumer mutates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 83f028d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merged
germ-mark
added a commit
that referenced
this pull request
Jul 29, 2026
The copy here would not have failed the build - it compiles with only "conformance was already stated in the type's module", and the owner's witness silently wins - so the mocks' == would have been shadowed rather than rejected. Also covers the .head rejection this branch added, which had no test, and the matching case where a bodiless method carries no body. Co-Authored-By: Claude Fable 5 <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.
Split out of #37, which had grown core-library changes inside a testing-mocks PR. This is independent of #32 and targets
main.initrejects a.getwith a body and a url with no scheme, but both stored properties were publicly settable, so a caller could assign a body onto a.get, or flip the method of one that had a body, and reach a stateinitrejects. Validation was advisory rather than structural — which is also why the guard inURLSession.data(for:)is load-bearing rather than redundant.bodyis nowletandrequestisprivate(set). Header fields cannot invalidate either check, sosettingHeader(_:for:)keeps them settable through a narrow API.Two bugs found while writing the tests
missingSchemewas unreachable in debug.initcalledassert(false)before throwing it, so that path trapped rather than surfacing the error — and could not be tested at all. My first attempt at a test segfaulted the suite.urn:andmailto:urls produced requests that could never be sent.HTTPRequestdrops everything after the scheme for those and reportsurl == nil. Anything comparing throughrequest.urlthen called every url of that shape equal to every other:initnow rejects them with the newHTTPRequestError.unrepresentableURL.Equatableis synthesized —HTTPRequestis alreadyHashable— so it compares scheme, authority and path structurally rather than depending on that rejection staying in place.Source-breaking
oauth4swift assigns header fields in place at three sites (
DPoP/Signing.swift, and twice inSession/OAuthSession+AuthRequest.swift) and needs the companion migration tosettingHeader(_:for:). I verified the break rather than assuming it: unfixed oauth4swiftmainagainst this fails with "'request' setter is inaccessible", and builds clean with the migration. A multiline-aware sweep of oauth4swift, AtprotoOAuth, AtprotoClient, GermShared, CoreAppLogic and ATResolve found no other mutator.The companion commit is ready on oauth4swift's
llm/adopt-germconvenience-api, waiting on this release. Noteminordoesn't actually gate consumers on 0.x, since SwiftPM'sfrom:isupToNextMajor— the lockstep release is what protects them.Test notes
38 tests, 10 suites, clean build, no warnings.
settingHeaderis covered including the two behaviours it inherits fromHTTPFields: repeated fields collapse to one comma-joined value, and a pseudo-header name traps.Once this lands, #32's
extension BundledHTTPRequest: EquatableinGermConvenienceMocksshould be deleted — it would otherwise be a duplicate conformance. There's a comment marking it.🤖 Generated with Claude Code