Adopt the conformance-fixtures wire contract for records - #64
Merged
Conversation
…rface
@haverstack/conformance-fixtures published (core#167 cleared), so this
brings the records routes in line with what it actually pins, going well
beyond this issue's original two work items once the full fixture set was
read:
- POST /records accepts a full record body, not just {typeId, content}:
id is client-minted (12 lowercase Crockford base-32 chars, no reserved
"_" prefix) and optional -- forwarded to ScopedStack.create() as opts.id,
which generates one when omitted. createdAt/updatedAt/version are never
accepted from the client, same as entityId/principalId. Returns 200, not
201, matching every other write in this API.
- PATCH /records/:id: the body IS the content patch (RFC 7396), never an
envelope -- fixes the wire-dishonesty bug where a conforming client's
{"title": "New"} read as body.content (undefined) and silently no-opped
while still bumping version. Also wires up If-Match -> ifVersion for
optimistic concurrency (412 version_conflict on mismatch).
- Added POST /records/:id/undelete (ScopedStack.undelete() had no wire
endpoint at all).
- Moved association removal from DELETE /:id/associations (a body-bearing
DELETE, undefined semantics per RFC 9110 SS9.3.5) to POST
/:id/associations/delete, the shape the fixtures pin.
- PUT /:id/permissions now returns 204 with no body instead of echoing the
permissions back at 200.
- Verified (no code changes needed, core already gets these right once the
right value reaches it): malformed/unknown-sort-field cursors -> 400
bad_request; reserved content keys (__proto__ etc) on PATCH -> 422 now
that the raw patch reaches Stack.update()'s validateReservedKeys();
payload-too-large record bodies -> 413 via the existing JSON body limit;
omitted `limit` on GET/POST query -> one default-sized page (50), not
everything.
Two things intentionally NOT done here, both documented rather than
worked around:
- POST /records/:id/migrate is not implemented. core@0.9.0's ScopedStack
has no scoped, permission-checked path for an ad-hoc per-record
migration commit -- commitMigration lives only on the raw StackAdapter,
called exclusively by the unscoped Stack.migrateAll() (bulk,
registered-migration-fn driven, not the client-computed-content shape
this endpoint needs). Implementing it would mean either bypassing
ScopedStack's permission layer or duplicating it at the route level --
both the exact anti-pattern recent sync work has been eliminating.
Needs a core-side addition first, same shape as putAttachment()'s
history.
- Three id-format validation fixtures (malformed charset, wrong length,
reserved "_" prefix) expect 400 bad_request; installed
@haverstack/core@0.9.0 (npm's latest) still throws StackValidationError
(422) for all three, with the same message text the fixtures expect for
400. This is a version-skew gap between the fixtures and the latest
published core, not a server bug -- tests pin the actual current
behavior with an explanatory comment instead of reclassifying errors by
string-matching, which the codebase has been deliberately removing
elsewhere (see #33).
Refs #32. Also adopts @haverstack/conformance-fixtures as a dev dependency
for reference, though the tests here are hand-written per the codebase's
existing style rather than a generic fixture-iterating harness.
…odes
Both core-side gaps flagged in the previous commit are fixed upstream:
- ScopedStack.commitMigration(id, toTypeId, content, opts?) now exists,
owner-acting-alone only per its own doc comment. Implements
POST /records/:id/migrate: {toTypeId, content} body, requireOwner()
gated (matching the doc's explicit instruction to answer 403 to anyone
else), validates toTypeId is a registered type before calling in,
accepts If-Match the same as PATCH.
- Id-format validation (malformed charset, wrong length, reserved "_"
prefix) now throws StackQueryError (400 bad_request) instead of
StackValidationError (422). Updated the three tests that were pinned
to the old behavior with an explanatory comment; that comment and the
version-skew note are gone.
Bumping @haverstack/core alone broke error serialization repo-wide: 57
tests started returning 500 where a typed status was expected.
@haverstack/wire-types (0.7.0, npm's latest) and the previously-installed
@haverstack/adapter-local (0.7.0) both still depend on core ^0.9.0, so
pnpm resolved two separate copies of @haverstack/core — server code threw
errors as 0.10.0 class instances, but wire-types' serializeError() did
`instanceof` checks against its own 0.9.0-resolved classes. Those checks
silently failed, serializeError() returned null, and every core error
fell through to the generic 500 handler instead of its real status/code.
@haverstack/adapter-local has a 0.8.0 already published against core
^0.10.0, bumped to it. @haverstack/wire-types has no core-0.10-compatible
release yet, so a pnpm.overrides pin forces a single deduped
@haverstack/core resolution across the tree in the meantime — verified
by confirming the 57 spurious 500s disappear with it and reappear
without it. This should come out once wire-types republishes.
Refs #32.
wire-types 0.8.0 depends on core ^0.10.0 (its "@haverstack/core": "workspace:^" now resolves correctly), so the pnpm.overrides pin forcing a single deduped core install is no longer needed for the path it was protecting -- confirmed by removing it and running the full suite clean. Two other stale-core paths remain in the dependency tree (blob-adapter-disk@0.7.0, a transitive dep of adapter-local, still on core ^0.9.0; conformance-fixtures@0.1.0 pulls in an old wire-types which pulls in core 0.9.0 too) but neither matters in practice: all 167 tests pass, including every attachment test that exercises blob-adapter-disk, and conformance-fixtures is dev-only with no runtime interaction. Left alone rather than overridden preemptively for a problem that isn't actually occurring. Refs #32.
4 tasks
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
@haverstack/conformance-fixturesis now published (core#167 cleared). This issue's text named two work items (PATCH body shape,/migrate), but reading the actual fixture data (createRecordFixtures,patchContentFixtures,deleteRecordFixtures/undeleteRecordFixtures,associateFixtures/dissociateFixtures,setPermissionsFixtures,errorResponseFixtures,queryRecordsFixtures,commitMigrationFixtures) turned up a much larger set of divergences in the records route surface. Fixed all of them:POST /recordsaccepts a full record body, not just{typeId, content}.idis client-minted (12 lowercase Crockford base-32 characters, no reserved_prefix) and optional — forwarded toScopedStack.create()'sopts.id, which generates one when omitted.createdAt/updatedAt/versionare never accepted from the client, same treatment asentityId/principalId. Returns200, not201— matches every other write in this API.PATCH /records/:id: the body IS the content patch (RFC 7396), never an envelope. Real wire-dishonesty bug: a conforming client's{"title": "New"}read asbody.content(undefined) and silently no-opped while still bumpingversion. Also wires upIf-Match→ifVersionfor optimistic concurrency (412 version_conflicton mismatch).POST /records/:id/undelete— had no wire endpoint at all.POST /records/:id/migrate— owner-acting-alone only (perScopedStack.commitMigration()'s own doc comment, added in core 0.10.0), body{toTypeId, content}, validatestoTypeIdis registered, leaves a pre-migration snapshot.DELETE /:id/associations(a body-bearingDELETE— undefined semantics per RFC 9110 §9.3.5) toPOST /:id/associations/delete.PUT /:id/permissionsnow returns204with no body, instead of echoing the permissions back at200.400 bad_request; reserved content keys (__proto__etc.) on PATCH →422; payload-too-large record bodies →413; omittinglimit→ one default-sized page (50), never everything._prefix) now correctly returns400 bad_request— was422 validationin core 0.9.0, fixed upstream in 0.10.0.Core update
Bumped
@haverstack/coreto^0.10.0,@haverstack/adapter-localto^0.8.0, and@haverstack/wire-typesto^0.8.0— all now consistently on core^0.10.0, so nopnpm.overridespin is needed. (Briefly was: bumping core alone broke error serialization repo-wide, sincewire-types' then-published version still resolved core^0.9.0via itsworkspace:^snapshot from its last release, producing two separate installed copies of@haverstack/coreand silently-failinginstanceofchecks inserializeError(). Fixed properly oncewire-typesrepublished rather than carrying the override.)Two other stale-core paths remain in the dependency tree (
blob-adapter-disk@0.7.0, transitive viaadapter-local, still on core^0.9.0;conformance-fixtures@0.1.0pulls in an oldwire-typestoo) but neither matters in practice — verified the full suite passes clean, including every attachment test exercisingblob-adapter-disk, andconformance-fixturesis dev-only with no runtime interaction. Not overridden preemptively for a problem that isn't actually occurring.Also adopted
@haverstack/conformance-fixturesas a dev dependency for reference, though the tests here are hand-written per the codebase's existing style rather than a generic fixture-iterating harness.Test plan
pnpm typecheckpnpm lintpnpm format:checkpnpm test— 167/167 passing. Covers: client-minted id (success, generated-when-omitted, three malformed cases → 400, duplicate → 409),If-Match/412 concurrency,POST /:id/undelete(success, idempotency, 403, 404),POST /:id/migrate(success, 403 non-owner, 401, unregistered toTypeId, schema failure, 404, pre-migration snapshot),PUT/GET /:id/permissions(previously untested), reserved content keys via PATCH, pagination defaults, malformed/unknown-sort-field cursors, oversized record body → 413.