Move circle definitions into the Circle table - #1661
Closed
toddmitchell wants to merge 3 commits into
Closed
Conversation
Groundwork for the enrollment model. Definitions lived in the shared key-three-value blob, which is why AppId and GrantOn could never be queried or constrained -- the columns shipped dormant with the drive-addressing schema work and nothing was ever wired to them. TableCircle had no caller at all. The enrollment pipeline has to answer "which circles enrol on connect?" on the hot path, and that is a WHERE GrantOn = ? against an indexed column, not a load-all-deserialize-filter over opaque rows. So the definitions have to live in the table before any of Cat 3 can be built. - CircleDefinition gains AppId, GrantOn, Designation and Emoji. All four are [JsonIgnore]: they are columns, and a second copy inside the row's data blob would let a query on the column disagree with the hydrated object. Equality and GetHashCode account for them, since EnsureSystemCirclesExist reconciles definitions by comparing them. - RedactedCircleDefinition exposes the same four without [JsonIgnore] -- it is the wire shape, and the client proposal renders circles by designation and emoji. - CircleGrantOn and CircleDesignation are new enums matching the column values. Every existing circle is None/Personal, so nothing changes behaviour until something sets them. - TableCircle grows UpsertAsync and GetAllAsync; TableCircleCached wraps both and invalidates the all-key alongside the per-circle key. - CircleDefinitionService moves off ThreeKeyValueStorage onto db.CircleCached with ToRecord/FromRecord doing the column-vs-blob split. - v13 -> v14 copies existing definitions across. Idempotent and additive: a definition already in the table is left alone, so a partial run repeats safely. The blob rows are deliberately left in place -- if this goes wrong the source data is still there. Cleaning them up is a separate job. No behaviour change. The four fields take their defaults for every existing circle, which is what they already were. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 21, 2026
[JsonIgnore] was the wrong tool. CircleDefinition is not only the stored shape -- CircleDefinitionControllerBase serves it directly and takes one as an update body -- so hiding AppId, GrantOn, Designation and Emoji from JSON starved clients of fields the client proposal renders circles by, and worse, made every client-side circle update arrive with GrantOn defaulted and silently reset it. Data loss on an ordinary edit. One attribute cannot make one type be both the wire shape and the stored shape. The fields are back on the wire, and the blob copy is cleared inside ToRecord instead -- the same clear-before-serialize trick ToConnectionsRecord already uses for the grant collections. The guarantee is unchanged: nothing in the blob can drift from the column, because deserializing the blob alone yields defaults. AppId is also no longer taken from an update request. Ownership is set when the circle is created and must not be reassignable by anyone who can PUT a definition. Tests pin both directions: no promoted value survives into the blob, the caller's object is intact afterwards, the fields round-trip through the record, they are visible on the wire, and an update body echoed back does not reset GrantOn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
toddmitchell
marked this pull request as ready for review
August 22, 2026 00:59
Member
Author
|
Superseded by #1688, which replays this onto |
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.
Groundwork for Cat 3 (the enrollment model). Stacked on
review-stamp-cat1(#1649) — retarget tomainonce that lands. Independent of Cat 2 (#1655).Why this has to happen first
Circle.AppId,GrantOn,DesignationandEmojishipped dormant with the drive-addressing schema work. Nothing was ever wired to them — in factgrepforTableCircleacrosssrc/servicesandsrc/appsreturns nothing. The table has no callers at all.Circle definitions actually live in
CircleDefinitionService'sThreeKeyValueStorageblob, which is exactly what part 1 warned about:The enrollment pipeline is specified as a table query (part 2, L188-194):
You can't run that against a blob store. So the definitions have to move before any of Cat 3 can be built.
What changed
CircleDefinitiongainsAppId,GrantOn,Designation,Emoji. They're columns, and a second copy inside the row'sdatablob would let a query on the column disagree with the hydrated object — the same promote-don't-duplicate rule Cat 1 applied toReviewedAt.They are not
[JsonIgnore], and the second commit explains why: this same type is the wire shape.CircleDefinitionControllerBaseserves it directly and takes one as an update body, so hiding the fields from JSON would starve clients and make every client-side circle update arrive withGrantOndefaulted — silently resetting it. Data loss on an ordinary edit.One attribute can't make one type be both the wire shape and the stored shape, so the blob copy is cleared inside
ToRecordinstead, the same trickToConnectionsRecorduses for the grant collections.AppIdis also no longer taken from an update request: ownership is set at creation and must not be reassignable by anyone who can PUT a definition.Equality and
GetHashCodeaccount for all four, becauseEnsureSystemCirclesExistAsyncreconciles definitions by comparing them — miss that and system circles silently stop reconciling.RedactedCircleDefinitionexposes the same four. Part 4 renders circles by designation and emoji.New enums
CircleGrantOn(None | Connect | OwnFlowConnect | Review) andCircleDesignation(Personal | Audience | Vendor), matching the column values.Table layer —
TableCirclegrowsUpsertAsyncandGetAllAsync;TableCircleCachedwraps both and invalidates the new all-key alongside the per-circle key.CircleDefinitionServicemoves offThreeKeyValueStorageontodb.CircleCached, withToRecord/FromRecorddoing the column-vs-blob split.v13 → v14copies existing definitions across. Idempotent and additive — a definition already in the table is skipped rather than overwritten, so a partial run repeats safely. The blob rows are deliberately left in place: if this goes wrong the source data is still sitting there. Cleaning them up is a separate job.Behaviour
None. All four fields take their defaults for every existing circle (
AppIdnull,GrantOnNone,DesignationPersonal,Emojinull), which is what they already were.What's not here
The other part-1 leftover:
AppRegistrationsexists as a table butAppRegistrationServicestill writes to theKeyThreeValueblob. Nothing in the circle work needs it untilDefaultCircleslands in the next PR, so it's cleaner as its own change than bulking this one out.Testing
Full solution suite green: 2,524 passed, 0 failed (68 skipped, all pre-existing
[Explicit]).Not verified: the v13→v14 migration has not been run against a live PostgreSQL or SQLite tenant holding real blob-stored definitions. Given this moves where every circle definition lives, that's the check I'd want before it leaves draft.
🤖 Generated with Claude Code