diff --git a/conformance/README.md b/conformance/README.md index bc36010..f10eab4 100644 --- a/conformance/README.md +++ b/conformance/README.md @@ -121,6 +121,7 @@ Every fixture names the rule it defends, in `why:`. The ones that catch real bug | `reconnect-prune` | pruning is gated on `synced`; a reconnect removes what vanished while away | | `partial-cycle-no-prune` | a cycle that never reaches `synced` prunes **nothing** | | `delete-recreate-uid` | identity is `uid`, never `name`; no state bleeds across a recreate | +| `adopt-created-dedup` | **I-IDEMPOTENT** — an optimistic `adoptSaved` and the watch echo of the create are one object by uid, not two | | `resync-midstream` | upstream continuity can be lost *without* the SSE connection dropping → a fresh cycle mid-stream | | `nested-field-removed` | `added`/`modified` **replace**; a deep-merge would resurrect a field the server deleted (a ghost) | | `status-follow-live` | `status` is read-only under the full projection: it follows the server live, and never becomes dirty, never conflicts, never enters a patch | diff --git a/conformance/bodies/cm-created.v1.yaml b/conformance/bodies/cm-created.v1.yaml new file mode 100644 index 0000000..4cabe01 --- /dev/null +++ b/conformance/bodies/cm-created.v1.yaml @@ -0,0 +1,12 @@ +# A ConfigMap that did not exist when the stream started: the host created it, the save returned this +# projected object, and the browser called adoptSaved on it. The watch echoes the SAME uid a beat +# later. New uid, new name — nothing about it collides with the baseline app-config. +apiVersion: v1 +kind: ConfigMap +metadata: + uid: cm-created-0007 + name: feature-flags + namespace: app + resourceVersion: "3001" +data: + beta: "true" diff --git a/conformance/fixtures/adopt-created-dedup.yaml b/conformance/fixtures/adopt-created-dedup.yaml new file mode 100644 index 0000000..37cbf74 --- /dev/null +++ b/conformance/fixtures/adopt-created-dedup.yaml @@ -0,0 +1,26 @@ +id: adopt-created-dedup +title: An optimistic create (adoptSaved) and the watch echo of it are the SAME object, by uid. +why: > + I-IDEMPOTENT. A create is host-owned: the store keys on metadata.uid and a pending create has none, + so staging it is the consumer's job (docs/client-state-model.md). Once the save returns the created + object the host may adoptSaved it, so the UI need not wait for the watch. The watch then echoes it as + `added` — and that echo must be a no-op, not a second card. Reconciled by uid, the optimistic insert + and its echo converge to one object, with nothing dirty and nothing to save. +suites: [client] +scope: { target: demo, version: v1, resource: configmaps, namespace: app } +projection: krm-full/v1 + +events: + - { type: reset } + - { type: added, body: cm-app.v1 } + - { type: synced } + - { type: added, body: cm-created.v1 } # the watch catches up to the create + +client: + edits: + - { after: 2, op: adopt, body: cm-created.v1 } # host adopts the save response BEFORE the echo + expect: + uids: [cm-app-0001, cm-created-0007] # two objects, not three — the echo deduped by uid + dirty: [] + conflicts: [] + patch: null diff --git a/conformance/gen/bodies.json b/conformance/gen/bodies.json index 22897c6..7ec49f2 100644 --- a/conformance/gen/bodies.json +++ b/conformance/gen/bodies.json @@ -111,6 +111,19 @@ "log-level": "info" } }, + "cm-created.v1": { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "uid": "cm-created-0007", + "name": "feature-flags", + "namespace": "app", + "resourceVersion": "3001" + }, + "data": { + "beta": "true" + } + }, "cm-flags.v1": { "apiVersion": "v1", "kind": "ConfigMap", diff --git a/conformance/gen/fixtures.json b/conformance/gen/fixtures.json index a504893..5ccc20b 100644 --- a/conformance/gen/fixtures.json +++ b/conformance/gen/fixtures.json @@ -1,4 +1,53 @@ [ + { + "id": "adopt-created-dedup", + "title": "An optimistic create (adoptSaved) and the watch echo of it are the SAME object, by uid.", + "why": "I-IDEMPOTENT. A create is host-owned: the store keys on metadata.uid and a pending create has none, so staging it is the consumer's job (docs/client-state-model.md). Once the save returns the created object the host may adoptSaved it, so the UI need not wait for the watch. The watch then echoes it as `added` — and that echo must be a no-op, not a second card. Reconciled by uid, the optimistic insert and its echo converge to one object, with nothing dirty and nothing to save.\n", + "suites": [ + "client" + ], + "scope": { + "target": "demo", + "version": "v1", + "resource": "configmaps", + "namespace": "app" + }, + "projection": "krm-full/v1", + "events": [ + { + "type": "reset" + }, + { + "type": "added", + "body": "cm-app.v1" + }, + { + "type": "synced" + }, + { + "type": "added", + "body": "cm-created.v1" + } + ], + "client": { + "edits": [ + { + "after": 2, + "op": "adopt", + "body": "cm-created.v1" + } + ], + "expect": { + "uids": [ + "cm-app-0001", + "cm-created-0007" + ], + "dirty": [], + "conflicts": [], + "patch": null + } + } + }, { "id": "array-atomic-on-change", "title": "When an array's length changes under an edit, the whole array conflicts — atomically.", diff --git a/docs/client-state-model.md b/docs/client-state-model.md index 622ff08..90f3d2d 100644 --- a/docs/client-state-model.md +++ b/docs/client-state-model.md @@ -57,6 +57,54 @@ Redacted paths are not placeholders and do not appear in the object. Render a wi `redactions(id)`, and do not offer an editor for it. The host save endpoint must also call [`gateway.ValidateMergePatch`](../gateway/patch.go) before writing to Kubernetes. +## Creating and deleting whole objects + +The store holds only objects with a server identity — a `metadata.uid` — whether the stream delivered +them or `adoptSaved` inserted one from a save response before its echo arrived. That limit is +deliberate, and it is why two operations do not live here: + +- A pending **create** has no server object, so no uid and no key. There is nothing to merge it + against; `changes()`, `patch()`, and `conflicts()` have no meaning for it. +- A **delete** has no fields to reconcile. + +Staging a pending create or delete is therefore the consumer's job, the same way the write itself is +(see [saving edits safely](saving.md)). Keep them in page-local state and render all three sources as +one review list under one Save: + +```ts +const pendingCreates = []; // client-only drafts keyed by a local draftId, never a uid: { draftId, name, data } +const pendingDeletes = new Set(); // uids marked for removal + +// One list, one Save: +// pendingCreates → "create " +// pendingDeletes → "delete " +// store.changes(uid) → field edits (skip a uid that is in pendingDeletes) +``` + +### Reflecting the result + +The recommended shape is still 204 and let the watch echo it (see [saving edits safely](saving.md)): a +create arrives as an `added` event, a delete as a `deleted` event, and the store converges on its own. +Two primitives exist for a host that cannot wait for the echo, and both are idempotent with it +(`I-IDEMPOTENT`): + +- `adoptSaved(object)` — insert the created object once the save returns it. The echo that follows is + a no-op, not a second card. +- `removeResource(uid)` — drop a deleted object before its `deleted` event arrives. + +Either way, clear the page-local entry — the `pendingCreates` draft or the `pendingDeletes` uid — when +its write succeeds. Those primitives update the store, not your pending lists; a completed mutation +left staged reappears in the review list and can be submitted twice. + +Two caveats keep this honest: + +- **A create reflects only _after_ the server responds.** `adoptSaved` needs the server-assigned uid + and a **projected** object (never a raw Kubernetes object — see [saving edits safely](saving.md)). Do + not fabricate a uid for a pending draft; keep it page-local until the create returns the object. +- **An optimistic delete is not self-healing.** A delete that _fails_ server-side produces no watch + event, so a `removeResource`d object does not reappear until the next snapshot. Re-add it on failure, + or skip the optimism and let the `deleted` echo do it. + ## Arrays and associative lists Arrays are atomic by default. A concurrent array change conflicts with a local array edit, which is diff --git a/docs/saving.md b/docs/saving.md index 6266c4b..3784933 100644 --- a/docs/saving.md +++ b/docs/saving.md @@ -79,3 +79,60 @@ intentionally incomplete, and a `PUT` can delete fields the browser never saw. `metadata.resourceVersion` may be stale when `krm-spec/v1` suppresses invisible status churn. Do not use the streamed value as a write precondition. The client-side three-way merge surfaces conflicts in the fields the user can see; send only the user's explicit merge-patch changes. + +## Creating and deleting whole objects + +A create and a delete are host writes exactly as a save is, and they stay host-side for the same +reasons: RBAC, attribution, and — for a create body — validation all live on the server. The client +stages the *intent*; your endpoint performs the *write*. See +[client state model](client-state-model.md#creating-and-deleting-whole-objects) for the client half — +the store keys on uid and has no merge for these, so the consumer aggregates staged create/delete with +`changes()` into one review list. + +```go +// POST /console/configmaps — create +func (s *server) createConfigMap(w http.ResponseWriter, r *http.Request) { + user := userFromSession(r) + scope := authorizedScope(user, r) + object := readObject(r) // the new object the browser assembled + + // Validate on the host, before the write — pin the GVK, the authorized scope and name, and an + // allowlist of the fields a browser may set. Never trust the assembled object as-is. + if err := validateCreate(object, scope); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + created, err := s.dynamicFor(user).Resource(configMaps).Namespace(scope.Namespace). + Create(r.Context(), object, metav1.CreateOptions{}) + if err != nil { + http.Error(w, "create failed", http.StatusBadGateway) + return + } + + // 204 and let the watch echo it — the same recommendation as save. To reflect it now instead, + // project it first and return it; the browser calls store.adoptSaved(projected). + _ = created + w.WriteHeader(http.StatusNoContent) +} + +// DELETE /console/configmaps/{name} — delete +func (s *server) deleteConfigMap(w http.ResponseWriter, r *http.Request) { + user := userFromSession(r) + scope := authorizedScope(user, r) + if err := s.dynamicFor(user).Resource(configMaps).Namespace(scope.Namespace). + Delete(r.Context(), scope.Name, metav1.DeleteOptions{}); err != nil { + http.Error(w, "delete failed", http.StatusBadGateway) + return + } + // 204; the `deleted` event prunes it from every open stream. To reflect it now instead, the + // browser calls store.removeResource(uid) with the uid it already tracks. + w.WriteHeader(http.StatusNoContent) +} +``` + +`ValidateMergePatch` guards a *patch*. A create sends a whole object, so validate it yourself before +the call — the `validateCreate` above stands in for a schema check or a field allowlist — and pass only +the sanitized object to `Create`. A projected or redacted field must no more ride in on a create body +than in a patch. API-server admission sits behind this as defense in depth, not as a substitute for the +host-side check. A delete carries no body to guard. diff --git a/packages/krm-stream/test/conformance.test.ts b/packages/krm-stream/test/conformance.test.ts index 56f8525..b6b179f 100644 --- a/packages/krm-stream/test/conformance.test.ts +++ b/packages/krm-stream/test/conformance.test.ts @@ -83,6 +83,10 @@ test("a snapshot cycle is reset … added* … synced", () => { test("client fixtures edit objects the stream actually delivered", () => { for (const f of clientFixtures()) { for (const edit of f.client?.edits ?? []) { + // `adopt` is the exception that proves the rule: it delivers an object the stream has NOT yet + // sent (the whole point is that its echo arrives later and must dedup by uid), and it carries a + // `body`, not a `uid`/`path`. The delivered-and-has-a-path checks are for edits, which it is not. + if (edit.op === "adopt") continue; const delivered = deliveredUidsBefore(f, edit.after); assert.ok( delivered.has(edit.uid), @@ -102,7 +106,7 @@ test("paths are segment arrays, never dot-joined strings", () => { ...(f.client?.expect?.absentPaths ?? []), ...(f.client?.expect?.readOnlyPaths ?? []), ...(f.client?.expect?.flashed ?? []), - ...(f.client?.edits ?? []).map((e) => e.path), + ...(f.client?.edits ?? []).filter((e) => e.op !== "adopt").map((e) => e.path), ]; for (const p of paths) { assert.ok(Array.isArray(p), `${f.id}: ${JSON.stringify(p)} must be a segment array`); diff --git a/packages/krm-stream/test/conformance.ts b/packages/krm-stream/test/conformance.ts index 240f893..07e598f 100644 --- a/packages/krm-stream/test/conformance.ts +++ b/packages/krm-stream/test/conformance.ts @@ -26,11 +26,15 @@ const CONFORMANCE = new URL("../../../conformance/", import.meta.url); * A fixture format that could not express that ordering could not test a three-way merge at all. */ export interface FixtureEdit { after: number; - op: "set" | "remove" | "addKey" | "renameKey" | "revert"; + op: "set" | "remove" | "addKey" | "renameKey" | "revert" | "adopt"; uid: string; path: Path; value?: unknown; newKey?: string; + /** `adopt` only: the bodies/ reference to hand `store.adoptSaved`. An adopt is not an edit to a + * delivered object — it IS a delivery, of the object a save returned — so it carries a `body`, not a + * `uid`/`path` like the other ops. */ + body?: string; } export interface FixtureExpect { diff --git a/packages/krm-stream/test/expect.ts b/packages/krm-stream/test/expect.ts index 2417bfa..8c24cb6 100644 --- a/packages/krm-stream/test/expect.ts +++ b/packages/krm-stream/test/expect.ts @@ -19,12 +19,18 @@ import assert from "node:assert/strict"; import type { LiveResourceStore } from "../src/index.ts"; import type { Path } from "../src/types.ts"; -import type { FixtureEdit, FixtureExpect } from "./conformance.ts"; +import { body, type FixtureEdit, type FixtureExpect } from "./conformance.ts"; /** `path` in a fixture edit always addresses the FIELD, not its container — even for the two ops * whose store signature takes the map plus a key. Keeping the fixture format uniform is worth the one * line of translation. */ export function applyEdit(store: LiveResourceStore, e: FixtureEdit): void { + // `adopt` is not an edit to a delivered object — it delivers one, the way a save response does. It + // carries a `body`, not a `uid`/`path`, so it returns before we touch either. + if (e.op === "adopt") { + store.adoptSaved(body(e.body!)); + return; + } const parent = e.path.slice(0, -1); const last = e.path[e.path.length - 1]; switch (e.op) {