Serve createRecord from MockPDS - #46
Merged
Merged
Conversation
Same guards and body handling as `putRecord`, but the PDS mints the record key instead of taking it from the input — a TID, since that is what the key callers split back out of `uri` has to parse as. Production paths that create records (blocking, for one) 400'd against the mock and so could not be tested at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6382405 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 |
Three fixes from review of the createRecord handler: Record uris were built in two places and had drifted. Reads reported `at://did:web:example.com/NSID(rawValue: "app.bsky.graph.block")/<rkey>` — a hardcoded authority, and `collection` interpolated as a struct instead of its rawValue — so the uri create returned and the uri a later read reported were different strings for the same record. `MockRepo` now owns the one builder and knows its own DID. The lexicon's optional rkey is no longer honored. Accepting it without also modeling the already-exists failure was putRecord wearing create's name, and that branch had no test. It is refused with a 400 that says to use putRecord. `createWithoutAuthIsRejected` asserted only that something threw, which an unserved endpoint's 400 satisfies just as well — it passed with the handler deleted. It now asserts the 401. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cid` was the literal "mock" on both createRecord and putRecord, which does not parse as a CID — no `b` prefix, not base32 — so anything feeding it back failed at the boundary. `PutRecordOutput.cid` is typed String, so nothing catches that at decode. It is now `Atproto.CID.mock().string`, which is what reads already returned. putRecord's uri was the placeholder "example.com". It now goes through the same builder as create and read, so all three name the record the same way. The 200-with-JSON envelope was hand-built at five sites across MockPDS and MockRepo. `HTTPDataResponse.mock(json:)` / `.mock(encoding:)` join the existing `.mock(error:status:)` family; net 51 fewer lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0.3.0 consolidates every failure onto `HTTPResponseError.unsuccessful` and reads the body through `bodyString`; `.unsuccessfulString` is no longer thrown from that module, so `getRecord`'s catch of that case would have gone dead. It matches the type and uses the accessors now. The mock's generic 400s carried "Invalid Request" — with a space, which is not an atproto error name — so `parse` matched nothing in `badRequestErrors` and every one of them surfaced as an opaque `.unrecognized(400 )`. They are "InvalidRequest" now, so consumers get a typed `.xrpcError`. "Unauthorized" (401 is a recognized status) and "RecordNotFound" (declared by GetRecord) were already fine and are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…'s uri Review follow-ups. `mock(encoding:)` accepted an already-encoded `Data` and re-encoded it as a base64 string. It passes Data through now, the way GermConvenience's `Data.decode` special-cases the same type on the way in. Note the obvious guard does NOT work: an `@available(*, unavailable)` Data overload still loses resolution to the generic one and the misuse compiles silently — verified, and the pass-through is pinned by a test instead. The create suite had grown put's response-shape tests and a general error-vocabulary test. Split into MockPDSPutRecordTests and MockPDSErrorTests, with the repeated hosting/rkey setup in MockPDSFixture. `getRecord` embeds a uri on a path `listRecords` does not share, and nothing asserted it — the typed accessor drops everything but `value`. Asserted through `callExpectingOptional`. Also: `MockRepo.did` is internal rather than public (nothing outside needs it), and the stale `// TODO: Mock CID` is gone — that cid was always a real mock CID. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
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
1 participant
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.
MockPDS.handleXrpcserved getRecord/listRecords/putRecord/deleteRecord and 400'd on everything else, so no production path that creates a record could be tested against the mock at all.Adds
com.atproto.repo.createRecord: sameauthedDidguard and body decoding asputRecordbeside it, but the PDS mints the record key instead of taking one from the input — a TID (Atproto.TID.mock()), since that is what callers split back out ofuriand parse.MockRepoalready stored records untyped by (collection, rkey), so storage is unchanged. Returns the realuri, since unlike put the caller did not choose the key.Test notes: new
MockPDSCreateRecordTestscovers create-then-read-back at the minted key, two creates landing as two distinct records, and the unauthenticated 401. Full suite green.Downstream: this unblocks
AuthDIDManager.block(target:)coverage in germDM — verified against a local path override there, where the test fails with.unrecognized(400 )on 0.6.0 and passes on this branch.🤖 Generated with Claude Code