refactor(api): session responses use explicit DTOs; published OpenAPI schema finally matches the wire#1024
Merged
Merged
Conversation
…truct
The blocking endpoint answered with an ad-hoc map{"session": sessionState}
whose bytes were shaped by a custom SessionState.MarshalJSON, while the
OpenAPI schema was generated from the struct tags. The two had drifted
apart from day one: the published schema said `instances` was a map of
name to entry and had no `status` property, while the wire carried an
ARRAY of {instance, error} entries plus `status`. Anyone building a
reverse-proxy plugin against the published contract parsed the wrong
shape. The SessionResponse/ThemesResponse types existed only to feed the
annotations; nothing kept them in sync with what handlers actually wrote.
The handlers now build and return the annotated DTOs (NewSessionResponse,
ThemesResponse), so the generated schema is derived from the same types
that produce the bytes and cannot drift again. The wire shape is unchanged
except that instance entries are now sorted by name instead of
map-iteration random (ordering was never part of the contract).
SessionState loses its MarshalJSON: the domain type no longer carries wire
concerns, which is stage 2 of the InstanceInfo/session split (stage 1:
#1023). Its marshalling tests move to the API package against the DTO,
joined by a wire-shape pin so the array+status contract is enforced by
test rather than by accident.
github-actions
Bot
added
the
documentation
|
Test Results✅ All tests passed! | 781 tests in 103.295s |
|
acouvreur
added a commit
that referenced
this pull request
Jul 11, 2026
Both stores serialized the raw domain InstanceInfo as the persisted session
entry, making the storage schema "whatever the domain struct happens to
marshal as": every field added to InstanceInfo silently became persisted
state with no version to gate evolution and no way to recognize records
written by an older release.
Sessions are now wrapped in a SessionRecord envelope ({"v":1,"instance":
{...}}) owned next to the Store interface. Its unmarshaler transparently
upgrades pre-versioning payloads (bare InstanceInfo documents), so live
sessions in valkey and state.json snapshots survive the upgrade instead of
being dropped. Foreign keys sharing the valkey keyspace keep the previous
tolerance: they decode to a record with an empty instance name and are
skipped by the existing name check.
The Store interface is unchanged (Get/Put still speak InstanceInfo), so the
core and providers are untouched; this is stage 3 of the InstanceInfo split
(stage 1: #1023, stage 2: #1024) and the prerequisite for slimming what
sessions actually persist, which will be a schema-version bump instead of a
silent break.
Migration coverage: record round-trip and legacy-upgrade unit tests, an
inmemory legacy-snapshot load test, and valkey testcontainers tests reading
a legacy payload through Get and Range and asserting new entries carry the
version. The sabliercmd storage fixtures already use the legacy shape, so
the state-file load tests now exercise the upgrade end to end.
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.
Finding
Stage 2 of the
InstanceInfo/session split (stage 1: #1023). This one targets the wire-format facet: the API contract was the domain struct, and the published contract was already wrong.The blocking endpoint returned an ad-hoc map with the domain object inside:
whose bytes were shaped by a custom
SessionState.MarshalJSON- but swag generates the OpenAPI schema from struct tags, not custom marshalers. The two have disagreed since day one:instances{instance, error}entriesstatus"ready"/"not-ready"errorAnyone building a reverse-proxy plugin against the published
openapi.jsonparsed the wrong shape.SessionResponse/ThemesResponseexisted only to feed annotations; handlers returned ad-hoc maps, so nothing could keep them in sync. And every future field on the domain struct silently became API surface.Discovery
Found during the API-layer design review by diffing
SessionState.MarshalJSON's output against thesablier.SessionStateschema in the committedopenapi.json.Fix
NewSessionResponsemaps the domain session into explicit DTOs (SessionResponse->SessionStateResponse->InstanceEntryResponse), and the handler returns the annotated type. The themes endpoint likewise returns the realThemesResponse. Schema and wire are now generated from the same types - they cannot drift again.SessionState.MarshalJSONis deleted: the domain type no longer carries wire concerns. Its tests move to the API package against the DTO.openapi.jsonregenerated: it now tells the truth (array +status+ stringerror). This changes the published spec, not the wire - the direction that fixes consumers.Wire compatibility
Byte-shape unchanged, with one deliberate improvement: instance entries are now sorted by name instead of map-iteration random. Ordering was never part of the contract and non-determinism was only ever a nuisance.
The inner
instanceobject intentionally still serializes the domainInstanceInfo- trimming it to "the stable fields plugins need" is a breaking change that belongs with the API-versioning work, and stage 3 (versioned session record) is the prerequisite for dropping the flat fields.Tests
TestNewSessionResponse- sorted entries, error-string mapping, status aggregation, empty-session-as-[].TestSessionResponseWireShape- pins array +status+ omitted-empty-error, so the historical contract is enforced by test rather than by accident.MarshalJSONtests; full API + core suites andgolangci-lintpass;make check-generategreen.Docs
openapi.jsonis the only doc artifact affected (regenerated; now accurate). No labels/config/behavior change.