feat: aippatch — proto/SQL PATCH framework for AIP-134 RPCs#177
Open
btc wants to merge 77 commits into
Open
Conversation
A small Go library plus a sibling code generator that turn AIP-134 PATCH RPCs into safe, dynamic Postgres UPDATE statements. The proto message and FieldMask carry intent on the wire; an aippatch.yaml plus codegen produce a typed Mapping[T] per resource; a runtime Apply[T] call validates the mask, builds the SQL, executes it, and returns the post-update proto. Built first at drill/thirdparty/aippatch/ with UpdateProfile as the v0 target; designed for clean extraction into a standalone module reusable across Spanda LLC products. Codegen consumes proto FileDescriptorSet plus parsed sql/migrations via pg_query_go plus aippatch.yaml policy; output is committed *.gen.go files reviewable in PRs and guarded by a --check mode in CI. v0 codec set covers scalars, timestamps, and enums — sufficient to retire drill's hand-rolled UpdateProfile handler. JSONB, declarative validators, declarative authz, AIP-193 error mapping, and AIP-154 ETag are roadmap items; each is backwards-compatible with v0 call sites.
Auto-cancel a Pro subscription via cancel_at_period_end=true when the user has been idle for two consecutive billing periods. No settings toggle — this is the default Sabermatic behavior. Establishes a Spanda tenet: earn money for ongoing value delivered. Trigger fires at Stripe invoice.upcoming (~T-7) when MAX(last_active) across auth_sessions falls before the previous period's start, with launch-day grandfathering via users.idle_eligible_after. Reversal via email-link click (signed token, single-use) or auto-reverse on any authenticated activity during the cancel window. sub_cancel_is_auto distinguishes our auto-cancels from user-initiated portal cancels. Migrations 015–018: auth_sessions soft-delete, users sub-state cache, stripe_webhook_dedup, keep_link_token_uses. Feature package home at internal/feat/idleunsub/.
Both opus and sonnet review agents returned ~30 findings. Major changes:
HIGH
- Acknowledge AIP-134 empty-mask deviation explicitly (Wire conformance
note); ErrorOnEmpty stays as v0 default.
- Fix s.b.Pool() — it is a method, not a field.
- Drop "matches sqlc's DBTX" claim; document it is a strict subset
satisfied by *pgxpool.Pool, *pgx.Conn, and pgx.Tx.
- Remove mustValidate panic helper from generated file; show clean
Mapping literal plus generated InitPatches() error per drill's
no-panic-at-init rule.
- Replace ub.SQL("RETURNING *") with ub.Returning(boundCols...) —
first-class API, sends only mapped columns, excludes unmapped sensitive
columns from the wire.
- Add AutoSet mechanism (yaml auto_set: { updated_at: NOW() }) so PATCH
bumps updated_at like the existing sqlc UPDATEs do; without this every
PATCH would leave updated_at stale.
- Use proto.CloneOf instead of proto.Clone(...).(T); add nil op.Message
guard.
- Add buf build -o buf.binpb to make codegen — current buf generate does
not emit a descriptor set.
MEDIUM
- Add Transactions subsection: Apply takes any DBTX including pgx.Tx.
- Add audit-logging hooks (returned diff) to v1 roadmap.
- Document CGO requirement for aippatchgen explicitly.
- Add NOT NULL extraction to migration replay; nullable columns deferred
to v1.
- Define module-extraction trigger as "second project consumes aippatch
in production" (v0.5).
- Audit existing test wording in rollout plan.
- Split smallint into separate type-table row with int16-to-int32
widening on decode.
- Add validated bool to Mapping; Apply rejects unvalidated mappings with
Internal.
LOW
- Document v0 nested-mask-path constraint and emit codegen diagnostic.
- List github.com/google/uuid in runtime imports.
- Add bytes/float/double to deferred-types table.
- State codecs are global and reusable across resources.
- Make algorithm step explicit that unmatched proto fields without
skip:true are diagnostic.
- Add go get step to rollout plan.
- Note RETURNING uses bound-columns list (privacy benefit).
- Correct codec key description (yaml name, not protoreflect name).
- Add grep-for-callers step before deleting UpdateDisplayName.
NIT
- Use op.Mask.GetPaths() throughout.
- Clarify two-phase ordering (field-number processing, alphabetical emit).
- Spell out "foreign-key constraints".
- Document yaml string-to-Go-enum mapping for empty_mask.
- Diagram label uses thirdparty/aippatch/cmd/aippatchgen.
- Spanda replication wording: runtime/codegen are drill-free, not the yaml.
Spec grew from 690 to ~770 lines.
Round 2 surfaced one structural bug (m.codecs referenced but missing from Mapping struct), pg_query_go v5/v6 drift, several real correctness issues, and several smaller items. Fixes: HIGH - Add Codecs map[string]EnumCodec plumbing: shared Codecs registry in generated init.gen.go, passed into Mapping.Validate(codecs) which copies the relevant subset into unexported m.codecs. Apply now references m.codecs that actually exists. - Add EnumCodec type to public API (ProtoEnum, ToText, FromText). - Scrub pg_query_go to v6 throughout (latest published; v6.2.2). - Tone down Returning(boundCols) "privacy" claim — bound non-writable columns ARE transmitted (e.g. email); only unmapped columns are excluded. MEDIUM - Fix typed-nil trap: any(op.Message) == nil is false for a nil *T; switch to op.Message.ProtoReflect().IsValid(). - Sort op.Where keys before iterating to keep SQL deterministic (otherwise pgx prepared-statement cache misses on every call). - Validate op.Where keys exist in m.bindingsByColumn — prevents attacker-controlled identifier injection via the map-key surface. - Tighten AutoSet conflict check: reject if column is *any* binding (writable or not), not just writable. - Add pg_query_go-based validation for AutoSet SQL literal: parse and reject multi-statement / non-expression input. - Remove faulty test-wording audit step — verified existing tests assert via connect.CodeOf(err), not on message text. - Document soft-deleted user wire change: today's UpdateProfile returns Internal; aippatch returns NotFound. Add to Wire conformance note, rollout plan, and Risks. - Fix cmd/server/main.go → cmd/drill/main.go (drill's actual binary path). LOW - UpdateAllWritable empty-mask path: explicit Internal in v0 (was silently emitting AutoSet-only UPDATE). - Fix uuid.UUID(v).String() → uuid.UUID(v.([16]byte)).String() in type table. - Switch validated bool to atomic.Bool for race safety under -race. - Replace fictional maskHasPath with slices.Contains in handler example. - Add field-number-vs-alphabetical ordering explanation to algorithm. - Add GetPaths nil-safety note. - Note PK appears in Bindings (no dedup with PK column). - AutoSet test pattern: SELECT before / Apply / SELECT after — within a tx, NOW() is constant, so the framework's separate query advances it. - Remove dead yaml override comments (free_full_educators_used skip entry was never visited). NIT - AIP-203 → AIP-134 §Update_Mask citation. - "stale" → "superseded" sqlc deletion wording. - Architecture diagram pg_query_go shows version. Spec grew from ~770 to ~830 lines.
Both opus and sonnet round-3 reviews returned "Approve with minor revisions"
— no HIGH severity findings. Sonnet's "H1" was a redundant nil check that
opus correctly classified NIT (the IsValid check actually works); fixed as
cosmetic.
MEDIUM (both reviewers)
- Validate(codecs) now explicitly fails when a Binding references an
enum codec name not in the supplied codecs map. Closes the silent
failure mode where dangling refs would only surface at first Apply.
- Validate doc spells out FromText construction, dangling-codec check,
and validated-stays-false-on-error.
- Pin huandu/go-sqlbuilder to @v1.36.0 in `go get` step (UpdateBuilder.
Returning was added in that release).
LOW
- UpdateAllWritable empty-mask path now returns CodeUnimplemented (was
Internal); AIP-aligned for "feature not implemented".
- SET-clause iteration now walks Mapping.Bindings order (alphabetical)
filtered by mask membership, instead of client-supplied path order.
Makes SQL deterministic regardless of how clients order the mask
paths — pgx prepared-statement cache hits, and golden tests are
stable.
- Add codegen step rejecting EnumCodec maps with duplicate ToText
values (would produce a lossy FromText).
- Note `sqlbuilder.PostgreSQL.NewUpdateBuilder()` is required (default
flavor would emit MySQL-style `?` placeholders).
- Fix AutoSet test pattern — NOW() is constant within a transaction so
SELECT-before/Apply/SELECT-after inside BeginFunc would see equal
values. Use clock_timestamp() OR top-level statements OR capture
pre-Apply NOW() and assert post >= captured.
NIT
- Drop dead `op.Message.ProtoReflect() == nil` clause; ProtoReflect
never returns nil for protoc-gen-go types.
- Hoist ProtoReflect() to a `src` local instead of calling repeatedly.
- Use `ub.IsNull(m.SoftDelete)` instead of string concatenation.
- Add bullet for ALTER TABLE ADD/DROP CONSTRAINT and other unlisted
AlterTableCmd kinds being ignored in v0.
- Sort AutoSet alphabetically by Column for stable diff (in addition
to Bindings).
- Move nested-mask-path rejection out of proto-field-iteration loop
(proto field names never have dots; the check belongs at yaml
validation time on writable/overrides keys).
- Document CGO first-build cost (~3 min) and CI cache requirement.
- Align spec on Makefile's existing `generate` target (was claiming a
nonexistent `codegen` target).
Final spec is implementation-ready. Both reviewers gave "Approve with
minor revisions"; ending the review loop after round 3 per CLAUDE.md
("up to 3 rounds, end early if clean").
15 tasks across 6 phases covering migrations 015–018, the internal/feat/idleunsub/ feature package (token sign/verify, trigger evaluation, KeepSubscription, AutoReverse, email composition), Stripe webhook integration, the public /sub/keep endpoint, the auth-middleware AutoReverse hook, the user-info RPC + AckKeptBanner, and the React KeptBanner component. Acceptance criteria + manual smoke test + spec out-of-scope list at the end. Spec: docs/superpowers/specs/2026-05-07-idle-auto-cancel-design.md
Round 4: opus + sonnet both rated "Approve with minor revisions". Both flagged the same M1 (UpdateAllWritable error-code inconsistency across four sections). Opus added M2 (codegen column-uniqueness) and several LOWs/NITs; sonnet added decode fd-nil-guard. MEDIUM - Reconcile UpdateAllWritable across all four sections that mentioned it: Errors section now lists CodeUnimplemented; Roadmap aligns with Apply walkthrough's defense-in-depth check; yaml-mapping prose says codegen is the primary rejection point. - Codegen step 10 added: verify column uniqueness across emitted bindings to catch overlapping override.column entries that would produce duplicate RETURNING and SET clauses Postgres rejects. - Codegen step 9 added explicitly: yaml empty_mask: update_writable is rejected at codegen, not just at runtime. LOW - decode() in Apply step 7 now guards fd == nil before passing to decode, symmetric with encode() in step 2. - AutoSet NOW() gotcha explained inline in the canonical yaml example (before, only in Testing strategy — readers who scrolled past would write a flaky test). - Op.Where doc strings now state values must be scalar / pgx-bindable; list comparators (IN, !=, <) deferred to v1+. - Generalize "call patches.InitPatches()" error message to "your generated InitPatches()" — the runtime is consumer-agnostic. - PK-without-proto-field case explained: column simply doesn't appear in Bindings; WHERE uses m.PK for identity regardless. NIT - Decisions row 8 now matches the wire-conformance "permanent per resource" claim (was contradictorily saying "relax later"). - "drop" in algorithm step 3.iii reworded to "emit no binding; field is registered as intentionally skipped" so the relationship to step 4 is unambiguous. - Risks #1 first-build cost wording softened from "~3 minutes" to "several minutes (varies by runner)". - Validate concurrency note added: callers serialize startup; sync.Once is left to the caller if needed.
Round 5: opus + sonnet both rated "Approve with minor revisions". Real findings: MEDIUM - Validate concurrency wording was wrong: concurrent map writes ARE a Go data race even when content would be identical. Reword to "Validate is NOT safe for concurrent invocation on the same Mapping" and require callers to serialize. The generated InitPatches() does this naturally; sync.Once is left to callers needing strict guarantees. - Document partial-clone discard on decode error: every error path in Apply returns zero T (not the partial result). Implementer must not return the in-progress clone on the error branch. - AutoSet column identifier safety: codegen now restricts a.Column to match [A-Za-z_][A-Za-z0-9_]*. Reserved-word and special-character column names are rejected at codegen rather than handled at runtime with pq.QuoteIdentifier (deferred to v1 if needed). LOW - Add Diagnostics example for column-uniqueness check (algorithm step 10) so it matches the pattern of every other step's diagnostic. - Add Diagnostics example for AutoSet identifier-safety rejection. - Reword Op.Where slice-value comment: it does not produce "col = ARRAY[…]" in the SQL string (the SQL is just "col = $1"); the array shape is a pgx wire-protocol artifact. Point preserved (slices won't IN-expand) without misleading SQL claim. - Document Op.Where SQL-only-column constraint: keys must reference a bound column. Soft-delete timestamps, audit columns, tenant filters not in the proto cannot appear in Where. Framework's SoftDelete handling covers the most common case. - Tighten CodeUnimplemented bullet to flag it as defense-in-depth only (consistent with Roadmap and Algorithm step 9 wording). NIT - Single bindingsByProto lookup in Apply step 2 (was double). - Document encode/decode fd-nil checks as defense-in-depth — Validate proves these can't fire under normal codegen flow; the runtime guards are belt-and-suspenders against hand-edited generated files. - Cross-link v1.36.0 floor in Decision #6 (was only in rollout step 1). - Rollout step 9 says `make generate` (which runs sqlc generate + aippatchgen --check) instead of just "regenerate sqlc" — keeps aippatch in sync if a developer touches sql/queries.
Round 6: opus rated "Approve as-is" (first clean verdict). Sonnet found one MEDIUM about the wire-change scope. MEDIUM (Sonnet) - Wire-change claim was narrower than reality. Verified: b.UpdateDisplayName (internal/backend/user.go:17-32) returns ErrUserNotFound for any pgx.ErrNoRows (soft-delete OR PK mismatch), and the handler wraps it as Internal. So today everything → Internal. With aippatch all zero-row cases (soft-delete, PK mismatch, future scope filters) → NotFound; pgx errors stay Internal. The wire change is broader than just soft-delete. Updated Wire conformance note, Risks #6, and rollout plan step 7 to describe this accurately. LOW (Opus) - Note that op.Where allows non-writable bound columns (e.g. email for scoping a per-email UPDATE) — writability gates SET-clause emission, not WHERE-clause membership. - Add note that multiple-invalid-mask-paths returns the first error in client-supplied order; tests should assert on connect.CodeOf, consistent with drill's existing handler tests. LOW (Sonnet) - proto.CloneOf version footnote: dropped specific minor version ("v1.36.6") — exact added-in version is uncertain; spec now just says "available in the protobuf-go versions used here" with the go.mod version cited. NIT - Annotate slices.Contains in handler example with Go-version note (stdlib since 1.21; drill on 1.25).
Sonnet review (3 HIGH, 4 MEDIUM, 5 LOW, 4 NIT). Opus timed out; will re-run alongside sonnet for round 2 on the fixed plan. HIGH - Replace fake emptyMessage stub (didn't satisfy proto.Message) with *emptypb.Empty in Task 4's pre-fixture tests. Generic constraint now works without inventing a stub. - Add ALTER TABLE … RENAME COLUMN handling to sql.go (was missing — spec Algorithm step 2 explicitly requires it). pg_query_go represents this as Node_RenameStmt at the top level; added applyRename + a testdata/sql_rename/ fixture and TestLoadSchema_RenameColumn. - Drop Task 44's soft-delete wire test. Verified that GetAuthSessionByToken (sql/queries/auth_sessions.sql:6-16) joins on users.deleted_at IS NULL — a soft-deleted user can't authenticate at all, so the wire returns Unauthenticated, not NotFound. The runtime TestApply_SoftDeletedRowReturnsNotFound (Task 22) already covers the zero-row branch. Task 44 now just runs the existing handler tests. MEDIUM - Add empty-models guard + single-package check in run() — multi-package yaml is a v1+ extension; v0 returns a clear error. - Strengthen Algorithm step 4 (every proto field accounted for) to emit an explicit diagnostic for unmatched fields, with deduplication against earlier diagnostics. Added TestProcessResource_UnmatchedProtoField_Diagnostic. - Document mapping mutation safety in TestApply_UpdateAllWritable (mapping is unshared per-test; t.Parallel is safe). - Move sort BEFORE the comparison loop in checkOutputs so iteration order is actually stable (was dead code after the loop returned). LOW - Drop dead `var _ = errors.New` from errors.go; document that the helpers accept %w-style wrapping. - Add buf.yaml in fixturepb/ so buf can resolve google.protobuf.Timestamp; add `buf dep update` to the generation step. - Add `os` import to TestRun_FixtureRoundTrip (was missing). - Add yaml `go_package_path` field, eliminating the drill-only heuristic. Spanda projects MUST set it; e2e fixture and aippatch.yaml now do too. - Update aippatch.yaml example to include go_package_path explicitly. NIT - Fix "Task 46" reference in Task 2's Makefile comment (actual wiring is Task 38). - Update TestLoadYaml_Basic to assert GoPackagePath.
Both opus and sonnet "Revise" verdicts. Real correctness bugs in
round-1 patches.
HIGH (Opus)
- Delete dead `case pg_query.AlterTableType_AT_RenameColumn:` block
from applyAlterTable. That constant doesn't exist in pg_query_go/v6;
AlterTableCmd has no GetNewname method. Top-level Node_RenameStmt
handling in applyRename is the only correct path. Verified against
pganalyze/pg_query_go/v6 and libpg_query/17-latest.
HIGH (Sonnet)
- Fix enum constant naming in init.gen.go template. Previous template
emitted `int32(drillv1.USER_ROLE_CANDIDATE)` but protoc-gen-go names
the constant `drillv1.UserRole_USER_ROLE_CANDIDATE`. Renamed
EnumValue.Name -> EnumValue.GoConst, populated in processResource
with `<EnumName>_<VALUE_NAME>` form. Template now emits valid Go.
MEDIUM (both)
- Replace fragile substring dedup in step-4 unmatched-field loop with
an explicit `diagnosedFields map[string]bool` populated at every
diagnostic emission site. Substring scan would false-positive on
any field name that's a substring of another (e.g. "name" ⊂
"display_name").
LOW (Sonnet)
- Derive Go package alias from the proto file's go_package option's
`;<alias>` suffix, not from the proto package name. Old code mapped
`aippatch.fixture.v1` -> `aippatchfixturev1` which disagrees with
fixturepb's actual `package fixturepb` declaration. New goPackageAlias
helper reads FileDescriptor.Options() and parses the option's
`<importPath>;<alias>` form.
LOW (both)
- Drop `var _ = pgx.ErrNoRows` import-keepalive in apply.go and remove
the unused `pgx` import (DBTX uses pgx.Rows but only in types.go).
LOW (Opus)
- Rename `new` -> `newName` / `oldName` in applyRename to avoid
shadowing the Go builtin.
NIT
- Simplify filename construction (single Split instead of double
SplitAfter).
- Align unsupported-kinds diagnostic wording with spec's Diagnostics
section ("unsupported in v0; mark skip:true").
Round 3: opus "Approve as-is", sonnet "Revise" but with only LOW + NIT findings (technically should have been "Approve with minor revisions" per format spec). Both verdicts confirm round-2 fixes landed cleanly. Polish applied (all sonnet's findings): LOW - Task 28 buf build path was wrong (resolved relatively from a deep testdata directory). Reframe to run from repo root with the absolute output path. Eliminates a silent "descriptor not found" failure mode. - Add code comment to run() explaining the single-import constraint for emitInit (already guarded; just document the why for future multi-package work). NIT - snakeCase already lowercases, drop redundant strings.ToLower in filename construction. - Replace bespoke isUp helper with strings.HasSuffix in aippatchtest/db.go. - Drop the (non-existent) double-wrap concern noted by sonnet — encode returns plain fmt.Errorf, not a connect.Error, so wrapping with connectInvalidArg is correct. Skipped sonnet's emitInit-test-vs-Codecs note — informational, no runtime impact in v0 (single-package guard handles it). Plan is converged after 3 review rounds. Implementation-ready.
… stubbed) TestProcessResource_Happy is intentionally failing: compatibility() returns false for all field kinds until Task 32 fills it in.
…Int{32,64}Kind/Timestamp/EnumKind)
… dup codec, update_writable, unmatched field)
…rPatch Drop the manual protoreflect field-validation loop and hand-rolled UpdateDisplayName call; delegate field validation and the UPDATE to aippatch.Apply. Add patches.InitPatches() to the test TestMain so the Mapping is validated before the handler tests run.
Cover auto_set_conflict (column also a binding), auto_set_bad_literal (multi-statement expression), auto_set_column_not_found, and auto_set_column_nullable. The missing_column case was already covered by TestProcessResource_UnmatchedProtoField_Diagnostic.
Verify that Apply preserves unmapped proto fields from the input message via proto.CloneOf — ensuring fields not in the Mapping's Bindings pass through unchanged in the returned proto.
The map was built during auto_set validation but never read — the comment "populated for validation; not stored in model" made this look intentional, but the validation checks use columnOwners instead. Remove the variable and its writes entirely.
Adds connectUnimplemented alongside connectInternal/connectInvalidArg/ connectNotFound so all connect error constructors follow the same fmt.Errorf pattern. Updates Apply to use the new helper and drops the now-unused errors import.
processAll, processResource, and emitResource took large structs by value; gocritic hugeParam/rangeValCopy flags these. Switch all three to pointer receivers and update callers to use index-based range loops and address-of at call sites.
The previous parse-only check accepted multi-statement payloads (e.g. "1); DROP TABLE x; SELECT (1") and comma-expanded SET payloads (e.g. "NOW(), col = 'x'"). Inspect the parse tree: reject if stmt count != 1, target list length != 1, or the single target is a RowExpr. Add tests for both injection vectors.
Distinguish client-input errors from server-config errors in encode. Define errEnumValueOutOfMap sentinel; wrap enum-out-of-map with it in encode(); check errors.Is in Apply's SET-clause loop and map to InvalidArgument rather than Internal. Add TestApply_EnumOutOfMapReturnsInvalidArg to exercise the path.
After resolving the table, check that r.PK and r.SoftDelete (when non-empty) name actual columns. Emit diagnostics with a hint listing candidate column names. Tests cover missing pk and missing soft_delete.
Two codegen bugs fixed: 1. Discarded error from FindDescriptorByName — misspelled proto_enum now emits a diagnostic instead of shipping an empty codec. 2. yaml Map keys not matching any proto enum value name were silently dropped — now emit a diagnostic listing the unknown keys and valid names. Tests cover both paths.
Verify Apply respects the caller's transaction: rolled-back tx leaves the row unchanged; committed tx persists the update.
Add Apply function doc comment per spec (op.Message non-nil, returns CloneOf). Add inline field doc comments to Mapping, Binding, AutoSetClause, EnumCodec, and Op structs noting the ProtoEnum field is metadata-only.
…ation
Replace raw "{{.X}}" interpolation in both templates with {{q .X}} where
q = strconv.Quote. This prevents generated code from becoming invalid
if a field value contains a double-quote or backslash. Defensive for
current constrained values; guards against future yaml typos.
Switch connectInternal("...: %s", err.Error()) to %w so underlying
errors participate in errors.Is/As chains.
resolveImportPath takes yamlResource by pointer; range loops over models use index-based iteration to avoid rangeValCopy.
- .gitignore: ignore /aippatchgen binary (mirrors /drill, /drillctl).
- algorithm.go: pre-validate all declared codecs (proto_enum descriptor
existence, yaml-key vs proto-value-name matching) up-front instead of
gating on binding usage. Catches silent drift in unused codec yaml.
- main.go: replace `for i := range models[1:] { m := &models[1:][i] }`
with `for i := 1; i < len(models); i++ { m := &models[i] }` — same
backing array, easier to read.
- algorithm_test.go: fix misleading comment on
TestProcessResource_AutoSetMultiTargetLiteral_Diagnostic — the parser
produces a single RowExpr target, not two targets.
Round-2 review feedback (opus LOW + NIT items).
Round-2 review (opus) flagged that bindings referencing a broken codec produced two diagnostics for the same root cause — once from the processAll pre-validation pass, again from processResource's per-binding codec-tracking branch. Thread a brokenCodecs map[string]bool from processAll → processResource. The per-binding branch now skips emission entirely (continues) for codecs already flagged broken. Pre-validation guarantees a clean enum descriptor + unique ToText + matching yaml keys for codecs that aren't in brokenCodecs, so the per-binding lookup is now a straight cast. Tests: two assertions adjusted to match pre-validation wording (the per-binding wording is no longer reachable for these failure modes).
btc
force-pushed
the
worktree-aippatch-impl
branch
from
1cfc2aa to
057b1b0
Compare
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
Adds aippatch, a Go framework that turns AIP-134 PATCH RPCs into safe, dynamic Postgres
UPDATEstatements via codegen. The proto message andFieldMaskcarry intent on the wire; anaippatch.yamlplus a sibling code generator produce a typedMapping[T]per resource; a runtimeApply[T]call validates the mask, builds the SQL, executes it, and returns the post-update proto.thirdparty/aippatch/— generic overproto.Message, no drill imports, ready to lift into a standalone Go module. Codecs: scalars, timestamps, enums; AutoSet for always-set columns (updated_at = NOW()); soft-delete andOp.Wherescope predicates;pgx.Txparticipation. Spec divergence from AIP-134 documented (empty mask rejected withInvalidArgumentby default).thirdparty/aippatch/cmd/aippatchgen/— readsbuf.binpb+ SQL migrations (viapg_query_go/v6) +aippatch.yaml; emits committedinternal/patches/*.gen.go.--checkmode for CI drift detection.aippatch.yamldeclares theUserresource;internal/patches/{user,init}.gen.gois generated and committed;cmd/drill/main.gocallspatches.InitPatches()at startup;UpdateProfilehandler shrinks from ~45 lines to ~14 and usesaippatch.Apply; supersededUpdateUserDisplayNamesqlc query andb.UpdateDisplayNameremoved.The design and plan went through extensive review rounds before implementation; the implementation itself was executed task-by-task with two-stage review per task, then a final 3-round opus + sonnet review against the whole branch. Spec, plan, and review trail are all in
docs/superpowers/.Test plan
make testpasses (buf lint, codegen drift check, frontend, backend-race, golangci-lint).go test ./thirdparty/aippatch/... ./internal/rpc/user/... -racepasses.go run ./thirdparty/aippatch/cmd/aippatchgen --checkexits 0.TestUpdateProfile_*handler tests pass unchanged (they assert viaconnect.CodeOf(err), not message text).UpdateProfileagainst a running drill instance with a validdisplay_namepatch; confirm response carries the post-update User andupdated_atadvances.Notable behavior change
UpdateProfilenow distinguishes zero-row outcomes (PK mismatch, soft-delete, future scope filters) from real DB errors: zero-row →NotFound, pgx errors →Internal. Previously every failure path ofb.UpdateDisplayNamecollapsed toInternal. Documented in the spec's Wire conformance note.Roadmap
v1+ items not in this PR (deliberately deferred): JSONB codec, nullable bound columns, declarative validators, AIP-193 error mapping, ETag concurrency, audit-log hooks. Each is backwards-compatible with v0 call sites.