feat(reservation): surface committed + opt-in metadata on listReservations (v0.1.25.36)#202
Merged
Merged
Conversation
…tions (v0.1.25.36) Implements #201 (follow-up to #197) against cycles-protocol v0.1.25.8 (runcycles/cycles-protocol#115). The list path already hydrated a full ReservationDetail per row, but toSummary dropped committed / metadata / committed_metadata before serializing — the data was read then thrown away. Hybrid projection: - committed (the COMMIT charge, a small scalar) now projects UNCONDITIONALLY on list rows, on the same footing as finalized_at_ms; NON_NULL strips it on non-COMMITTED rows. - metadata (reserve-time) and committed_metadata (commit-time) are arbitrary-size, possibly-PII maps, so they stay OFF list rows by default and are projected only when the caller opts in via a new comma-separated `include` query parameter (?include=metadata,committed_metadata). Unrecognized / empty tokens are ignored without error. The three fields move from ReservationDetail up to the shared ReservationSummary base (ReservationDetail becomes a marker subclass; the single-GET path still serializes all three). A new ReservationInclude enum parses the param (locale-root, case-insensitive). `include` is projection-only and is deliberately NOT folded into FilterHasher, so changing it mid-pagination never invalidates a cursor. listReservations gains an include-aware overload; the legacy 18-arg signature delegates with an empty set, which normalizes null and takes a private EnumSet snapshot. Tests: ReservationIncludeTest (parse edge cases), RedisReservationQueryTest +5 (committed-always + include combinations + legacy-overload default), ReservationControllerTest +3 (param parsing/threading). Full mvn verify green against the v0.1.25.8 spec, JaCoCo 95% gate met. AUDIT.md updated. Spec-first: gated on cycles-protocol#115 reaching main before the contract test resolves the new fields against the published spec.
…ursor Adds RedisReservationQueryTest#includeDoesNotBindCursor, the sorted-pagination case the review flagged as missing: page 1 with no include captures the cursor, page 2 reuses that cursor while flipping include=committed_metadata. Asserts the cursor stays valid (no 400, contrast cursorMismatchRejected where sort_by binds) and that the newly-requested field projects onto page 2. Locks in the projection-only / not-cursor-bound guarantee against a future refactor that might fold include into FilterHasher.
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.
Summary
Implements #201 (follow-up to #197): surface
committed,metadata, andcommitted_metadataon theGET /v1/reservationslist rows. These already appear on the single-rowGET /v1/reservations/{id}but were dropped from list responses — the list path already hydrated a fullReservationDetailper row, thentoSummarydiscarded them before serializing.Implements the hybrid design agreed on the issue, per spec cycles-protocol v0.1.25.8 (runcycles/cycles-protocol#115):
committed(the COMMIT charge — a small scalar) → projected unconditionally on list rows, on the same footing asfinalized_at_ms(NON_NULLstrips it on non-COMMITTED rows).metadata(reserve-time) andcommitted_metadata(commit-time) → arbitrary-size, possibly-PII maps, so omitted from list rows by default and projected only when the caller opts in via a newincludequery parameter.Changes
includequery param onlistReservations: comma-separated field list (?include=metadata,committed_metadata). Recognized tokensmetadata/committed_metadata; unrecognized / empty tokens ignored (no 400). Parsed by a newReservationIncludeenum (Locale.ROOT, case-insensitive).ReservationDetailup to the sharedReservationSummarybase;ReservationDetailbecomes a marker subclass (single-GET still serializes all three unconditionally).includeis projection-only — deliberately not folded intoFilterHasher, so changing it mid-pagination never invalidates a cursor (contrast the window filters, which do bind the cursor).listReservationsgains an include-aware overload; the legacy 18-arg signature delegates with an empty set. The overload normalizes a null arg and takes a privateEnumSetsnapshot (defensive copy).0.1.25.35 → 0.1.25.36;AUDIT.mdupdated.Tests
ReservationIncludeTest— parse edge cases (null/blank/whitespace/case/unknown/duplicate/trailing-comma).RedisReservationQueryTest+5 — committed-always; each include combination; legacy-overload default.ReservationControllerTest+3 — param parsed → empty / both / ignored-unknown sets threaded.mvn verifygreen against the v0.1.25.8 spec; JaCoCo 95% gate met.Review
Codex read-only pass: 2 Low findings, both applied —
Locale.ROOTlowercase in the parser, and null/defensive-copy normalization of theincludeset at the overload entry.Merge order
Spec-first. Gated on runcycles/cycles-protocol#115 reaching
mainfirst — the contract test (OpenApiContractDiffTest) fetches the spec fromcycles-protocol@main, so it will only resolve the new fields/param once #115 merges.