From b4f3181aeff8e2bae9498442f2fb9d560ee9b6bb Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 00:48:03 -0400 Subject: [PATCH 1/2] Revision history: change sets, revisions, and revert (#48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The undo substrate. The agent is going to act freely — "empty the garlic bed and plant cucumbers" runs without a confirmation prompt — and that is only a defensible default if the result is easy to roll back. So undo lands before the agent write path, not after it. The unit of undo is the OPERATION, not the row. A change set groups the row-level revisions it produced; revert replays their inverses. Emptying a bed and replanting it touches one object and many plantings, and undoing half of that is worse than useless. Two properties shape the rest: Revert is itself a change set (reverts_id names its target), so history is append-only and an undo can be undone. git revert, not git reset. Revert is version-guarded per entity. Every snapshot carries the row's version; if something edited that row after the change set being reverted, restoring the old snapshot would silently discard the newer edit, so it is reported as a conflict and left alone while the rest of the change set still reverts. The auto-scope is what keeps this invasive but shallow. Service mutations call record(), which joins the change set on the context if one is open and otherwise opens a single-op one on the spot. REST handlers needed no edits at all and every UI mutation lands in history for free; only the agent wraps a whole turn. Multi-row operations (FillRegion, ClearObject) pass all their changes in one record call, so a 12-plop fill is one change set with 12 revisions and one undo. Revisions are buffered and written with their change set in a single transaction, so an operation that fails partway leaves no half-recorded history. Recording is best-effort after the fact: the row is already written, so failing the caller there would report a failure that didn't happen and invite a duplicate retry. A gap is logged loudly instead. Two sharp edges handled explicitly rather than by luck: Deleting an object cascades its plantings away at the FK level, where the service never sees them. deleteObjectRecording snapshots them first, or the delete would be listed in history and not actually be undoable. Restoring deleted rows reuses their ids, and a restored plop needs its object back first. planRevert runs three explicit passes — restore parents then children, then updates, then delete created children before their parents — instead of trusting reverse-seq ordering to happen to be right. Garden deletion stays out of scope, as designed: the cascade is invisible to the service and ON DELETE CASCADE would take the history with it. The history UI will say so rather than pretend otherwise. Closes #48 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 8 +- internal/api/api.go | 10 +- internal/api/history.go | 140 +++++ internal/api/history_test.go | 156 +++++ internal/domain/domain.go | 91 +++ internal/service/gardens.go | 16 +- internal/service/objects.go | 39 +- internal/service/ops.go | 43 +- internal/service/plantings.go | 33 +- internal/service/revisions.go | 572 +++++++++++++++++++ internal/service/revisions_test.go | 506 ++++++++++++++++ internal/store/migrations/0006_revisions.sql | 53 ++ internal/store/objects.go | 27 + internal/store/plantings.go | 31 + internal/store/revisions.go | 192 +++++++ 15 files changed, 1902 insertions(+), 15 deletions(-) create mode 100644 internal/api/history.go create mode 100644 internal/api/history_test.go create mode 100644 internal/service/revisions.go create mode 100644 internal/service/revisions_test.go create mode 100644 internal/store/migrations/0006_revisions.sql create mode 100644 internal/store/revisions.go diff --git a/DESIGN.md b/DESIGN.md index d1aebb0..cfd16aa 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -22,6 +22,7 @@ SQLite, centimeters everywhere (display-side imperial conversion only), `version - **garden_shares** — (garden_id, user_id) unique, role `viewer`|`editor`, created_by. Owner is implicit via `gardens.owner_id`, never a share row. - **garden_objects** — one polymorphic table: `kind` (`bed`|`grow_bag`|`container`|`in_ground`|`tree`|`path`|`structure`), name, `shape` (`rect`|`circle`; `polygon` + `points` JSON reserved in schema, not in the v1 editor), center `x_cm`,`y_cm` in garden space, `width_cm`,`height_cm` (circle: width=diameter), `rotation_deg`, `z_index`, `plantable` bool, nullable `color` override, `props` JSON for kind-specific extras (bed height, tree species…), notes. - **plants** — catalog. `owner_id NULL` = built-in (~30 seeded via migration, read-only, clone to customize). name, category (`vegetable`|`herb`|`flower`|`fruit`|`tree_shrub`|`cover`), `spacing_cm` (mature spacing), `color` hex, `icon` (emoji string — zero assets in v1), nullable `days_to_maturity`, notes. Users see built-ins + their own. +- **change_sets** / **revisions** — the undo substrate. A change set groups the row-level revisions one logical operation produced (`source` ui/agent/api, `summary`, nullable `agent_run_id`, nullable `reverts_id`); a revision is `(entity_type, entity_id, op, before, after)` with full JSON row snapshots. The unit of undo is the **operation, not the row**. Revert is itself a change set, so an undo can be undone; it is version-guarded per entity and reports conflicts rather than clobbering a later edit. Garden *deletion* is deliberately not revertible (the cascade is invisible to the service, and would take the history with it). - **plantings** ("plops") — object_id, plant_id, center `x_cm`,`y_cm` **in the parent object's local frame** (moving/rotating a bed moves its plants for free), `radius_cm` (a plop is a circular patch), nullable `count` (NULL = derived: `max(1, round(π·r² / spacing_cm²))`), nullable label, `planted_at`/`removed_at` dates. "Clear bed" sets `removed_at`; v1 shows rows where `removed_at IS NULL`. Year-over-year history and a future plan/scenario layer build on these dates without schema surgery. ## Geometry @@ -52,6 +53,8 @@ POST /auth/register | /auth/login | /auth/logout GET /auth/me GET /auth GET /auth/oidc/login GET /auth/oidc/callback (authorization-code + PKCE) GET,POST /gardens GET,PATCH,DELETE /gardens/:id GET /gardens/:id/full ← one-shot editor load: garden + objects + plantings + referenced plants +GET /gardens/:id/history ← change sets, newest first (paginated) +POST /change-sets/:id/revert ← undo an operation; 201, or 409 + the conflicts it skipped POST /gardens/:id/copy ← deep-copy a garden you own (objects + active plops; not shares/link) POST /gardens/:id/objects PATCH,DELETE /objects/:id POST /objects/:id/plantings PATCH,DELETE /plantings/:id @@ -76,7 +79,8 @@ internal/store/ sqlite.go (open/pragmas), migrate.go, migrations/*.sql internal/domain/ plain structs + sentinel errors (ErrNotFound, ErrForbidden, ErrVersionConflict) internal/service/ ★ THE SEAM. All permission checks + business ops. auth.go, gardens.go, - objects.go, plantings.go, plants.go, shares.go, ops.go (FillRegion etc.) + objects.go, plantings.go, plants.go, shares.go, ops.go (FillRegion etc.), + revisions.go (change-set scoping, recording, revert) internal/api/ thin gin handlers (decode → service → encode), session middleware, ginserver setup, routes.go, oidc.go, spa.go (embed fallback) internal/webdist/ dist.go: //go:embed all:dist (web build copied in by Makefile) @@ -85,6 +89,8 @@ web/ Vite app Makefile (cd web && npm run build) → copy dist → CGO_ENABLED=0 go build ``` +**Every mutation is recorded.** Service mutations call `record(...)`, which joins the change set on the context if one is open and otherwise opens a single-op one for that mutation alone. That auto-scope is what keeps undo shallow: REST handlers need no changes at all, and only the agent wraps a whole turn (`WithChangeSet`) so a multi-step turn undoes as one unit. A multi-row operation passes all its changes in one `record` call for the same reason. + **The service seam is the point.** Every operation is a method on `*service.Service` taking `(ctx, actorUserID, args)`; all ACL checks live there, not in handlers. REST handlers are thin adapters; future agent tools are equally thin `llm.DefineTool[Args]` adapters over the same methods — so agent calls inherit permission enforcement for free. Bulk ops designed for agents from the start, e.g. `FillRegion(ctx, actor, objectID, region, plantID, spacingOverride)` with `region` a rect/circle in object-local coordinates. ### Auth diff --git a/internal/api/api.go b/internal/api/api.go index 8fe83d8..3cfb26b 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -81,8 +81,9 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { gardens.GET("/:id", h.getGarden) gardens.PATCH("/:id", h.updateGarden) gardens.DELETE("/:id", h.deleteGarden) - gardens.POST("/:id/copy", h.copyGarden) // duplicate a garden the actor owns - gardens.GET("/:id/full", h.getGardenFull) // one-shot editor load + gardens.POST("/:id/copy", h.copyGarden) // duplicate a garden the actor owns + gardens.GET("/:id/full", h.getGardenFull) // one-shot editor load + gardens.GET("/:id/history", h.getGardenHistory) // change sets, newest first gardens.POST("/:id/objects", h.createObject) // Sharing (owner-managed; a recipient may remove their own share). @@ -111,6 +112,11 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { plantings.PATCH("/:id", h.updatePlanting) plantings.DELETE("/:id", h.deletePlanting) + // Undo. A change set is addressed by its own id; the service resolves the + // owning garden for the permission check, same as objects and plantings. + changeSets := v1.Group("/change-sets", h.requireAuth()) + changeSets.POST("/:id/revert", h.revertChangeSet) + // Plant catalog: built-ins (seeded, read-only) plus the actor's own rows. plants := v1.Group("/plants", h.requireAuth()) plants.GET("", h.listPlants) diff --git a/internal/api/history.go b/internal/api/history.go new file mode 100644 index 0000000..c700337 --- /dev/null +++ b/internal/api/history.go @@ -0,0 +1,140 @@ +package api + +import ( + "net/http" + "strconv" + + "github.com/gin-gonic/gin" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// Change history and undo (#48). Two endpoints: read a garden's change sets, and +// revert one. + +// historyResponse is the body of GET /gardens/:id/history. hasMore lets the +// client page without a separate count query — it asks for one row more than it +// shows and reports whether that row existed. +type historyResponse struct { + ChangeSets []changeSetView `json:"changeSets"` + HasMore bool `json:"hasMore"` +} + +// changeSetView is one history entry. It deliberately omits the revisions +// themselves: the list renders counts, and nothing in the UI needs the JSON +// snapshots. +type changeSetView struct { + ID int64 `json:"id"` + GardenID int64 `json:"gardenId"` + ActorID int64 `json:"actorId"` + ActorName string `json:"actorName"` + Source string `json:"source"` + Summary string `json:"summary"` + AgentRunID *string `json:"agentRunId,omitempty"` + RevertsID *int64 `json:"revertsId,omitempty"` + RevertedByID *int64 `json:"revertedById,omitempty"` + Counts []countView `json:"counts"` + CreatedAt string `json:"createdAt"` +} + +type countView struct { + EntityType string `json:"entityType"` + Op string `json:"op"` + N int `json:"n"` +} + +// revertResponse is the body of POST /change-sets/:id/revert, on both the clean +// (201) and partial (409) paths. Carrying both fields either way is what lets the +// UI say "2 of 3 changes undone; the north bed was edited since and was left +// alone" instead of a generic failure — a partial revert really did change +// things, and pretending otherwise would be a lie. +type revertResponse struct { + ChangeSet *changeSetView `json:"changeSet"` + Conflicts []conflictView `json:"conflicts"` +} + +type conflictView struct { + EntityType string `json:"entityType"` + EntityID int64 `json:"entityId"` + Reason string `json:"reason"` + Name string `json:"name,omitempty"` +} + +func (h *handlers) getGardenHistory(c *gin.Context) { + gardenID, ok := parseIDParam(c, "id") + if !ok { + return + } + // 0 means "the service's default"; it clamps the upper bound too. + limit := intQuery(c, "limit", 0) + offset := intQuery(c, "offset", 0) + + sets, hasMore, err := h.svc.GardenHistory(c.Request.Context(), mustActor(c).ID, gardenID, limit, offset) + if err != nil { + writeServiceError(c, err) + return + } + views := make([]changeSetView, 0, len(sets)) + for i := range sets { + views = append(views, *toChangeSetView(&sets[i])) + } + c.JSON(http.StatusOK, historyResponse{ChangeSets: views, HasMore: hasMore}) +} + +func (h *handlers) revertChangeSet(c *gin.Context) { + id, ok := parseIDParam(c, "id") + if !ok { + return + } + cs, conflicts, err := h.svc.RevertChangeSet(c.Request.Context(), mustActor(c).ID, id) + if err != nil { + writeServiceError(c, err) + return + } + body := revertResponse{ChangeSet: toChangeSetView(cs), Conflicts: toConflictViews(conflicts)} + if len(conflicts) > 0 { + // 409 even when part of the revert applied: something the caller asked for + // did not happen, and the body says exactly what. + c.JSON(http.StatusConflict, body) + return + } + c.JSON(http.StatusCreated, body) +} + +func toChangeSetView(cs *domain.ChangeSet) *changeSetView { + if cs == nil { + return nil + } + counts := make([]countView, 0, len(cs.Counts)) + for _, c := range cs.Counts { + counts = append(counts, countView{EntityType: c.EntityType, Op: c.Op, N: c.N}) + } + return &changeSetView{ + ID: cs.ID, GardenID: cs.GardenID, ActorID: cs.ActorID, ActorName: cs.ActorName, + Source: cs.Source, Summary: cs.Summary, AgentRunID: cs.AgentRunID, + RevertsID: cs.RevertsID, RevertedByID: cs.RevertedByID, + Counts: counts, CreatedAt: cs.CreatedAt, + } +} + +func toConflictViews(cs []domain.RevertConflict) []conflictView { + views := make([]conflictView, 0, len(cs)) + for _, c := range cs { + views = append(views, conflictView{EntityType: c.EntityType, EntityID: c.EntityID, Reason: c.Reason, Name: c.Name}) + } + return views +} + +// intQuery reads a non-negative integer query parameter, falling back to def on +// an absent or malformed value. +func intQuery(c *gin.Context, name string, def int) int { + raw := c.Query(name) + if raw == "" { + return def + } + v, err := strconv.Atoi(raw) + if err != nil || v < 0 { + return def + } + return v +} diff --git a/internal/api/history_test.go b/internal/api/history_test.go new file mode 100644 index 0000000..e1182fe --- /dev/null +++ b/internal/api/history_test.go @@ -0,0 +1,156 @@ +package api + +import ( + "net/http" + "strconv" + "testing" +) + +func historyPath(gardenID int64) string { + return "/api/v1/gardens/" + strconv.FormatInt(gardenID, 10) + "/history" +} + +func revertPath(changeSetID int64) string { + return "/api/v1/change-sets/" + strconv.FormatInt(changeSetID, 10) + "/revert" +} + +// TestHistoryAndRevertAPI walks the whole loop over HTTP: a mutation shows up in +// history without anyone asking for it, reverting it answers 201 with the new +// change set, and the change is actually gone from /full. +func TestHistoryAndRevertAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "hist@example.com") + gid := createGardenAPI(t, r, cookie, "G") + + w := doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{ + "kind": "bed", "name": "North Bed", "xCm": 100, "yCm": 100, "widthCm": 100, "heightCm": 100, + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String()) + } + objectID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + + // The create landed in history with no explicit change set anywhere. + w = doJSON(t, r, http.MethodGet, historyPath(gid), nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("history: status %d, body %s", w.Code, w.Body.String()) + } + body := decodeMap(t, w.Body.Bytes()) + sets, _ := body["changeSets"].([]any) + if len(sets) != 1 { + t.Fatalf("got %d change sets, want 1: %s", len(sets), w.Body.String()) + } + if body["hasMore"].(bool) { + t.Error("hasMore should be false for a single-entry history") + } + entry := sets[0].(map[string]any) + if entry["summary"] != "Added North Bed" { + t.Errorf("summary = %v", entry["summary"]) + } + if entry["source"] != "ui" || entry["actorName"] == "" { + t.Errorf("unexpected entry: %+v", entry) + } + counts, _ := entry["counts"].([]any) + if len(counts) != 1 { + t.Fatalf("counts = %+v", entry["counts"]) + } + + // Revert it: 201, and the new change set points back at the original. + csID := int64(entry["id"].(float64)) + w = doJSON(t, r, http.MethodPost, revertPath(csID), nil, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("revert: status %d, body %s", w.Code, w.Body.String()) + } + rev := decodeMap(t, w.Body.Bytes()) + cs := rev["changeSet"].(map[string]any) + if int64(cs["revertsId"].(float64)) != csID { + t.Errorf("revertsId = %v, want %d", cs["revertsId"], csID) + } + if conflicts, _ := rev["conflicts"].([]any); len(conflicts) != 0 { + t.Errorf("unexpected conflicts: %+v", conflicts) + } + + // The object is gone from the editor payload. + w = doJSON(t, r, http.MethodGet, fullPath(gid), nil, cookie) + full := decodeMap(t, w.Body.Bytes()) + if objects, _ := full["objects"].([]any); len(objects) != 0 { + t.Errorf("%d objects survived the revert", len(objects)) + } + _ = objectID +} + +// TestRevertConflictAPI: a partial revert answers 409 and names what it skipped, +// because "2 of 3 undone, the north bed was edited since" is the only useful +// thing to say — a bare failure would be a lie about what happened. +func TestRevertConflictAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "conflict@example.com") + gid := createGardenAPI(t, r, cookie, "G") + + w := doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{ + "kind": "bed", "name": "Bed", "xCm": 100, "yCm": 100, "widthCm": 100, "heightCm": 100, + }, cookie) + obj := decodeMap(t, w.Body.Bytes()) + objectID := int64(obj["id"].(float64)) + version := int64(obj["version"].(float64)) + + // Move it (change set #2), then edit it again (change set #3). + w = doJSON(t, r, http.MethodPatch, objectPath(objectID), map[string]any{ + "xCm": 300, "version": version, + }, cookie) + if w.Code != http.StatusOK { + t.Fatalf("move: status %d, body %s", w.Code, w.Body.String()) + } + version = int64(decodeMap(t, w.Body.Bytes())["version"].(float64)) + w = doJSON(t, r, http.MethodPatch, objectPath(objectID), map[string]any{ + "name": "Renamed", "version": version, + }, cookie) + if w.Code != http.StatusOK { + t.Fatalf("rename: status %d, body %s", w.Code, w.Body.String()) + } + + // Undoing the move now conflicts: the row changed after it. + w = doJSON(t, r, http.MethodGet, historyPath(gid), nil, cookie) + sets := decodeMap(t, w.Body.Bytes())["changeSets"].([]any) + moveID := int64(sets[1].(map[string]any)["id"].(float64)) // newest first: rename, move, create + + w = doJSON(t, r, http.MethodPost, revertPath(moveID), nil, cookie) + if w.Code != http.StatusConflict { + t.Fatalf("revert: status %d, want 409, body %s", w.Code, w.Body.String()) + } + conflicts := decodeMap(t, w.Body.Bytes())["conflicts"].([]any) + if len(conflicts) != 1 { + t.Fatalf("conflicts = %+v", conflicts) + } + c := conflicts[0].(map[string]any) + if c["reason"] != "changed" || c["name"] != "Renamed" { + t.Errorf("conflict = %+v", c) + } + + // The object was left exactly alone. + w = doJSON(t, r, http.MethodGet, fullPath(gid), nil, cookie) + objects := decodeMap(t, w.Body.Bytes())["objects"].([]any) + o := objects[0].(map[string]any) + if o["xCm"].(float64) != 300 || o["name"] != "Renamed" { + t.Errorf("conflicted object was modified: %+v", o) + } +} + +// TestHistoryRequiresAccess — an unauthenticated caller can't read history, and a +// stranger gets 404 rather than a hint that the garden exists. +func TestHistoryRequiresAccess(t *testing.T) { + r := authEngine(t, localCfg()) + owner := registerAndCookie(t, r, "owner@example.com") + gid := createGardenAPI(t, r, owner, "G") + stranger := registerAndCookie(t, r, "stranger@example.com") + + if w := doJSON(t, r, http.MethodGet, historyPath(gid), nil, nil); w.Code != http.StatusUnauthorized { + t.Errorf("anonymous history status = %d, want 401", w.Code) + } + if w := doJSON(t, r, http.MethodGet, historyPath(gid), nil, stranger); w.Code != http.StatusNotFound { + t.Errorf("stranger history status = %d, want 404", w.Code) + } + if w := doJSON(t, r, http.MethodPost, revertPath(1), nil, stranger); w.Code != http.StatusNotFound { + t.Errorf("stranger revert status = %d, want 404", w.Code) + } +} diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 1183185..b13ffbb 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -93,6 +93,97 @@ const ( CategoryFruit = "fruit" CategoryTreeShrub = "tree_shrub" CategoryCover = "cover" + + // Where a change set came from. SourceAgent is what the history UI badges + // differently, so an autonomous edit is never mistaken for a hand edit. + SourceUI = "ui" + SourceAgent = "agent" + SourceAPI = "api" + + // The entity types a revision can snapshot. Garden covers metadata edits + // only — garden creation and deletion are deliberately not revertible. + EntityGarden = "garden" + EntityObject = "object" + EntityPlanting = "planting" + + OpCreate = "create" + OpUpdate = "update" + OpDelete = "delete" +) + +// ChangeSet groups the revisions produced by one logical operation — the unit of +// undo. A revert is itself a change set (RevertsID names its target), so history +// is append-only and an undo can be undone. +type ChangeSet struct { + ID int64 `json:"id"` + GardenID int64 `json:"gardenId"` + ActorID int64 `json:"actorId"` + Source string `json:"source"` + Summary string `json:"summary"` + AgentRunID *string `json:"agentRunId,omitempty"` + RevertsID *int64 `json:"revertsId,omitempty"` + CreatedAt string `json:"createdAt"` + + // Computed by the service for the history list, never persisted. + ActorName string `json:"actorName,omitempty"` + // RevertedByID is the change set that reverted this one, if any — what lets + // the list mark an entry as already undone. + RevertedByID *int64 `json:"revertedById,omitempty"` + // Counts tallies the revisions by (entity type, op) so the list can say + // "3 plops added, 1 bed moved" without loading every revision. + Counts []ChangeCount `json:"counts,omitempty"` + // Revisions is populated only when a single change set is loaded in full. + Revisions []Revision `json:"revisions,omitempty"` +} + +// ChangeCount is one (entity type, op) tally within a change set. +type ChangeCount struct { + EntityType string `json:"entityType"` + Op string `json:"op"` + N int `json:"n"` +} + +// Revision is one row-level change within a change set. Before/After are JSON +// row snapshots (nil for create/delete respectively) and include the row's +// version, which the revert guard compares against. +type Revision struct { + ID int64 `json:"id"` + ChangeSetID int64 `json:"changeSetId"` + Seq int64 `json:"seq"` + EntityType string `json:"entityType"` + EntityID int64 `json:"entityId"` + Op string `json:"op"` + Before *string `json:"before,omitempty"` + After *string `json:"after,omitempty"` +} + +// RevertConflict reports one entity a revert deliberately left alone, because +// reverting it would have discarded a change made after the change set being +// reverted. The rest of the change set still reverts. +type RevertConflict struct { + EntityType string `json:"entityType"` + EntityID int64 `json:"entityId"` + // Reason is one of the ConflictReason* values below. + Reason string `json:"reason"` + // Name is a human label for the entity where one exists (an object's name), + // so the UI can say "the north bed" rather than "object 41". + Name string `json:"name,omitempty"` +} + +// Why a revert skipped an entity. +const ( + // ConflictChanged: the row was edited after the change set being reverted, so + // restoring the old snapshot would silently discard that edit. + ConflictChanged = "changed" + // ConflictMissing: the row is gone, so there is nothing to revert to. + ConflictMissing = "missing" + // ConflictExists: a row with that id exists again, so restoring the deleted + // snapshot would overwrite it. + ConflictExists = "exists" + // ConflictUnsupported: this kind of change has no inverse the revert knows how + // to apply. Reported rather than silently skipped, so a change recorded by a + // later feature that revert wasn't taught about is visible instead of quiet. + ConflictUnsupported = "unsupported" ) // User is a pansy account. It may have a local password, OIDC identity, or both. diff --git a/internal/service/gardens.go b/internal/service/gardens.go index 54ca7a1..0811e2f 100644 --- a/internal/service/gardens.go +++ b/internal/service/gardens.go @@ -140,7 +140,8 @@ func (s *Service) ListGardens(ctx context.Context, actorID int64) ([]domain.Gard // version mismatch it returns (current garden, ErrVersionConflict) so the handler // can return the fresh row for the client to rebase. func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in GardenInput, version int64) (*domain.Garden, error) { - if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil { + before, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner) + if err != nil { return nil, err } g, err := gardenFromInput(in, false) // no defaults: an update states every field @@ -153,7 +154,18 @@ func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in if updated != nil { updated.MyRole = roleOwner.String() // only the owner reaches here } - return updated, err + if err != nil { + return updated, err + } + // MyRole is computed, not stored; blank it in the snapshot so a revert can't + // write a role into a row that has no such column. + snap := *before + snap.MyRole = "" + after := *updated + after.MyRole = "" + s.record(ctx, gardenID, actorID, "Edited garden settings", + changeUpdate(domain.EntityGarden, gardenID, &snap, &after)) + return updated, nil } // CopyGarden duplicates a garden into a new one owned by the actor, carrying diff --git a/internal/service/objects.go b/internal/service/objects.go index d392bea..c2357ca 100644 --- a/internal/service/objects.go +++ b/internal/service/objects.go @@ -122,7 +122,23 @@ func (s *Service) CreateObject(ctx context.Context, actorID, gardenID int64, in if err := finalizeObject(o, g); err != nil { return nil, err } - return s.store.CreateObject(ctx, o) + created, err := s.store.CreateObject(ctx, o) + if err != nil { + return nil, err + } + s.record(ctx, gardenID, actorID, "Added "+objectLabel(created), + changeCreate(domain.EntityObject, created.ID, created)) + return created, nil +} + +// objectLabel names an object for a change-set summary: its own name when it has +// one, else its kind ("bed", "grow_bag"), so history reads as "Added bed" rather +// than "Added ". +func objectLabel(o *domain.GardenObject) string { + if o.Name != "" { + return o.Name + } + return o.Kind } // objectForRole loads an object and enforces that the actor holds at least min @@ -148,21 +164,36 @@ func (s *Service) UpdateObject(ctx context.Context, actorID, objectID int64, pat return nil, err } + before := *o // objectForRole returns the current row, and the patch mutates it applyObjectPatch(o, patch) if err := finalizeObject(o, g); err != nil { return nil, err } o.Version = version - return s.store.UpdateObject(ctx, o) + updated, err := s.store.UpdateObject(ctx, o) + if err != nil { + return updated, err // may carry the current row on a version conflict + } + s.record(ctx, g.ID, actorID, "Edited "+objectLabel(updated), + changeUpdate(domain.EntityObject, updated.ID, &before, updated)) + return updated, nil } // DeleteObject removes an object (its plantings cascade) from a garden the actor // can edit. func (s *Service) DeleteObject(ctx context.Context, actorID, objectID int64) error { - if _, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor); err != nil { + o, g, err := s.objectForRole(ctx, actorID, objectID, roleEditor) + if err != nil { return err } - return s.store.DeleteObject(ctx, objectID) + // deleteObjectRecording snapshots the plantings the FK is about to cascade + // away, so the delete is actually revertible rather than merely listed. + changes, err := s.deleteObjectRecording(ctx, o) + if err != nil { + return err + } + s.record(ctx, g.ID, actorID, "Deleted "+objectLabel(o), changes...) + return nil } // GardenFull returns the whole editor payload for a garden the actor can view. diff --git a/internal/service/ops.go b/internal/service/ops.go index 70e3f42..f4db23c 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -2,6 +2,7 @@ package service import ( "context" + "fmt" "math" "strings" @@ -151,6 +152,14 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde if err != nil { return nil, err } + // One record call with every plop, so a fill auto-scopes into ONE change set + // with N revisions — undoing a fill is one click, not N. + changes := make([]change, 0, len(created)) + for i := range created { + changes = append(changes, changeCreate(domain.EntityPlanting, created[i].ID, &created[i])) + } + s.record(ctx, o.GardenID, actorID, + fmt.Sprintf("Planted %d %s in %s", len(created), plant.Name, objectLabel(o)), changes...) for i := range created { created[i].DerivedCount = derivedCount(created[i].RadiusCM, spacing) } @@ -219,11 +228,41 @@ func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, // non-plantable after it was planted must still be clearable (you can always // remove existing plops, only not add new ones). func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int, error) { - if _, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor); err != nil { + o, g, err := s.objectForRole(ctx, actorID, objectID, roleEditor) + if err != nil { + return 0, err + } + // Snapshot the rows the bulk UPDATE is about to touch, since it reports only a + // count. Re-read afterwards for the after-images rather than synthesizing them, + // so the snapshot is what the database actually holds (version included). + before, err := s.store.ListActivePlantingsForObject(ctx, objectID) + if err != nil { return 0, err } today := s.now().UTC().Format(dateLayout) - return s.store.ClearObjectPlantings(ctx, objectID, today) + n, err := s.store.ClearObjectPlantings(ctx, objectID, today) + if err != nil || n == 0 { + return n, err + } + after, err := s.store.ListPlantingsForObject(ctx, objectID) + if err != nil { + return n, err + } + afterByID := make(map[int64]*domain.Planting, len(after)) + for i := range after { + afterByID[after[i].ID] = &after[i] + } + changes := make([]change, 0, len(before)) + for i := range before { + b := before[i] + a, ok := afterByID[b.ID] + if !ok { + continue // vanished between the two reads; nothing coherent to record + } + changes = append(changes, changeUpdate(domain.EntityPlanting, b.ID, &b, a)) + } + s.record(ctx, g.ID, actorID, fmt.Sprintf("Cleared %s (%d plantings)", objectLabel(o), n), changes...) + return n, nil } // DescribeResult is a structured summary of a garden for prompting an agent. diff --git a/internal/service/plantings.go b/internal/service/plantings.go index 84daf69..b1006b9 100644 --- a/internal/service/plantings.go +++ b/internal/service/plantings.go @@ -72,7 +72,7 @@ type PlantingPatch struct { // CreatePlanting places a plop in a plantable object the actor can edit. func (s *Service) CreatePlanting(ctx context.Context, actorID, objectID int64, in PlantingInput) (*domain.Planting, error) { - o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) + o, g, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { return nil, err } @@ -105,6 +105,8 @@ func (s *Service) CreatePlanting(ctx context.Context, actorID, objectID int64, i if err != nil { return nil, err } + s.record(ctx, g.ID, actorID, "Planted "+plant.Name+" in "+objectLabel(o), + changeCreate(domain.EntityPlanting, created.ID, created)) created.DerivedCount = derivedCount(created.RadiusCM, plant.SpacingCM) return created, nil } @@ -116,11 +118,12 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64, if err != nil { return nil, err // ErrNotFound } - o, _, err := s.objectForRole(ctx, actorID, pl.ObjectID, roleEditor) + o, g, err := s.objectForRole(ctx, actorID, pl.ObjectID, roleEditor) if err != nil { return nil, err } + before := *pl // GetPlanting returns the current row, and the patch mutates it originalPlantID := pl.PlantID applyPlantingPatch(pl, patch) // Fetch the plop's plant for the derived count. Only when the actor is @@ -155,10 +158,26 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64, } return updated, err } + s.record(ctx, g.ID, actorID, plantingEditSummary(&before, updated, plant, o), + changeUpdate(domain.EntityPlanting, updated.ID, &before, updated)) updated.DerivedCount = derivedCount(updated.RadiusCM, plant.SpacingCM) return updated, nil } +// plantingEditSummary describes a plop edit for the history list. Soft-removal +// ("clear bed", harvested) is the one edit worth naming specifically — it reads +// as a removal to the person who did it, not as an edit. +func plantingEditSummary(before, after *domain.Planting, plant *domain.Plant, o *domain.GardenObject) string { + switch { + case before.RemovedAt == nil && after.RemovedAt != nil: + return "Removed " + plant.Name + " from " + objectLabel(o) + case before.RemovedAt != nil && after.RemovedAt == nil: + return "Restored " + plant.Name + " in " + objectLabel(o) + default: + return "Edited " + plant.Name + " in " + objectLabel(o) + } +} + // DeletePlanting hard-deletes a plop in an object the actor can edit. (Soft // removal — "clear bed" / harvested — sets removed_at via UpdatePlanting.) func (s *Service) DeletePlanting(ctx context.Context, actorID, plantingID int64) error { @@ -166,10 +185,16 @@ func (s *Service) DeletePlanting(ctx context.Context, actorID, plantingID int64) if err != nil { return err } - if _, _, err := s.objectForRole(ctx, actorID, pl.ObjectID, roleEditor); err != nil { + o, g, err := s.objectForRole(ctx, actorID, pl.ObjectID, roleEditor) + if err != nil { return err } - return s.store.DeletePlanting(ctx, plantingID) + if err := s.store.DeletePlanting(ctx, plantingID); err != nil { + return err + } + s.record(ctx, g.ID, actorID, "Deleted a planting from "+objectLabel(o), + changeDelete(domain.EntityPlanting, pl.ID, pl)) + return nil } // visiblePlant loads a plant the actor may reference in a planting: a built-in or diff --git a/internal/service/revisions.go b/internal/service/revisions.go new file mode 100644 index 0000000..c163a3a --- /dev/null +++ b/internal/service/revisions.go @@ -0,0 +1,572 @@ +package service + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "sync" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// This file is pansy's undo substrate (#48). The agent acts freely — "empty the +// garlic bed and plant cucumbers" runs without a confirmation prompt — which is +// only defensible because the result is easy to roll back. +// +// The unit of undo is the OPERATION, not the row: a change set groups the +// row-level revisions it produced, and revert replays their inverses. Two +// properties shape everything here: +// +// - Revert is itself a change set (reverts_id names its target). History is +// append-only, so an undo can be undone. git revert, not git reset. +// - Revert is version-guarded per entity. If a row changed after the change set +// being reverted, restoring the old snapshot would silently discard that +// edit, so it is reported as a conflict and left alone — the rest of the +// change set still reverts. + +// maxHistoryPageSize caps a history page so a season of edits can't be pulled in +// one request. +const maxHistoryPageSize = 100 + +// defaultHistoryPageSize is the page size when a caller doesn't ask for one. +const defaultHistoryPageSize = 50 + +// changeSetKey is the context key carrying the ambient change scope. +type changeSetKey struct{} + +// changeScope accumulates the revisions of one logical operation. Revisions are +// buffered rather than written as they happen, so an operation that fails partway +// leaves no half-recorded change set behind — the whole thing lands, or none +// of it does. +type changeScope struct { + gardenID int64 + actorID int64 + source string + summary string + agentRunID *string + + mu sync.Mutex + revs []domain.Revision +} + +func (sc *changeScope) append(revs []domain.Revision) { + sc.mu.Lock() + defer sc.mu.Unlock() + sc.revs = append(sc.revs, revs...) +} + +func (sc *changeScope) taken() []domain.Revision { + sc.mu.Lock() + defer sc.mu.Unlock() + return sc.revs +} + +// scopeFrom returns the change scope on ctx, or nil when the caller opened none. +func scopeFrom(ctx context.Context) *changeScope { + sc, _ := ctx.Value(changeSetKey{}).(*changeScope) + return sc +} + +// change is one row-level mutation waiting to be recorded. before/after are the +// row structs themselves; they are marshalled to JSON snapshots at record time. +type change struct { + entityType string + entityID int64 + op string + before any + after any +} + +func changeCreate(entityType string, id int64, after any) change { + return change{entityType: entityType, entityID: id, op: domain.OpCreate, after: after} +} + +func changeUpdate(entityType string, id int64, before, after any) change { + return change{entityType: entityType, entityID: id, op: domain.OpUpdate, before: before, after: after} +} + +func changeDelete(entityType string, id int64, before any) change { + return change{entityType: entityType, entityID: id, op: domain.OpDelete, before: before} +} + +// ChangeSetOptions describes the change set WithChangeSet opens. +type ChangeSetOptions struct { + // Source is one of domain.SourceUI / SourceAgent / SourceAPI. The history UI + // badges agent changes differently, so an autonomous edit is never mistaken + // for a hand edit. + Source string + // Summary is the one-line description shown in the history list. + Summary string + // AgentRunID joins this change set back to the executus run that produced it. + AgentRunID *string +} + +// WithChangeSet runs fn with a change scope on the context, so every mutation fn +// performs lands in ONE change set — the agent's "a whole turn is one undo" +// guarantee. The change set is written only if fn succeeds; if fn changed +// nothing, none is written and (nil, nil) is returned. +// +// Requires editor role on the garden, checked up front so a scope is never opened +// for an actor who couldn't have mutated anything anyway. +func (s *Service) WithChangeSet(ctx context.Context, actorID, gardenID int64, opts ChangeSetOptions, fn func(context.Context) error) (*domain.ChangeSet, error) { + if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleEditor); err != nil { + return nil, err + } + if !validChangeSource(opts.Source) { + return nil, domain.ErrInvalidInput + } + sc := &changeScope{ + gardenID: gardenID, actorID: actorID, + source: opts.Source, summary: opts.Summary, agentRunID: opts.AgentRunID, + } + if err := fn(context.WithValue(ctx, changeSetKey{}, sc)); err != nil { + return nil, err + } + return s.commitScope(ctx, sc, nil) +} + +// commitScope writes a scope's buffered revisions as one change set. revertsID is +// set only by RevertChangeSet. A scope with no revisions writes nothing — an +// operation that changed nothing doesn't belong in history. +func (s *Service) commitScope(ctx context.Context, sc *changeScope, revertsID *int64) (*domain.ChangeSet, error) { + revs := sc.taken() + if len(revs) == 0 { + return nil, nil + } + return s.store.WriteChangeSet(ctx, &domain.ChangeSet{ + GardenID: sc.gardenID, + ActorID: sc.actorID, + Source: sc.source, + Summary: sc.summary, + AgentRunID: sc.agentRunID, + RevertsID: revertsID, + }, revs) +} + +func validChangeSource(s string) bool { + return s == domain.SourceUI || s == domain.SourceAgent || s == domain.SourceAPI +} + +// record files the changes a mutation just made. When the caller opened a scope +// (WithChangeSet) they join it; otherwise this operation becomes its own +// single-op change set on the spot — the AUTO-SCOPE path, which is what keeps +// this feature invasive but shallow: REST handlers need no edits at all and every +// UI mutation lands in history for free. Only the agent wraps a whole turn. +// +// Note a multi-row operation (FillRegion, ClearObject) passes all of its changes +// in ONE call, so it auto-scopes into one change set with N revisions rather than +// N change sets. +// +// Recording is best-effort by design: the row is already written, so failing the +// caller here would report a failure that didn't happen and invite a duplicate +// retry. A history gap is logged loudly instead. +func (s *Service) record(ctx context.Context, gardenID, actorID int64, summary string, changes ...change) { + if len(changes) == 0 { + return + } + revs, err := toRevisions(changes) + if err != nil { + slog.Error("service: snapshot revisions", "error", err, "garden", gardenID, "summary", summary) + return + } + if sc := scopeFrom(ctx); sc != nil { + sc.append(revs) + return + } + if _, err := s.store.WriteChangeSet(ctx, &domain.ChangeSet{ + GardenID: gardenID, ActorID: actorID, Source: domain.SourceUI, Summary: summary, + }, revs); err != nil { + slog.Error("service: record change set", "error", err, "garden", gardenID, "summary", summary) + } +} + +// toRevisions marshals each change's before/after rows to JSON snapshots. +func toRevisions(changes []change) ([]domain.Revision, error) { + revs := make([]domain.Revision, 0, len(changes)) + for _, c := range changes { + before, err := snapshot(c.before) + if err != nil { + return nil, err + } + after, err := snapshot(c.after) + if err != nil { + return nil, err + } + revs = append(revs, domain.Revision{ + EntityType: c.entityType, EntityID: c.entityID, Op: c.op, + Before: before, After: after, + }) + } + return revs, nil +} + +// snapshot marshals a row to a JSON string, preserving nil as a NULL column. +func snapshot(v any) (*string, error) { + if v == nil { + return nil, nil + } + b, err := json.Marshal(v) + if err != nil { + return nil, fmt.Errorf("service: marshal snapshot: %w", err) + } + s := string(b) + return &s, nil +} + +// GardenHistory returns a page of a garden's change sets, newest first, for an +// actor who can at least view it, plus whether more pages follow. limit is +// clamped to [1, maxHistoryPageSize]. +// +// hasMore is answered by reading one row beyond the page and discarding it, +// rather than by a second COUNT over a table that only grows. Keeping that here +// (not in the handler) means the clamp and the probe can't disagree. +func (s *Service) GardenHistory(ctx context.Context, actorID, gardenID int64, limit, offset int) ([]domain.ChangeSet, bool, error) { + if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer); err != nil { + return nil, false, err + } + if limit <= 0 { + limit = defaultHistoryPageSize + } + if limit > maxHistoryPageSize { + limit = maxHistoryPageSize + } + if offset < 0 { + offset = 0 + } + sets, err := s.store.ListChangeSets(ctx, gardenID, limit+1, offset) + if err != nil { + return nil, false, err + } + if len(sets) > limit { + return sets[:limit], true, nil + } + return sets, false, nil +} + +// RevertChangeSet undoes a change set by applying the inverse of each of its +// revisions, inside one NEW change set that points back at the target — so the +// undo is itself undoable. Requires editor role on the garden. +// +// Entities that changed after the target change set are reported as conflicts and +// left untouched; everything else still reverts. Returns the new change set (nil +// when nothing could be reverted) alongside those conflicts. +func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int64) (*domain.ChangeSet, []domain.RevertConflict, error) { + target, err := s.store.GetChangeSet(ctx, changeSetID) + if err != nil { + return nil, nil, err + } + if _, err := s.requireGardenRole(ctx, actorID, target.GardenID, roleEditor); err != nil { + return nil, nil, err + } + + conflicts := []domain.RevertConflict{} + sc := &changeScope{ + gardenID: target.GardenID, actorID: actorID, source: domain.SourceUI, + summary: revertSummary(target), + } + for _, r := range planRevert(target.Revisions) { + changes, conflict, err := s.applyInverse(ctx, r) + if err != nil { + return nil, nil, err + } + if conflict != nil { + conflicts = append(conflicts, *conflict) + continue + } + revs, err := toRevisions(changes) + if err != nil { + return nil, nil, err + } + sc.append(revs) + } + + cs, err := s.commitScope(ctx, sc, &target.ID) + if err != nil { + return nil, nil, err + } + return cs, conflicts, nil +} + +// planRevert orders a change set's revisions for inverse application. Three +// passes, because the inverses carry foreign-key dependencies the original +// operations did not: +// +// 1. restore deleted rows — parents (objects) BEFORE children (plantings), or a +// restored plop has no object to hang off and its FK rejects it; +// 2. undo updates; +// 3. delete created rows — children before parents, so a delete never leans on +// the cascade to clean up rows this revert is responsible for. +// +// Reverse seq order within each pass: the last change made is the first undone. +// The passes are explicit rather than relying on reverse seq happening to order +// parents correctly, which it only does by luck. +func planRevert(revs []domain.Revision) []domain.Revision { + rank := func(r domain.Revision) int { + switch { + case r.Op == domain.OpDelete && r.EntityType == domain.EntityObject: + return 0 + case r.Op == domain.OpDelete: + return 1 + case r.Op == domain.OpUpdate: + return 2 + case r.Op == domain.OpCreate && r.EntityType == domain.EntityObject: + return 4 + default: // create, non-object + return 3 + } + } + planned := make([]domain.Revision, 0, len(revs)) + for pass := 0; pass <= 4; pass++ { + for i := len(revs) - 1; i >= 0; i-- { + if rank(revs[i]) == pass { + planned = append(planned, revs[i]) + } + } + } + return planned +} + +// applyInverse undoes one revision, returning the changes it made (to be recorded +// in the revert's own change set), or a conflict describing why it declined. A +// returned error is an infrastructure failure and aborts the revert; a conflict +// is a normal outcome that skips just this entity. +func (s *Service) applyInverse(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { + switch r.EntityType { + case domain.EntityObject: + return s.revertObject(ctx, r) + case domain.EntityPlanting: + return s.revertPlanting(ctx, r) + case domain.EntityGarden: + return s.revertGarden(ctx, r) + default: + return nil, conflict(r, domain.ConflictUnsupported, ""), nil + } +} + +func (s *Service) revertObject(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { + switch r.Op { + case domain.OpCreate: + // Inverse of a create is a delete. + cur, err := s.store.GetObject(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + return nil, nil, nil // already gone: the inverse is a no-op, not a conflict + } + if err != nil { + return nil, nil, err + } + if c := versionGuard(r, cur.Version, cur.Name); c != nil { + return nil, c, nil + } + changes, err := s.deleteObjectRecording(ctx, cur) + return changes, nil, err + + case domain.OpUpdate: + cur, err := s.store.GetObject(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + return nil, conflict(r, domain.ConflictMissing, ""), nil + } + if err != nil { + return nil, nil, err + } + if c := versionGuard(r, cur.Version, cur.Name); c != nil { + return nil, c, nil + } + var target domain.GardenObject + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + target.Version = cur.Version + updated, err := s.store.UpdateObject(ctx, &target) + if errors.Is(err, domain.ErrVersionConflict) { + return nil, conflict(r, domain.ConflictChanged, cur.Name), nil + } + if err != nil { + return nil, nil, err + } + return []change{changeUpdate(domain.EntityObject, updated.ID, cur, updated)}, nil, nil + + case domain.OpDelete: + // Inverse of a delete is a restore, under the original id. + if existing, err := s.store.GetObject(ctx, r.EntityID); err == nil { + return nil, conflict(r, domain.ConflictExists, existing.Name), nil + } else if !errors.Is(err, domain.ErrNotFound) { + return nil, nil, err + } + var target domain.GardenObject + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + restored, err := s.store.RestoreObject(ctx, &target) + if err != nil { + return nil, nil, err + } + return []change{changeCreate(domain.EntityObject, restored.ID, restored)}, nil, nil + } + return nil, conflict(r, domain.ConflictUnsupported, ""), nil +} + +func (s *Service) revertPlanting(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { + switch r.Op { + case domain.OpCreate: + cur, err := s.store.GetPlanting(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + return nil, nil, nil // already gone + } + if err != nil { + return nil, nil, err + } + if c := versionGuard(r, cur.Version, ""); c != nil { + return nil, c, nil + } + if err := s.store.DeletePlanting(ctx, cur.ID); err != nil { + return nil, nil, err + } + return []change{changeDelete(domain.EntityPlanting, cur.ID, cur)}, nil, nil + + case domain.OpUpdate: + cur, err := s.store.GetPlanting(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + return nil, conflict(r, domain.ConflictMissing, ""), nil + } + if err != nil { + return nil, nil, err + } + if c := versionGuard(r, cur.Version, ""); c != nil { + return nil, c, nil + } + var target domain.Planting + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + target.Version = cur.Version + updated, err := s.store.UpdatePlanting(ctx, &target) + if errors.Is(err, domain.ErrVersionConflict) { + return nil, conflict(r, domain.ConflictChanged, ""), nil + } + if err != nil { + return nil, nil, err + } + return []change{changeUpdate(domain.EntityPlanting, updated.ID, cur, updated)}, nil, nil + + case domain.OpDelete: + if _, err := s.store.GetPlanting(ctx, r.EntityID); err == nil { + return nil, conflict(r, domain.ConflictExists, ""), nil + } else if !errors.Is(err, domain.ErrNotFound) { + return nil, nil, err + } + var target domain.Planting + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + restored, err := s.store.RestorePlanting(ctx, &target) + if err != nil { + return nil, nil, err + } + return []change{changeCreate(domain.EntityPlanting, restored.ID, restored)}, nil, nil + } + return nil, conflict(r, domain.ConflictUnsupported, ""), nil +} + +// revertGarden undoes a garden metadata edit. Garden creation and deletion are +// never recorded (see the migration), so update is the only op reachable here. +func (s *Service) revertGarden(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { + if r.Op != domain.OpUpdate { + return nil, conflict(r, domain.ConflictUnsupported, ""), nil + } + cur, err := s.store.GetGarden(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + return nil, conflict(r, domain.ConflictMissing, ""), nil + } + if err != nil { + return nil, nil, err + } + if c := versionGuard(r, cur.Version, cur.Name); c != nil { + return nil, c, nil + } + var target domain.Garden + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + target.Version = cur.Version + updated, err := s.store.UpdateGarden(ctx, &target) + if errors.Is(err, domain.ErrVersionConflict) { + return nil, conflict(r, domain.ConflictChanged, cur.Name), nil + } + if err != nil { + return nil, nil, err + } + return []change{changeUpdate(domain.EntityGarden, updated.ID, cur, updated)}, nil, nil +} + +// deleteObjectRecording deletes an object and returns every change to record. +// Deleting an object cascades its plantings away in SQLite without the service +// ever seeing them, so they are snapshotted first — otherwise the delete could be +// listed in history but never reverted. Shared by DeleteObject and the revert of +// an object creation, so both stay honest about the cascade. +func (s *Service) deleteObjectRecording(ctx context.Context, o *domain.GardenObject) ([]change, error) { + plops, err := s.store.ListPlantingsForObject(ctx, o.ID) + if err != nil { + return nil, err + } + if err := s.store.DeleteObject(ctx, o.ID); err != nil { + return nil, err + } + changes := make([]change, 0, len(plops)+1) + changes = append(changes, changeDelete(domain.EntityObject, o.ID, o)) + for i := range plops { + p := plops[i] + changes = append(changes, changeDelete(domain.EntityPlanting, p.ID, &p)) + } + return changes, nil +} + +// versionGuard reports a conflict when the row's current version differs from the +// one the change set left behind — i.e. something edited it since, and reverting +// would silently discard that edit. A revision with no after-snapshot (a delete) +// has nothing to compare and passes. +func versionGuard(r domain.Revision, currentVersion int64, name string) *domain.RevertConflict { + var after struct { + Version int64 `json:"version"` + } + if r.After == nil { + return nil + } + if err := json.Unmarshal([]byte(*r.After), &after); err != nil { + return conflict(r, domain.ConflictUnsupported, name) + } + if after.Version != currentVersion { + return conflict(r, domain.ConflictChanged, name) + } + return nil +} + +func conflict(r domain.Revision, reason, name string) *domain.RevertConflict { + return &domain.RevertConflict{EntityType: r.EntityType, EntityID: r.EntityID, Reason: reason, Name: name} +} + +// unsnapshot parses a JSON row snapshot back into a row struct. A missing +// snapshot where one is required is a corrupt revision, not a normal outcome. +func unsnapshot(s *string, into any) error { + if s == nil { + return fmt.Errorf("service: revision is missing the snapshot it needs") + } + if err := json.Unmarshal([]byte(*s), into); err != nil { + return fmt.Errorf("service: parse snapshot: %w", err) + } + return nil +} + +// revertSummary describes the revert in the history list. Reverting a revert +// reads as "Redid …" rather than a stack of nested "Undid" prefixes. +func revertSummary(target *domain.ChangeSet) string { + verb := "Undid" + if target.RevertsID != nil { + verb = "Redid" + } + if target.Summary == "" { + return verb + " an earlier change" + } + return verb + ": " + target.Summary +} diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go new file mode 100644 index 0000000..b29eb7e --- /dev/null +++ b/internal/service/revisions_test.go @@ -0,0 +1,506 @@ +package service + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// history returns a garden's change sets newest-first, failing the test on error. +func history(t *testing.T, s *Service, actor, gardenID int64) []domain.ChangeSet { + t.Helper() + sets, _, err := s.GardenHistory(context.Background(), actor, gardenID, 0, 0) + if err != nil { + t.Fatalf("GardenHistory: %v", err) + } + return sets +} + +// revisionsOf loads a change set's revisions. +func revisionsOf(t *testing.T, s *Service, id int64) []domain.Revision { + t.Helper() + cs, err := s.store.GetChangeSet(context.Background(), id) + if err != nil { + t.Fatalf("GetChangeSet: %v", err) + } + return cs.Revisions +} + +// TestAutoScopeRecordsEveryMutation is the acceptance criterion that keeps this +// feature shallow: a plain REST-shaped mutation, with nobody opening a change +// set, still lands in history. +func TestAutoScopeRecordsEveryMutation(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + bed := seedBed(t, s, owner, g.ID) + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil { + t.Fatalf("UpdateObject: %v", err) + } + + sets := history(t, s, owner, g.ID) + if len(sets) != 2 { + t.Fatalf("got %d change sets, want 2 (create + update)", len(sets)) + } + // Newest first. + if sets[0].Summary != "Edited North Bed" { + t.Errorf("newest summary = %q", sets[0].Summary) + } + if sets[0].Source != domain.SourceUI { + t.Errorf("source = %q, want ui", sets[0].Source) + } + if sets[0].ActorID != owner || sets[0].ActorName == "" { + t.Errorf("actor not resolved: %+v", sets[0]) + } + if len(sets[0].Counts) != 1 || sets[0].Counts[0].Op != domain.OpUpdate || sets[0].Counts[0].N != 1 { + t.Errorf("counts = %+v", sets[0].Counts) + } +} + +// TestFillRegionIsOneChangeSet: N plops, one undo. The whole reason the unit of +// undo is the operation and not the row. +func TestFillRegionIsOneChangeSet(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + created, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil) + if err != nil { + t.Fatalf("FillNamedRegion: %v", err) + } + if len(created) < 2 { + t.Fatalf("expected a multi-plop fill, got %d", len(created)) + } + + sets := history(t, s, owner, g.ID) + fill := sets[0] + revs := revisionsOf(t, s, fill.ID) + if len(revs) != len(created) { + t.Fatalf("fill produced %d revisions for %d plops", len(revs), len(created)) + } + for _, r := range revs { + if r.Op != domain.OpCreate || r.EntityType != domain.EntityPlanting { + t.Errorf("unexpected revision %+v", r) + } + if r.Before != nil || r.After == nil { + t.Errorf("a create should snapshot only the after state: %+v", r) + } + } + + // Reverting removes all N and leaves the bed as it was. + _, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID) + if err != nil { + t.Fatalf("RevertChangeSet: %v", err) + } + if len(conflicts) != 0 { + t.Fatalf("unexpected conflicts: %+v", conflicts) + } + active, err := s.store.ListActivePlantingsForObject(ctx, bed.ID) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(active) != 0 { + t.Errorf("%d plops survived the revert", len(active)) + } +} + +// TestRevertRestoresObjectGeometry covers the everyday case: move a bed, undo it. +func TestRevertRestoresObjectGeometry(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + moved, err := s.UpdateObject(ctx, owner, bed.ID, + ObjectPatch{XCM: f64Ptr(300), YCM: f64Ptr(400)}, bed.Version) + if err != nil { + t.Fatalf("UpdateObject: %v", err) + } + + sets := history(t, s, owner, g.ID) + if _, conflicts, err := s.RevertChangeSet(ctx, owner, sets[0].ID); err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + + back, err := s.store.GetObject(ctx, bed.ID) + if err != nil { + t.Fatalf("GetObject: %v", err) + } + if back.XCM != bed.XCM || back.YCM != bed.YCM { + t.Errorf("position = (%v,%v), want the original (%v,%v)", back.XCM, back.YCM, bed.XCM, bed.YCM) + } + // The revert is a WRITE, not a rewind: the row's version moves forward. + if back.Version <= moved.Version { + t.Errorf("version = %d, want > %d (revert is an append-only change)", back.Version, moved.Version) + } +} + +// TestRevertOfRevertRestoresTheChange — undo is undoable, because a revert is +// itself a change set rather than a rewrite of history. +func TestRevertOfRevertRestoresTheChange(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(300)}, bed.Version); err != nil { + t.Fatalf("UpdateObject: %v", err) + } + move := history(t, s, owner, g.ID)[0] + + undo, _, err := s.RevertChangeSet(ctx, owner, move.ID) + if err != nil { + t.Fatalf("revert: %v", err) + } + if undo.RevertsID == nil || *undo.RevertsID != move.ID { + t.Fatalf("revert doesn't point at its target: %+v", undo) + } + if o, _ := s.store.GetObject(ctx, bed.ID); o.XCM != bed.XCM { + t.Fatalf("undo didn't restore x: %v", o.XCM) + } + + redo, conflicts, err := s.RevertChangeSet(ctx, owner, undo.ID) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert of revert: err=%v conflicts=%+v", err, conflicts) + } + if o, _ := s.store.GetObject(ctx, bed.ID); o.XCM != 300 { + t.Errorf("redo didn't reapply the move: x = %v", o.XCM) + } + if redo.Summary == "" { + t.Error("a revert should carry a summary") + } + + // The original is marked as reverted, and every step is still in the list. + sets := history(t, s, owner, g.ID) + if len(sets) != 4 { // create bed, move, undo, redo + t.Fatalf("got %d change sets, want 4", len(sets)) + } + var moveEntry *domain.ChangeSet + for i := range sets { + if sets[i].ID == move.ID { + moveEntry = &sets[i] + } + } + if moveEntry == nil || moveEntry.RevertedByID == nil || *moveEntry.RevertedByID != undo.ID { + t.Errorf("the move isn't marked as reverted: %+v", moveEntry) + } +} + +// TestRevertConflictsOnLaterEdit is the guard that stops undo from silently +// discarding someone's newer work — and the promise that the REST of the change +// set still reverts. +func TestRevertConflictsOnLaterEdit(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + bedA := seedBed(t, s, owner, g.ID) + bedB, err := s.CreateObject(ctx, owner, g.ID, ObjectInput{ + Kind: domain.KindBed, Name: "B", XCM: 200, YCM: 200, WidthCM: 100, HeightCM: 100, + }) + if err != nil { + t.Fatalf("CreateObject: %v", err) + } + + // One change set touching both beds. + _, err = s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{Source: domain.SourceAgent, Summary: "Rearranged"}, + func(ctx context.Context) error { + if _, err := s.UpdateObject(ctx, owner, bedA.ID, ObjectPatch{XCM: f64Ptr(600)}, bedA.Version); err != nil { + return err + } + _, err := s.UpdateObject(ctx, owner, bedB.ID, ObjectPatch{XCM: f64Ptr(300)}, bedB.Version) + return err + }) + if err != nil { + t.Fatalf("WithChangeSet: %v", err) + } + turn := history(t, s, owner, g.ID)[0] + if turn.Source != domain.SourceAgent || len(revisionsOf(t, s, turn.ID)) != 2 { + t.Fatalf("expected one agent change set with 2 revisions: %+v", turn) + } + + // Someone edits bed A afterwards. + cur, _ := s.store.GetObject(ctx, bedA.ID) + if _, err := s.UpdateObject(ctx, owner, bedA.ID, ObjectPatch{Name: strPtr("Touched")}, cur.Version); err != nil { + t.Fatalf("later edit: %v", err) + } + + _, conflicts, err := s.RevertChangeSet(ctx, owner, turn.ID) + if err != nil { + t.Fatalf("RevertChangeSet: %v", err) + } + if len(conflicts) != 1 { + t.Fatalf("got %d conflicts, want 1: %+v", len(conflicts), conflicts) + } + if conflicts[0].EntityID != bedA.ID || conflicts[0].Reason != domain.ConflictChanged { + t.Errorf("unexpected conflict %+v", conflicts[0]) + } + if conflicts[0].Name != "Touched" { + t.Errorf("conflict should name the entity as it stands now, got %q", conflicts[0].Name) + } + + // A was left alone; B still reverted. + a, _ := s.store.GetObject(ctx, bedA.ID) + if a.XCM != 600 || a.Name != "Touched" { + t.Errorf("conflicted object was modified: %+v", a) + } + b, _ := s.store.GetObject(ctx, bedB.ID) + if b.XCM != bedB.XCM { + t.Errorf("bed B should have reverted to %v, got %v", bedB.XCM, b.XCM) + } +} + +// TestRevertRestoresDeletedObjectWithItsPlantings covers the cascade: deleting an +// object takes its plops with it at the FK level, where the service never sees +// them. If they aren't snapshotted the delete is listed in history but can't +// actually be undone. +func TestRevertRestoresDeletedObjectWithItsPlantings(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + plop, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 10, YCM: 10, RadiusCM: 20, + }) + if err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + if err := s.DeleteObject(ctx, owner, bed.ID); err != nil { + t.Fatalf("DeleteObject: %v", err) + } + + del := history(t, s, owner, g.ID)[0] + revs := revisionsOf(t, s, del.ID) + if len(revs) != 2 { + t.Fatalf("delete recorded %d revisions, want 2 (the bed and its plop)", len(revs)) + } + + if _, conflicts, err := s.RevertChangeSet(ctx, owner, del.ID); err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + + back, err := s.store.GetObject(ctx, bed.ID) + if err != nil { + t.Fatalf("bed not restored: %v", err) + } + if back.ID != bed.ID || back.Name != bed.Name || back.XCM != bed.XCM { + t.Errorf("restored bed differs: %+v", back) + } + restoredPlop, err := s.store.GetPlanting(ctx, plop.ID) + if err != nil { + t.Fatalf("plop not restored: %v", err) + } + if restoredPlop.ObjectID != bed.ID || restoredPlop.XCM != plop.XCM { + t.Errorf("restored plop differs: %+v", restoredPlop) + } +} + +// TestRevertClearObject: "clear bed" is a bulk soft-remove, and undoing it should +// bring the whole bed back planted. +func TestRevertClearObject(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + t.Fatalf("fill: %v", err) + } + before, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) + + n, err := s.ClearObject(ctx, owner, bed.ID) + if err != nil { + t.Fatalf("ClearObject: %v", err) + } + if n != len(before) { + t.Fatalf("cleared %d of %d", n, len(before)) + } + if active, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID); len(active) != 0 { + t.Fatalf("%d plops still active after clear", len(active)) + } + + clear := history(t, s, owner, g.ID)[0] + if _, conflicts, err := s.RevertChangeSet(ctx, owner, clear.ID); err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + after, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) + if len(after) != len(before) { + t.Errorf("%d plops active after undo, want %d", len(after), len(before)) + } +} + +// TestRevertPermissions: a viewer may read history but not undo; a stranger sees +// neither (existence stays masked). +func TestRevertPermissions(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + viewer := seedUser(t, s, "v@example.com") + stranger := seedUser(t, s, "s@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + if _, err := s.AddShare(ctx, owner, g.ID, "v@example.com", domain.RoleViewer); err != nil { + t.Fatalf("AddShare: %v", err) + } + _ = bed + cs := history(t, s, owner, g.ID)[0] + + if _, _, err := s.GardenHistory(ctx, viewer, g.ID, 0, 0); err != nil { + t.Errorf("a viewer should be able to read history: %v", err) + } + if _, _, err := s.RevertChangeSet(ctx, viewer, cs.ID); !errors.Is(err, domain.ErrForbidden) { + t.Errorf("viewer revert err = %v, want ErrForbidden", err) + } + if _, _, err := s.GardenHistory(ctx, stranger, g.ID, 0, 0); !errors.Is(err, domain.ErrNotFound) { + t.Errorf("stranger history err = %v, want ErrNotFound", err) + } + if _, _, err := s.RevertChangeSet(ctx, stranger, cs.ID); !errors.Is(err, domain.ErrNotFound) { + t.Errorf("stranger revert err = %v, want ErrNotFound", err) + } +} + +// TestWithChangeSetDiscardsOnFailure: history records operations that happened, +// not operations that were attempted. +func TestWithChangeSetDiscardsOnFailure(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + before := len(history(t, s, owner, g.ID)) + sentinel := errors.New("boom") + _, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{Source: domain.SourceAgent, Summary: "Half a turn"}, + func(ctx context.Context) error { + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(600)}, bed.Version); err != nil { + return err + } + return sentinel + }) + if !errors.Is(err, sentinel) { + t.Fatalf("err = %v, want the sentinel", err) + } + if got := len(history(t, s, owner, g.ID)); got != before { + t.Errorf("failed turn left %d change sets behind", got-before) + } +} + +// TestChangeSetSkippedWhenNothingChanged — a scope that mutates nothing writes no +// change set, so history isn't padded with empty entries. +func TestChangeSetSkippedWhenNothingChanged(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + before := len(history(t, s, owner, g.ID)) + cs, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{Source: domain.SourceAgent, Summary: "Looked around"}, + func(context.Context) error { return nil }) + if err != nil { + t.Fatalf("WithChangeSet: %v", err) + } + if cs != nil { + t.Errorf("expected no change set, got %+v", cs) + } + if got := len(history(t, s, owner, g.ID)); got != before { + t.Errorf("history grew by %d", got-before) + } +} + +// TestGardenMetadataRevert covers the third entity type. +func TestGardenMetadataRevert(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + if _, err := s.UpdateGarden(ctx, owner, g.ID, GardenInput{ + Name: "Renamed", WidthCM: 1200, HeightCM: 1000, UnitPref: domain.UnitImperial, + }, g.Version); err != nil { + t.Fatalf("UpdateGarden: %v", err) + } + cs := history(t, s, owner, g.ID)[0] + if _, conflicts, err := s.RevertChangeSet(ctx, owner, cs.ID); err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + back, _ := s.store.GetGarden(ctx, g.ID) + if back.Name != g.Name || back.WidthCM != g.WidthCM { + t.Errorf("garden not restored: %+v", back) + } + // MyRole is computed, never stored — the snapshot must not have carried it + // back into the row. + if back.MyRole != "" { + t.Errorf("MyRole leaked into the stored row: %q", back.MyRole) + } +} + +// TestPlanRevertOrdersRestoresParentFirst pins the ordering the FKs depend on, +// rather than trusting reverse-seq to get it right by luck. +func TestPlanRevertOrdersRestoresParentFirst(t *testing.T) { + revs := []domain.Revision{ + {Seq: 1, EntityType: domain.EntityPlanting, EntityID: 10, Op: domain.OpDelete}, + {Seq: 2, EntityType: domain.EntityObject, EntityID: 1, Op: domain.OpDelete}, + {Seq: 3, EntityType: domain.EntityObject, EntityID: 2, Op: domain.OpCreate}, + {Seq: 4, EntityType: domain.EntityPlanting, EntityID: 11, Op: domain.OpCreate}, + {Seq: 5, EntityType: domain.EntityObject, EntityID: 3, Op: domain.OpUpdate}, + } + got := planRevert(revs) + want := []struct { + entity string + id int64 + }{ + {domain.EntityObject, 1}, // restore parents first + {domain.EntityPlanting, 10}, // then children + {domain.EntityObject, 3}, // then updates + {domain.EntityPlanting, 11}, // then delete created children + {domain.EntityObject, 2}, // and only then their parents + } + if len(got) != len(want) { + t.Fatalf("planned %d revisions, want %d", len(got), len(want)) + } + for i, w := range want { + if got[i].EntityType != w.entity || got[i].EntityID != w.id { + t.Errorf("step %d = %s/%d, want %s/%d", i, got[i].EntityType, got[i].EntityID, w.entity, w.id) + } + } +} + +// TestSnapshotsCaptureVersion — the revert guard compares against the version in +// the after-snapshot, so it has to actually be there. +func TestSnapshotsCaptureVersion(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + + revs := revisionsOf(t, s, history(t, s, owner, g.ID)[0].ID) + if len(revs) != 1 || revs[0].After == nil { + t.Fatalf("unexpected revisions %+v", revs) + } + var snap domain.GardenObject + if err := json.Unmarshal([]byte(*revs[0].After), &snap); err != nil { + t.Fatalf("snapshot isn't a garden object: %v", err) + } + if snap.Version != bed.Version || snap.ID != bed.ID { + t.Errorf("snapshot = %+v, want version %d id %d", snap, bed.Version, bed.ID) + } +} + +func f64Ptr(v float64) *float64 { return &v } diff --git a/internal/store/migrations/0006_revisions.sql b/internal/store/migrations/0006_revisions.sql new file mode 100644 index 0000000..3b4912f --- /dev/null +++ b/internal/store/migrations/0006_revisions.sql @@ -0,0 +1,53 @@ +-- Revision history: the undo substrate (#48). +-- +-- The unit of undo is the OPERATION, not the row. "Empty the garlic bed and +-- plant cucumbers" touches one object and many plantings; undoing half of that +-- is worse than useless. So a change_set groups the row-level revisions it +-- produced, and revert replays their inverses. +-- +-- Two properties this schema is built around: +-- * Revert is itself a change set (reverts_id points at its target). History is +-- append-only and never rewritten, so an undo can be undone. git revert, not +-- git reset. +-- * Every revision carries full JSON row snapshots INCLUDING version, which is +-- what the revert guard compares against so a later edit is reported as a +-- conflict rather than silently clobbered. +-- +-- JSON snapshots rather than a shadow table per entity: the rows are small, +-- household scale makes storage a non-issue, and one code path covers all three +-- entity types. +-- +-- Deliberate gap: garden DELETION is not revertible. Dropping a garden cascades +-- its objects and plantings without the service ever seeing them, and the +-- ON DELETE CASCADE below would take the history with it. The history UI says so +-- rather than pretending otherwise. +CREATE TABLE change_sets ( + id INTEGER PRIMARY KEY, + garden_id INTEGER NOT NULL REFERENCES gardens (id) ON DELETE CASCADE, + actor_id INTEGER NOT NULL REFERENCES users (id) ON DELETE CASCADE, + source TEXT NOT NULL CHECK (source IN ('ui', 'agent', 'api')), + summary TEXT NOT NULL DEFAULT '', + agent_run_id TEXT, -- executus run id when source='agent' + reverts_id INTEGER REFERENCES change_sets (id), -- set when this change set reverts another + created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) +); + +-- The history panel's only query: one garden, newest first. +CREATE INDEX idx_change_sets_garden ON change_sets (garden_id, created_at DESC); + +-- "Has this change set been reverted?" — the list marks reverted entries, so the +-- lookup by target must not be a scan. +CREATE INDEX idx_change_sets_reverts ON change_sets (reverts_id) WHERE reverts_id IS NOT NULL; + +CREATE TABLE revisions ( + id INTEGER PRIMARY KEY, + change_set_id INTEGER NOT NULL REFERENCES change_sets (id) ON DELETE CASCADE, + seq INTEGER NOT NULL, -- order within the change set + entity_type TEXT NOT NULL CHECK (entity_type IN ('garden', 'object', 'planting')), + entity_id INTEGER NOT NULL, + op TEXT NOT NULL CHECK (op IN ('create', 'update', 'delete')), + before TEXT, -- JSON row snapshot; NULL for create + after TEXT -- JSON row snapshot; NULL for delete +); + +CREATE INDEX idx_revisions_change_set ON revisions (change_set_id, seq); diff --git a/internal/store/objects.go b/internal/store/objects.go index 03cf57b..da17ccf 100644 --- a/internal/store/objects.go +++ b/internal/store/objects.go @@ -61,6 +61,33 @@ func (d *DB) CreateObject(ctx context.Context, o *domain.GardenObject) (*domain. return created, nil } +// RestoreObject re-inserts a deleted object under its ORIGINAL id, preserving +// version and timestamps — the inverse of a delete, for reverting a change set +// (#48). SQLite allows an explicit INTEGER PRIMARY KEY insert once the row is +// gone; if the id has been taken again the insert fails on the primary key, which +// is the correct outcome (the service checks first and reports a conflict). +// +// Deliberately not routed through objectInsert: that omits id/version/timestamps +// because a normal create must let the database assign them. +func (d *DB) RestoreObject(ctx context.Context, o *domain.GardenObject) (*domain.GardenObject, error) { + restored, err := scanObject(d.sql.QueryRowContext(ctx, + `INSERT INTO garden_objects + (id, garden_id, kind, name, shape, points, x_cm, y_cm, width_cm, height_cm, + rotation_deg, z_index, plantable, color, props, grid_size_cm, snap_to_grid, notes, + version, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, + strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) + RETURNING `+objectColumns, + o.ID, o.GardenID, o.Kind, o.Name, o.Shape, o.Points, o.XCM, o.YCM, o.WidthCM, o.HeightCM, + o.RotationDeg, o.ZIndex, boolToInt(o.Plantable), o.Color, o.Props, + o.GridSizeCM, boolToInt(o.SnapToGrid), o.Notes, o.Version, o.CreatedAt, + )) + if err != nil { + return nil, fmt.Errorf("store: restore object: %w", err) + } + return restored, nil +} + // GetObject returns the object with the given id, or domain.ErrNotFound. func (d *DB) GetObject(ctx context.Context, id int64) (*domain.GardenObject, error) { o, err := scanObject(d.sql.QueryRowContext(ctx, diff --git a/internal/store/plantings.go b/internal/store/plantings.go index 1af1c9b..9a705ce 100644 --- a/internal/store/plantings.go +++ b/internal/store/plantings.go @@ -71,6 +71,37 @@ func (d *DB) ListActivePlantingsForObject(ctx context.Context, objectID int64) ( objectID) } +// ListPlantingsForObject returns every plop in an object, removed ones included. +// Used when an object is deleted: the FK cascades its plantings away without the +// service seeing them, so they are snapshotted first or the delete would not be +// revertible. Always a non-nil slice. +func (d *DB) ListPlantingsForObject(ctx context.Context, objectID int64) ([]domain.Planting, error) { + return queryPlantings(ctx, d.sql, + `SELECT `+plantingColumns+` FROM plantings WHERE object_id = ? ORDER BY id`, + objectID) +} + +// RestorePlanting re-inserts a deleted plop under its ORIGINAL id, preserving +// version and timestamps — the plop counterpart of RestoreObject, and subject to +// the same reasoning. Its parent object must exist again first, or the FK +// rejects it; the revert orders object restores ahead of planting restores. +func (d *DB) RestorePlanting(ctx context.Context, p *domain.Planting) (*domain.Planting, error) { + restored, err := scanPlanting(d.sql.QueryRowContext(ctx, + `INSERT INTO plantings + (id, object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at, removed_at, + version, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, + strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) + RETURNING `+plantingColumns, + p.ID, p.ObjectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, + p.PlantedAt, p.RemovedAt, p.Version, p.CreatedAt, + )) + if err != nil { + return nil, fmt.Errorf("store: restore planting: %w", err) + } + return restored, nil +} + // ClearObjectPlantings soft-removes every active plop in an object in one UPDATE // (sets removed_at=date, bumps version) and returns how many rows it affected. func (d *DB) ClearObjectPlantings(ctx context.Context, objectID int64, date string) (int, error) { diff --git a/internal/store/revisions.go b/internal/store/revisions.go new file mode 100644 index 0000000..1f788c2 --- /dev/null +++ b/internal/store/revisions.go @@ -0,0 +1,192 @@ +package store + +import ( + "context" + "database/sql" + "errors" + "fmt" + + "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" +) + +// changeSetColumns lists change_sets columns in the order scanChangeSet expects. +const changeSetColumns = `id, garden_id, actor_id, source, summary, agent_run_id, reverts_id, created_at` + +func scanChangeSet(s scanner) (*domain.ChangeSet, error) { + var cs domain.ChangeSet + if err := s.Scan( + &cs.ID, &cs.GardenID, &cs.ActorID, &cs.Source, &cs.Summary, + &cs.AgentRunID, &cs.RevertsID, &cs.CreatedAt, + ); err != nil { + return nil, err + } + return &cs, nil +} + +// revisionColumns lists revisions columns in the order scanRevision expects. +const revisionColumns = `id, change_set_id, seq, entity_type, entity_id, op, before, after` + +func scanRevision(s scanner) (*domain.Revision, error) { + var r domain.Revision + if err := s.Scan( + &r.ID, &r.ChangeSetID, &r.Seq, &r.EntityType, &r.EntityID, &r.Op, &r.Before, &r.After, + ); err != nil { + return nil, err + } + return &r, nil +} + +// WriteChangeSet inserts a change set and all of its revisions in one +// transaction, so history never records half an operation. The service buffers +// revisions while the operation runs and calls this once it has succeeded — +// which is also why an operation that fails partway leaves no change set behind. +// seq is assigned from the slice order. Returns the stored change set. +func (d *DB) WriteChangeSet(ctx context.Context, cs *domain.ChangeSet, revs []domain.Revision) (*domain.ChangeSet, error) { + tx, err := d.sql.BeginTx(ctx, nil) + if err != nil { + return nil, fmt.Errorf("store: begin change set: %w", err) + } + defer func() { _ = tx.Rollback() }() + + created, err := scanChangeSet(tx.QueryRowContext(ctx, + `INSERT INTO change_sets (garden_id, actor_id, source, summary, agent_run_id, reverts_id) + VALUES (?, ?, ?, ?, ?, ?) + RETURNING `+changeSetColumns, + cs.GardenID, cs.ActorID, cs.Source, cs.Summary, cs.AgentRunID, cs.RevertsID)) + if err != nil { + return nil, fmt.Errorf("store: insert change set: %w", err) + } + + for i := range revs { + r := &revs[i] + if _, err := tx.ExecContext(ctx, + `INSERT INTO revisions (change_set_id, seq, entity_type, entity_id, op, before, after) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + created.ID, int64(i+1), r.EntityType, r.EntityID, r.Op, r.Before, r.After, + ); err != nil { + return nil, fmt.Errorf("store: insert revision: %w", err) + } + } + + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("store: commit change set: %w", err) + } + return created, nil +} + +// ListChangeSets returns a garden's change sets newest-first, one page at a time. +// Each carries its actor's display name, the id of the change set that reverted +// it (if any), and per-(entity,op) counts — everything the history list renders, +// without a second round-trip per row. Always a non-nil slice. +func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset int) ([]domain.ChangeSet, error) { + rows, err := d.sql.QueryContext(ctx, + `SELECT `+qualifyColumns("cs", changeSetColumns)+`, u.display_name, rev.id + FROM change_sets cs + JOIN users u ON u.id = cs.actor_id + LEFT JOIN change_sets rev ON rev.reverts_id = cs.id + WHERE cs.garden_id = ? + ORDER BY cs.id DESC + LIMIT ? OFFSET ?`, + gardenID, limit, offset) + if err != nil { + return nil, fmt.Errorf("store: list change sets: %w", err) + } + defer rows.Close() + + sets := []domain.ChangeSet{} + ids := []any{} + byID := map[int64]int{} + for rows.Next() { + var cs domain.ChangeSet + if err := rows.Scan( + &cs.ID, &cs.GardenID, &cs.ActorID, &cs.Source, &cs.Summary, + &cs.AgentRunID, &cs.RevertsID, &cs.CreatedAt, &cs.ActorName, &cs.RevertedByID, + ); err != nil { + return nil, fmt.Errorf("store: scan change set: %w", err) + } + byID[cs.ID] = len(sets) + ids = append(ids, cs.ID) + sets = append(sets, cs) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("store: iterate change sets: %w", err) + } + if len(sets) == 0 { + return sets, nil + } + + // One grouped query for the whole page rather than N per-row counts. + countRows, err := d.sql.QueryContext(ctx, + `SELECT change_set_id, entity_type, op, COUNT(*) + FROM revisions + WHERE change_set_id IN (`+placeholders(len(ids))+`) + GROUP BY change_set_id, entity_type, op + ORDER BY change_set_id, entity_type, op`, + ids...) + if err != nil { + return nil, fmt.Errorf("store: count revisions: %w", err) + } + defer countRows.Close() + for countRows.Next() { + var csID int64 + var c domain.ChangeCount + if err := countRows.Scan(&csID, &c.EntityType, &c.Op, &c.N); err != nil { + return nil, fmt.Errorf("store: scan revision count: %w", err) + } + if i, ok := byID[csID]; ok { + sets[i].Counts = append(sets[i].Counts, c) + } + } + if err := countRows.Err(); err != nil { + return nil, fmt.Errorf("store: iterate revision counts: %w", err) + } + return sets, nil +} + +// GetChangeSet returns one change set with its revisions loaded in seq order, or +// domain.ErrNotFound. +func (d *DB) GetChangeSet(ctx context.Context, id int64) (*domain.ChangeSet, error) { + cs, err := scanChangeSet(d.sql.QueryRowContext(ctx, + `SELECT `+changeSetColumns+` FROM change_sets WHERE id = ?`, id)) + if errors.Is(err, sql.ErrNoRows) { + return nil, domain.ErrNotFound + } + if err != nil { + return nil, fmt.Errorf("store: get change set: %w", err) + } + + rows, err := d.sql.QueryContext(ctx, + `SELECT `+revisionColumns+` FROM revisions WHERE change_set_id = ? ORDER BY seq`, id) + if err != nil { + return nil, fmt.Errorf("store: list revisions: %w", err) + } + defer rows.Close() + + cs.Revisions = []domain.Revision{} + for rows.Next() { + r, err := scanRevision(rows) + if err != nil { + return nil, fmt.Errorf("store: scan revision: %w", err) + } + cs.Revisions = append(cs.Revisions, *r) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("store: iterate revisions: %w", err) + } + return cs, nil +} + +// placeholders returns "?, ?, …" for an IN clause of n values. +func placeholders(n int) string { + if n <= 0 { + return "NULL" + } + b := make([]byte, 0, n*3) + for i := 0; i < n; i++ { + if i > 0 { + b = append(b, ',', ' ') + } + b = append(b, '?') + } + return string(b) +} -- 2.54.0 From 2dd9f2f04fbbfaf764a3c9e4d54bbcc76ee5f1a4 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 01:06:43 -0400 Subject: [PATCH 2/2] Address Gadfly review on revision history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six real bugs, three of which would have broken the feature's central promise. The worst was that a failed operation threw away the history for changes that had already committed. WithChangeSet buffers HISTORY, not data — the store writes inside fn commit as they go — so discarding the buffer on error left real mutations with no change set and no way to undo them. That is exactly the situation undo exists for: an agent turn that did half a thing and then failed. Now the buffer is written, the summary says the operation failed partway, and the error is still returned. RevertChangeSet does the same for a revert that fails mid-loop. The partial state is still partial, but it is visible and undoable instead of orphaned. versionGuard falsely conflicted whenever one change set held more than one revision for the same entity — an agent turn that moves a bed and then renames it. Each inverse bumps the row's version, so from the second one on the snapshot's version no longer matched the live row and the revert flagged its own work as somebody else's edit. RevertChangeSet now tracks what it has written and the guard compares against that. ListChangeSets found "was this reverted?" with a LEFT JOIN, which emits one duplicate row per revert once a change set has been reverted more than once (undo, redo, undo again). Now a scalar subquery. Reverting an object creation cascade-deleted anything planted in that bed since, quietly. The plops were snapshotted so it was recoverable, but deleting someone's plants as a side effect of an unrelated undo should be reported. It now conflicts and leaves the bed alone. ClearObject cleared by predicate and snapshotted by a separate read, so a plop created between the two was removed with no revision to undo it by. It now clears exactly the ids it read. It also returned an error when only the post-clear re-read failed, which told the caller a clear had failed after it had already applied — inviting a retry of an applied operation. That path now logs the history gap and reports success, matching how record() treats its own write failures. RestoreObject/RestorePlanting could lose the race between the existence check and the insert and surface a raw constraint error. The revert now re-checks and reports ConflictExists, which is what that condition means. RevertChangeSet takes a source, so an agent undoing its own work is distinguishable from a person clicking undo — the whole point of the badge. Refactoring: the three revert bodies repeated a load/guard/unsnapshot/update skeleton (flagged by 3 of 5 models). They are now one generic revertEntity over a small per-type op table, so the ordering, guards and conflict reporting cannot drift apart. The API's view structs duplicated domain types that already carried every field, against the package's direct-JSON convention — dropped. intQuery moved next to the other request helpers. planRevert's comment said three passes where the code runs five. A revert where every revision is already a no-op now answers 200 rather than 201 with a null change set. Six new tests, one per bug. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/errors.go | 14 + internal/api/history.go | 122 +++------ internal/api/history_test.go | 3 - internal/service/ops.go | 23 +- internal/service/revisions.go | 400 +++++++++++++++++------------ internal/service/revisions_test.go | 232 +++++++++++++++-- internal/store/plantings.go | 21 +- internal/store/revisions.go | 8 +- 8 files changed, 543 insertions(+), 280 deletions(-) diff --git a/internal/api/errors.go b/internal/api/errors.go index 966c826..2a16c31 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -69,6 +69,20 @@ func writeVersionConflict(c *gin.Context, current any) { }) } +// intQuery reads a non-negative integer query parameter, falling back to def on +// an absent or malformed value. +func intQuery(c *gin.Context, name string, def int) int { + raw := c.Query(name) + if raw == "" { + return def + } + v, err := strconv.Atoi(raw) + if err != nil || v < 0 { + return def + } + return v +} + // parseIDParam reads a positive int64 path parameter, writing a 400 and // returning ok=false on a malformed value. func parseIDParam(c *gin.Context, name string) (int64, bool) { diff --git a/internal/api/history.go b/internal/api/history.go index c700337..de77f40 100644 --- a/internal/api/history.go +++ b/internal/api/history.go @@ -2,7 +2,6 @@ package api import ( "net/http" - "strconv" "github.com/gin-gonic/gin" @@ -10,54 +9,26 @@ import ( ) // Change history and undo (#48). Two endpoints: read a garden's change sets, and -// revert one. +// revert one. Both encode domain types directly, like the rest of the package — +// domain.ChangeSet already carries the actor name, revert linkage and per-op +// counts the history list renders, and its Revisions field is omitempty so the +// JSON snapshots never ride along on a list response. // historyResponse is the body of GET /gardens/:id/history. hasMore lets the -// client page without a separate count query — it asks for one row more than it -// shows and reports whether that row existed. +// client page without a separate count query over a table that only grows. type historyResponse struct { - ChangeSets []changeSetView `json:"changeSets"` - HasMore bool `json:"hasMore"` + ChangeSets []domain.ChangeSet `json:"changeSets"` + HasMore bool `json:"hasMore"` } -// changeSetView is one history entry. It deliberately omits the revisions -// themselves: the list renders counts, and nothing in the UI needs the JSON -// snapshots. -type changeSetView struct { - ID int64 `json:"id"` - GardenID int64 `json:"gardenId"` - ActorID int64 `json:"actorId"` - ActorName string `json:"actorName"` - Source string `json:"source"` - Summary string `json:"summary"` - AgentRunID *string `json:"agentRunId,omitempty"` - RevertsID *int64 `json:"revertsId,omitempty"` - RevertedByID *int64 `json:"revertedById,omitempty"` - Counts []countView `json:"counts"` - CreatedAt string `json:"createdAt"` -} - -type countView struct { - EntityType string `json:"entityType"` - Op string `json:"op"` - N int `json:"n"` -} - -// revertResponse is the body of POST /change-sets/:id/revert, on both the clean -// (201) and partial (409) paths. Carrying both fields either way is what lets the -// UI say "2 of 3 changes undone; the north bed was edited since and was left -// alone" instead of a generic failure — a partial revert really did change -// things, and pretending otherwise would be a lie. +// revertResponse is the body of POST /change-sets/:id/revert on every path. +// Carrying both fields regardless is what lets the UI say "2 of 3 changes undone; +// the north bed was edited since and was left alone" instead of a generic +// failure — a partial revert really did change things, and pretending otherwise +// would be a lie. ChangeSet is null when nothing needed reverting. type revertResponse struct { - ChangeSet *changeSetView `json:"changeSet"` - Conflicts []conflictView `json:"conflicts"` -} - -type conflictView struct { - EntityType string `json:"entityType"` - EntityID int64 `json:"entityId"` - Reason string `json:"reason"` - Name string `json:"name,omitempty"` + ChangeSet *domain.ChangeSet `json:"changeSet"` + Conflicts []domain.RevertConflict `json:"conflicts"` } func (h *handlers) getGardenHistory(c *gin.Context) { @@ -74,11 +45,7 @@ func (h *handlers) getGardenHistory(c *gin.Context) { writeServiceError(c, err) return } - views := make([]changeSetView, 0, len(sets)) - for i := range sets { - views = append(views, *toChangeSetView(&sets[i])) - } - c.JSON(http.StatusOK, historyResponse{ChangeSets: views, HasMore: hasMore}) + c.JSON(http.StatusOK, historyResponse{ChangeSets: sets, HasMore: hasMore}) } func (h *handlers) revertChangeSet(c *gin.Context) { @@ -86,55 +53,28 @@ func (h *handlers) revertChangeSet(c *gin.Context) { if !ok { return } - cs, conflicts, err := h.svc.RevertChangeSet(c.Request.Context(), mustActor(c).ID, id) + // A revert through the REST API is a person clicking undo. The agent reverts + // its own work through the service directly and stamps SourceAgent, so the + // history badge can tell the two apart. + cs, conflicts, err := h.svc.RevertChangeSet(c.Request.Context(), mustActor(c).ID, id, domain.SourceUI) if err != nil { writeServiceError(c, err) return } - body := revertResponse{ChangeSet: toChangeSetView(cs), Conflicts: toConflictViews(conflicts)} - if len(conflicts) > 0 { + if conflicts == nil { + conflicts = []domain.RevertConflict{} + } + body := revertResponse{ChangeSet: cs, Conflicts: conflicts} + switch { + case len(conflicts) > 0: // 409 even when part of the revert applied: something the caller asked for // did not happen, and the body says exactly what. c.JSON(http.StatusConflict, body) - return - } - c.JSON(http.StatusCreated, body) -} - -func toChangeSetView(cs *domain.ChangeSet) *changeSetView { - if cs == nil { - return nil - } - counts := make([]countView, 0, len(cs.Counts)) - for _, c := range cs.Counts { - counts = append(counts, countView{EntityType: c.EntityType, Op: c.Op, N: c.N}) - } - return &changeSetView{ - ID: cs.ID, GardenID: cs.GardenID, ActorID: cs.ActorID, ActorName: cs.ActorName, - Source: cs.Source, Summary: cs.Summary, AgentRunID: cs.AgentRunID, - RevertsID: cs.RevertsID, RevertedByID: cs.RevertedByID, - Counts: counts, CreatedAt: cs.CreatedAt, + case cs == nil: + // Every revision resolved to a no-op (already undone by hand, say). Nothing + // was created, so 200 rather than a 201 pointing at nothing. + c.JSON(http.StatusOK, body) + default: + c.JSON(http.StatusCreated, body) } } - -func toConflictViews(cs []domain.RevertConflict) []conflictView { - views := make([]conflictView, 0, len(cs)) - for _, c := range cs { - views = append(views, conflictView{EntityType: c.EntityType, EntityID: c.EntityID, Reason: c.Reason, Name: c.Name}) - } - return views -} - -// intQuery reads a non-negative integer query parameter, falling back to def on -// an absent or malformed value. -func intQuery(c *gin.Context, name string, def int) int { - raw := c.Query(name) - if raw == "" { - return def - } - v, err := strconv.Atoi(raw) - if err != nil || v < 0 { - return def - } - return v -} diff --git a/internal/api/history_test.go b/internal/api/history_test.go index e1182fe..f4cc616 100644 --- a/internal/api/history_test.go +++ b/internal/api/history_test.go @@ -28,8 +28,6 @@ func TestHistoryAndRevertAPI(t *testing.T) { if w.Code != http.StatusCreated { t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String()) } - objectID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) - // The create landed in history with no explicit change set anywhere. w = doJSON(t, r, http.MethodGet, historyPath(gid), nil, cookie) if w.Code != http.StatusOK { @@ -76,7 +74,6 @@ func TestHistoryAndRevertAPI(t *testing.T) { if objects, _ := full["objects"].([]any); len(objects) != 0 { t.Errorf("%d objects survived the revert", len(objects)) } - _ = objectID } // TestRevertConflictAPI: a partial revert answers 409 and names what it skipped, diff --git a/internal/service/ops.go b/internal/service/ops.go index f4db23c..9fd6d0a 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "log/slog" "math" "strings" @@ -233,20 +234,32 @@ func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int return 0, err } // Snapshot the rows the bulk UPDATE is about to touch, since it reports only a - // count. Re-read afterwards for the after-images rather than synthesizing them, - // so the snapshot is what the database actually holds (version included). + // count — then clear exactly those ids. Clearing "every active plop" instead + // would let a plop created between this read and the UPDATE be removed with no + // revision recorded: cleared, with no way to undo it. before, err := s.store.ListActivePlantingsForObject(ctx, objectID) if err != nil { return 0, err } + ids := make([]int64, 0, len(before)) + for i := range before { + ids = append(ids, before[i].ID) + } today := s.now().UTC().Format(dateLayout) - n, err := s.store.ClearObjectPlantings(ctx, objectID, today) + n, err := s.store.ClearObjectPlantings(ctx, objectID, today, ids) if err != nil || n == 0 { return n, err } + + // From here the clear HAS happened. A failure to build the history entry must + // not be reported as a failed clear — the caller would retry an operation that + // already applied. Log the gap and report success, matching how record() + // treats its own write failures. after, err := s.store.ListPlantingsForObject(ctx, objectID) if err != nil { - return n, err + slog.Error("service: clear succeeded but history could not be recorded", + "error", err, "object", objectID, "cleared", n) + return n, nil } afterByID := make(map[int64]*domain.Planting, len(after)) for i := range after { @@ -257,7 +270,7 @@ func (s *Service) ClearObject(ctx context.Context, actorID, objectID int64) (int b := before[i] a, ok := afterByID[b.ID] if !ok { - continue // vanished between the two reads; nothing coherent to record + continue // deleted outright between the two reads; nothing coherent to record } changes = append(changes, changeUpdate(domain.EntityPlanting, b.ID, &b, a)) } diff --git a/internal/service/revisions.go b/internal/service/revisions.go index c163a3a..d71afa2 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -122,11 +122,29 @@ func (s *Service) WithChangeSet(ctx context.Context, actorID, gardenID int64, op source: opts.Source, summary: opts.Summary, agentRunID: opts.AgentRunID, } if err := fn(context.WithValue(ctx, changeSetKey{}, sc)); err != nil { + // fn failed, but the mutations it completed before failing are already + // committed — this scope buffers history, not data. Dropping the buffer + // would leave those changes with no history and no way to undo them, + // which is exactly the situation undo exists for. Record what happened, + // mark the summary, and still report the failure. + sc.summary = partialSummary(sc.summary) + if _, cerr := s.commitScope(ctx, sc, nil); cerr != nil { + slog.Error("service: partial turn could not be recorded", "error", cerr, "garden", gardenID) + } return nil, err } return s.commitScope(ctx, sc, nil) } +// partialSummary marks a change set whose operation failed partway, so the +// history list can say so rather than presenting it as a completed action. +func partialSummary(summary string) string { + if summary == "" { + return "Partly applied (the operation failed partway)" + } + return summary + " (failed partway)" +} + // commitScope writes a scope's buffered revisions as one change set. revertsID is // set only by RevertChangeSet. A scope with no revisions writes nothing — an // operation that changed nothing doesn't belong in history. @@ -247,12 +265,20 @@ func (s *Service) GardenHistory(ctx context.Context, actorID, gardenID int64, li // RevertChangeSet undoes a change set by applying the inverse of each of its // revisions, inside one NEW change set that points back at the target — so the -// undo is itself undoable. Requires editor role on the garden. +// undo is itself undoable. Requires editor role on the garden. source says who +// asked (the agent can undo its own work, and the history badge should show that). // // Entities that changed after the target change set are reported as conflicts and // left untouched; everything else still reverts. Returns the new change set (nil -// when nothing could be reverted) alongside those conflicts. -func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int64) (*domain.ChangeSet, []domain.RevertConflict, error) { +// when nothing needed reverting) alongside those conflicts. +// +// The inverses are applied one row at a time rather than in a single +// transaction, because the store's version-guarded update path is per-row. That +// means a failure partway leaves the garden partly reverted — so whatever DID +// apply is recorded before the error is returned, and the partial revert is +// itself a change set you can undo. Losing the history for changes that really +// happened would be strictly worse than the partial state. +func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int64, source string) (*domain.ChangeSet, []domain.RevertConflict, error) { target, err := s.store.GetChangeSet(ctx, changeSetID) if err != nil { return nil, nil, err @@ -260,15 +286,30 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6 if _, err := s.requireGardenRole(ctx, actorID, target.GardenID, roleEditor); err != nil { return nil, nil, err } + if !validChangeSource(source) { + return nil, nil, domain.ErrInvalidInput + } conflicts := []domain.RevertConflict{} sc := &changeScope{ - gardenID: target.GardenID, actorID: actorID, source: domain.SourceUI, + gardenID: target.GardenID, actorID: actorID, source: source, summary: revertSummary(target), } + // A change set may hold several revisions for the SAME entity (an agent turn + // that moves a bed and then renames it). Each inverse bumps that row's + // version, so from the second one on, the version in the snapshot no longer + // matches the live row — and a naive guard would call our own work a conflict. + // This tracks what we just wrote so the guard compares against reality. + applied := map[entityKey]int64{} + for _, r := range planRevert(target.Revisions) { - changes, conflict, err := s.applyInverse(ctx, r) + changes, conflict, err := s.applyInverse(ctx, r, applied) if err != nil { + // Record what actually landed before surfacing the failure, so the + // partial revert is visible and undoable rather than orphaned. + if _, cerr := s.commitScope(ctx, sc, &target.ID); cerr != nil { + slog.Error("service: partial revert could not be recorded", "error", cerr, "changeSet", changeSetID) + } return nil, nil, err } if conflict != nil { @@ -289,15 +330,27 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6 return cs, conflicts, nil } -// planRevert orders a change set's revisions for inverse application. Three -// passes, because the inverses carry foreign-key dependencies the original -// operations did not: +// entityKey identifies a row across the entity types a revision can name. +type entityKey struct { + entityType string + id int64 +} + +func keyOf(r domain.Revision) entityKey { + return entityKey{entityType: r.EntityType, id: r.EntityID} +} + +// planRevert orders a change set's revisions for inverse application, because the +// inverses carry foreign-key dependencies the original operations did not. Five +// passes, in this order: // -// 1. restore deleted rows — parents (objects) BEFORE children (plantings), or a -// restored plop has no object to hang off and its FK rejects it; +// 0. restore deleted OBJECTS — parents first, or a restored plop has no object +// to hang off and its FK rejects it; +// 1. restore everything else deleted; // 2. undo updates; -// 3. delete created rows — children before parents, so a delete never leans on -// the cascade to clean up rows this revert is responsible for. +// 3. delete created non-objects (plantings) — children before parents; +// 4. delete created OBJECTS last, so a delete never leans on the cascade to +// clean up rows this revert is itself responsible for. // // Reverse seq order within each pass: the last change made is the first undone. // The passes are explicit rather than relying on reverse seq happening to order @@ -332,172 +385,192 @@ func planRevert(revs []domain.Revision) []domain.Revision { // in the revert's own change set), or a conflict describing why it declined. A // returned error is an infrastructure failure and aborts the revert; a conflict // is a normal outcome that skips just this entity. -func (s *Service) applyInverse(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { +// +// applied carries the versions this revert has already written, so a change set +// holding several revisions for one entity doesn't mistake its own earlier work +// for someone else's edit. +func (s *Service) applyInverse(ctx context.Context, r domain.Revision, applied map[entityKey]int64) ([]change, *domain.RevertConflict, error) { switch r.EntityType { case domain.EntityObject: - return s.revertObject(ctx, r) + return revertEntity(ctx, s, r, applied, objectRevertOps(s)) case domain.EntityPlanting: - return s.revertPlanting(ctx, r) + return revertEntity(ctx, s, r, applied, plantingRevertOps(s)) case domain.EntityGarden: - return s.revertGarden(ctx, r) + return revertEntity(ctx, s, r, applied, gardenRevertOps(s)) default: return nil, conflict(r, domain.ConflictUnsupported, ""), nil } } -func (s *Service) revertObject(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { - switch r.Op { - case domain.OpCreate: - // Inverse of a create is a delete. - cur, err := s.store.GetObject(ctx, r.EntityID) - if errors.Is(err, domain.ErrNotFound) { +// revertOps is everything reverting one entity type needs. The three +// implementations differ only in which store methods they call and how a row +// names itself; the ordering, guards and conflict reporting are shared below so +// they cannot drift apart. +type revertOps[T any] struct { + entityType string + get func(ctx context.Context, id int64) (*T, error) + update func(ctx context.Context, row *T) (*T, error) + // restore re-inserts a deleted row under its original id. nil means this + // entity type is never deleted (gardens), and a delete revision for it is + // reported as unsupported rather than silently skipped. + restore func(ctx context.Context, row *T) (*T, error) + // remove undoes a creation. It returns the changes it made, because deleting + // an object also deletes the plantings hanging off it. + remove func(ctx context.Context, row *T) ([]change, error) + // blockRemoval optionally refuses a removal, e.g. an object that has gained + // plantings since. Returns a conflict reason, or "" to allow it. + blockRemoval func(ctx context.Context, row *T) (string, error) + version func(row *T) int64 + setVersion func(row *T, v int64) + label func(row *T) string +} + +// revertEntity applies the inverse of one revision for one entity type. +func revertEntity[T any]( + ctx context.Context, s *Service, r domain.Revision, applied map[entityKey]int64, ops revertOps[T], +) ([]change, *domain.RevertConflict, error) { + key := keyOf(r) + + // Inverse of a delete is a restore, under the original id. + if r.Op == domain.OpDelete { + if ops.restore == nil { + return nil, conflict(r, domain.ConflictUnsupported, ""), nil + } + if existing, err := ops.get(ctx, r.EntityID); err == nil { + return nil, conflict(r, domain.ConflictExists, ops.label(existing)), nil + } else if !errors.Is(err, domain.ErrNotFound) { + return nil, nil, err + } + var target T + if err := unsnapshot(r.Before, &target); err != nil { + return nil, nil, err + } + restored, err := ops.restore(ctx, &target) + if err != nil { + // The id may have been taken between the check above and this insert. + // Re-check rather than surfacing a driver-specific constraint error: + // "someone else has that id now" is a conflict, not a failure. + if existing, gerr := ops.get(ctx, r.EntityID); gerr == nil { + return nil, conflict(r, domain.ConflictExists, ops.label(existing)), nil + } + return nil, nil, err + } + applied[key] = ops.version(restored) + return []change{changeCreate(ops.entityType, r.EntityID, restored)}, nil, nil + } + + cur, err := ops.get(ctx, r.EntityID) + if errors.Is(err, domain.ErrNotFound) { + if r.Op == domain.OpCreate { return nil, nil, nil // already gone: the inverse is a no-op, not a conflict } - if err != nil { - return nil, nil, err - } - if c := versionGuard(r, cur.Version, cur.Name); c != nil { - return nil, c, nil - } - changes, err := s.deleteObjectRecording(ctx, cur) - return changes, nil, err - - case domain.OpUpdate: - cur, err := s.store.GetObject(ctx, r.EntityID) - if errors.Is(err, domain.ErrNotFound) { - return nil, conflict(r, domain.ConflictMissing, ""), nil - } - if err != nil { - return nil, nil, err - } - if c := versionGuard(r, cur.Version, cur.Name); c != nil { - return nil, c, nil - } - var target domain.GardenObject - if err := unsnapshot(r.Before, &target); err != nil { - return nil, nil, err - } - target.Version = cur.Version - updated, err := s.store.UpdateObject(ctx, &target) - if errors.Is(err, domain.ErrVersionConflict) { - return nil, conflict(r, domain.ConflictChanged, cur.Name), nil - } - if err != nil { - return nil, nil, err - } - return []change{changeUpdate(domain.EntityObject, updated.ID, cur, updated)}, nil, nil - - case domain.OpDelete: - // Inverse of a delete is a restore, under the original id. - if existing, err := s.store.GetObject(ctx, r.EntityID); err == nil { - return nil, conflict(r, domain.ConflictExists, existing.Name), nil - } else if !errors.Is(err, domain.ErrNotFound) { - return nil, nil, err - } - var target domain.GardenObject - if err := unsnapshot(r.Before, &target); err != nil { - return nil, nil, err - } - restored, err := s.store.RestoreObject(ctx, &target) - if err != nil { - return nil, nil, err - } - return []change{changeCreate(domain.EntityObject, restored.ID, restored)}, nil, nil - } - return nil, conflict(r, domain.ConflictUnsupported, ""), nil -} - -func (s *Service) revertPlanting(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { - switch r.Op { - case domain.OpCreate: - cur, err := s.store.GetPlanting(ctx, r.EntityID) - if errors.Is(err, domain.ErrNotFound) { - return nil, nil, nil // already gone - } - if err != nil { - return nil, nil, err - } - if c := versionGuard(r, cur.Version, ""); c != nil { - return nil, c, nil - } - if err := s.store.DeletePlanting(ctx, cur.ID); err != nil { - return nil, nil, err - } - return []change{changeDelete(domain.EntityPlanting, cur.ID, cur)}, nil, nil - - case domain.OpUpdate: - cur, err := s.store.GetPlanting(ctx, r.EntityID) - if errors.Is(err, domain.ErrNotFound) { - return nil, conflict(r, domain.ConflictMissing, ""), nil - } - if err != nil { - return nil, nil, err - } - if c := versionGuard(r, cur.Version, ""); c != nil { - return nil, c, nil - } - var target domain.Planting - if err := unsnapshot(r.Before, &target); err != nil { - return nil, nil, err - } - target.Version = cur.Version - updated, err := s.store.UpdatePlanting(ctx, &target) - if errors.Is(err, domain.ErrVersionConflict) { - return nil, conflict(r, domain.ConflictChanged, ""), nil - } - if err != nil { - return nil, nil, err - } - return []change{changeUpdate(domain.EntityPlanting, updated.ID, cur, updated)}, nil, nil - - case domain.OpDelete: - if _, err := s.store.GetPlanting(ctx, r.EntityID); err == nil { - return nil, conflict(r, domain.ConflictExists, ""), nil - } else if !errors.Is(err, domain.ErrNotFound) { - return nil, nil, err - } - var target domain.Planting - if err := unsnapshot(r.Before, &target); err != nil { - return nil, nil, err - } - restored, err := s.store.RestorePlanting(ctx, &target) - if err != nil { - return nil, nil, err - } - return []change{changeCreate(domain.EntityPlanting, restored.ID, restored)}, nil, nil - } - return nil, conflict(r, domain.ConflictUnsupported, ""), nil -} - -// revertGarden undoes a garden metadata edit. Garden creation and deletion are -// never recorded (see the migration), so update is the only op reachable here. -func (s *Service) revertGarden(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) { - if r.Op != domain.OpUpdate { - return nil, conflict(r, domain.ConflictUnsupported, ""), nil - } - cur, err := s.store.GetGarden(ctx, r.EntityID) - if errors.Is(err, domain.ErrNotFound) { return nil, conflict(r, domain.ConflictMissing, ""), nil } if err != nil { return nil, nil, err } - if c := versionGuard(r, cur.Version, cur.Name); c != nil { + if c := versionGuard(r, ops.version(cur), ops.label(cur), applied[key]); c != nil { return nil, c, nil } - var target domain.Garden + + // Inverse of a create is a removal. + if r.Op == domain.OpCreate { + if ops.remove == nil { + return nil, conflict(r, domain.ConflictUnsupported, ""), nil + } + if ops.blockRemoval != nil { + reason, err := ops.blockRemoval(ctx, cur) + if err != nil { + return nil, nil, err + } + if reason != "" { + return nil, conflict(r, reason, ops.label(cur)), nil + } + } + changes, err := ops.remove(ctx, cur) + if err != nil { + return nil, nil, err + } + delete(applied, key) + return changes, nil, nil + } + + if r.Op != domain.OpUpdate { + return nil, conflict(r, domain.ConflictUnsupported, ""), nil + } + var target T if err := unsnapshot(r.Before, &target); err != nil { return nil, nil, err } - target.Version = cur.Version - updated, err := s.store.UpdateGarden(ctx, &target) + ops.setVersion(&target, ops.version(cur)) + updated, err := ops.update(ctx, &target) if errors.Is(err, domain.ErrVersionConflict) { - return nil, conflict(r, domain.ConflictChanged, cur.Name), nil + return nil, conflict(r, domain.ConflictChanged, ops.label(cur)), nil } if err != nil { return nil, nil, err } - return []change{changeUpdate(domain.EntityGarden, updated.ID, cur, updated)}, nil, nil + applied[key] = ops.version(updated) + return []change{changeUpdate(ops.entityType, r.EntityID, cur, updated)}, nil, nil +} + +func objectRevertOps(s *Service) revertOps[domain.GardenObject] { + return revertOps[domain.GardenObject]{ + entityType: domain.EntityObject, + get: s.store.GetObject, + update: s.store.UpdateObject, + restore: s.store.RestoreObject, + remove: s.deleteObjectRecording, + // Undoing "added a bed" would cascade away anything planted in it since. + // Those plops are snapshotted, so it would be recoverable — but deleting + // someone's plants as a side effect of an unrelated undo should be + // reported, not performed quietly. + blockRemoval: func(ctx context.Context, o *domain.GardenObject) (string, error) { + plops, err := s.store.ListPlantingsForObject(ctx, o.ID) + if err != nil { + return "", err + } + if len(plops) > 0 { + return domain.ConflictChanged, nil + } + return "", nil + }, + version: func(o *domain.GardenObject) int64 { return o.Version }, + setVersion: func(o *domain.GardenObject, v int64) { o.Version = v }, + label: func(o *domain.GardenObject) string { return o.Name }, + } +} + +func plantingRevertOps(s *Service) revertOps[domain.Planting] { + return revertOps[domain.Planting]{ + entityType: domain.EntityPlanting, + get: s.store.GetPlanting, + update: s.store.UpdatePlanting, + restore: s.store.RestorePlanting, + remove: func(ctx context.Context, p *domain.Planting) ([]change, error) { + if err := s.store.DeletePlanting(ctx, p.ID); err != nil { + return nil, err + } + return []change{changeDelete(domain.EntityPlanting, p.ID, p)}, nil + }, + version: func(p *domain.Planting) int64 { return p.Version }, + setVersion: func(p *domain.Planting, v int64) { p.Version = v }, + label: func(p *domain.Planting) string { return "" }, + } +} + +// gardenRevertOps has no restore or remove: garden creation and deletion are +// never recorded (see the migration), so metadata updates are all that can reach +// here, and anything else is honestly reported as unsupported. +func gardenRevertOps(s *Service) revertOps[domain.Garden] { + return revertOps[domain.Garden]{ + entityType: domain.EntityGarden, + get: s.store.GetGarden, + update: s.store.UpdateGarden, + version: func(g *domain.Garden) int64 { return g.Version }, + setVersion: func(g *domain.Garden, v int64) { g.Version = v }, + label: func(g *domain.Garden) string { return g.Name }, + } } // deleteObjectRecording deletes an object and returns every change to record. @@ -526,17 +599,26 @@ func (s *Service) deleteObjectRecording(ctx context.Context, o *domain.GardenObj // one the change set left behind — i.e. something edited it since, and reverting // would silently discard that edit. A revision with no after-snapshot (a delete) // has nothing to compare and passes. -func versionGuard(r domain.Revision, currentVersion int64, name string) *domain.RevertConflict { - var after struct { - Version int64 `json:"version"` - } +// +// appliedVersion is non-zero when THIS revert already wrote to this row, which +// happens whenever a change set holds more than one revision for the same entity. +// In that case the live version is our own doing and is the correct thing to +// expect — comparing against the snapshot would flag our own work as a conflict. +func versionGuard(r domain.Revision, currentVersion int64, name string, appliedVersion int64) *domain.RevertConflict { if r.After == nil { return nil } - if err := json.Unmarshal([]byte(*r.After), &after); err != nil { - return conflict(r, domain.ConflictUnsupported, name) + expected := appliedVersion + if expected == 0 { + var after struct { + Version int64 `json:"version"` + } + if err := json.Unmarshal([]byte(*r.After), &after); err != nil { + return conflict(r, domain.ConflictUnsupported, name) + } + expected = after.Version } - if after.Version != currentVersion { + if expected != currentVersion { return conflict(r, domain.ConflictChanged, name) } return nil diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index b29eb7e..2963b47 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "strings" "testing" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" @@ -96,7 +97,7 @@ func TestFillRegionIsOneChangeSet(t *testing.T) { } // Reverting removes all N and leaves the bed as it was. - _, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID) + _, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) if err != nil { t.Fatalf("RevertChangeSet: %v", err) } @@ -127,7 +128,7 @@ func TestRevertRestoresObjectGeometry(t *testing.T) { } sets := history(t, s, owner, g.ID) - if _, conflicts, err := s.RevertChangeSet(ctx, owner, sets[0].ID); err != nil || len(conflicts) != 0 { + if _, conflicts, err := s.RevertChangeSet(ctx, owner, sets[0].ID, domain.SourceUI); err != nil || len(conflicts) != 0 { t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) } @@ -158,7 +159,7 @@ func TestRevertOfRevertRestoresTheChange(t *testing.T) { } move := history(t, s, owner, g.ID)[0] - undo, _, err := s.RevertChangeSet(ctx, owner, move.ID) + undo, _, err := s.RevertChangeSet(ctx, owner, move.ID, domain.SourceUI) if err != nil { t.Fatalf("revert: %v", err) } @@ -169,7 +170,7 @@ func TestRevertOfRevertRestoresTheChange(t *testing.T) { t.Fatalf("undo didn't restore x: %v", o.XCM) } - redo, conflicts, err := s.RevertChangeSet(ctx, owner, undo.ID) + redo, conflicts, err := s.RevertChangeSet(ctx, owner, undo.ID, domain.SourceUI) if err != nil || len(conflicts) != 0 { t.Fatalf("revert of revert: err=%v conflicts=%+v", err, conflicts) } @@ -236,7 +237,7 @@ func TestRevertConflictsOnLaterEdit(t *testing.T) { t.Fatalf("later edit: %v", err) } - _, conflicts, err := s.RevertChangeSet(ctx, owner, turn.ID) + _, conflicts, err := s.RevertChangeSet(ctx, owner, turn.ID, domain.SourceUI) if err != nil { t.Fatalf("RevertChangeSet: %v", err) } @@ -289,7 +290,7 @@ func TestRevertRestoresDeletedObjectWithItsPlantings(t *testing.T) { t.Fatalf("delete recorded %d revisions, want 2 (the bed and its plop)", len(revs)) } - if _, conflicts, err := s.RevertChangeSet(ctx, owner, del.ID); err != nil || len(conflicts) != 0 { + if _, conflicts, err := s.RevertChangeSet(ctx, owner, del.ID, domain.SourceUI); err != nil || len(conflicts) != 0 { t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) } @@ -336,7 +337,7 @@ func TestRevertClearObject(t *testing.T) { } clear := history(t, s, owner, g.ID)[0] - if _, conflicts, err := s.RevertChangeSet(ctx, owner, clear.ID); err != nil || len(conflicts) != 0 { + if _, conflicts, err := s.RevertChangeSet(ctx, owner, clear.ID, domain.SourceUI); err != nil || len(conflicts) != 0 { t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) } after, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) @@ -365,27 +366,29 @@ func TestRevertPermissions(t *testing.T) { if _, _, err := s.GardenHistory(ctx, viewer, g.ID, 0, 0); err != nil { t.Errorf("a viewer should be able to read history: %v", err) } - if _, _, err := s.RevertChangeSet(ctx, viewer, cs.ID); !errors.Is(err, domain.ErrForbidden) { + if _, _, err := s.RevertChangeSet(ctx, viewer, cs.ID, domain.SourceUI); !errors.Is(err, domain.ErrForbidden) { t.Errorf("viewer revert err = %v, want ErrForbidden", err) } if _, _, err := s.GardenHistory(ctx, stranger, g.ID, 0, 0); !errors.Is(err, domain.ErrNotFound) { t.Errorf("stranger history err = %v, want ErrNotFound", err) } - if _, _, err := s.RevertChangeSet(ctx, stranger, cs.ID); !errors.Is(err, domain.ErrNotFound) { + if _, _, err := s.RevertChangeSet(ctx, stranger, cs.ID, domain.SourceUI); !errors.Is(err, domain.ErrNotFound) { t.Errorf("stranger revert err = %v, want ErrNotFound", err) } } -// TestWithChangeSetDiscardsOnFailure: history records operations that happened, -// not operations that were attempted. -func TestWithChangeSetDiscardsOnFailure(t *testing.T) { +// TestWithChangeSetRecordsWhatCommittedOnFailure — a turn that fails partway has +// still committed the mutations it got through, because this scope buffers +// HISTORY, not data. Dropping the buffer would leave those changes with no +// history and no way to undo them, which is exactly the situation undo exists +// for. So they're recorded, marked as partial, and the failure is still reported. +func TestWithChangeSetRecordsWhatCommittedOnFailure(t *testing.T) { s := newTestService(t, openConfig()) owner := seedUser(t, s, "a@example.com") g := seedGarden(t, s, owner) bed := seedBed(t, s, owner, g.ID) ctx := context.Background() - before := len(history(t, s, owner, g.ID)) sentinel := errors.New("boom") _, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{Source: domain.SourceAgent, Summary: "Half a turn"}, func(ctx context.Context) error { @@ -397,8 +400,24 @@ func TestWithChangeSetDiscardsOnFailure(t *testing.T) { if !errors.Is(err, sentinel) { t.Fatalf("err = %v, want the sentinel", err) } - if got := len(history(t, s, owner, g.ID)); got != before { - t.Errorf("failed turn left %d change sets behind", got-before) + + // The move really happened, so it must be in history and undoable. + if o, _ := s.store.GetObject(ctx, bed.ID); o.XCM != 600 { + t.Fatalf("the committed move was rolled back? x = %v", o.XCM) + } + sets := history(t, s, owner, g.ID) + partial := sets[0] + if !strings.Contains(partial.Summary, "failed partway") { + t.Errorf("summary = %q, want it to say the turn failed partway", partial.Summary) + } + if len(revisionsOf(t, s, partial.ID)) != 1 { + t.Errorf("expected the one committed mutation to be recorded") + } + if _, conflicts, err := s.RevertChangeSet(ctx, owner, partial.ID, domain.SourceUI); err != nil || len(conflicts) != 0 { + t.Fatalf("the partial turn should be undoable: err=%v conflicts=%+v", err, conflicts) + } + if o, _ := s.store.GetObject(ctx, bed.ID); o.XCM != bed.XCM { + t.Errorf("undo of the partial turn didn't restore x: %v", o.XCM) } } @@ -437,7 +456,7 @@ func TestGardenMetadataRevert(t *testing.T) { t.Fatalf("UpdateGarden: %v", err) } cs := history(t, s, owner, g.ID)[0] - if _, conflicts, err := s.RevertChangeSet(ctx, owner, cs.ID); err != nil || len(conflicts) != 0 { + if _, conflicts, err := s.RevertChangeSet(ctx, owner, cs.ID, domain.SourceUI); err != nil || len(conflicts) != 0 { t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) } back, _ := s.store.GetGarden(ctx, g.ID) @@ -504,3 +523,184 @@ func TestSnapshotsCaptureVersion(t *testing.T) { } func f64Ptr(v float64) *float64 { return &v } + +// TestRevertChangeSetTouchingOneEntityTwice — an agent turn that moves a bed and +// then renames it holds two revisions for the same object. Each inverse bumps +// that row's version, so from the second one on the snapshot's version no longer +// matches the live row. Without tracking what the revert itself just wrote, the +// guard flags our own work as somebody else's edit and the undo half-fails. +func TestRevertChangeSetTouchingOneEntityTwice(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + _, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{Source: domain.SourceAgent, Summary: "Two edits"}, + func(ctx context.Context) error { + moved, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(600)}, bed.Version) + if err != nil { + return err + } + _, err = s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("Renamed")}, moved.Version) + return err + }) + if err != nil { + t.Fatalf("WithChangeSet: %v", err) + } + turn := history(t, s, owner, g.ID)[0] + if len(revisionsOf(t, s, turn.ID)) != 2 { + t.Fatalf("expected 2 revisions for the same object") + } + + _, conflicts, err := s.RevertChangeSet(ctx, owner, turn.ID, domain.SourceUI) + if err != nil { + t.Fatalf("RevertChangeSet: %v", err) + } + if len(conflicts) != 0 { + t.Fatalf("the revert conflicted with its own work: %+v", conflicts) + } + back, _ := s.store.GetObject(ctx, bed.ID) + if back.XCM != bed.XCM || back.Name != bed.Name { + t.Errorf("both edits should have been undone, got x=%v name=%q", back.XCM, back.Name) + } +} + +// TestRevertOfObjectCreateRefusesWhenPlanted — undoing "added a bed" would +// cascade away anything planted in it since. Those plops would be snapshotted and +// so recoverable, but deleting someone's plants as a side effect of an unrelated +// undo should be reported, not performed quietly. +func TestRevertOfObjectCreateRefusesWhenPlanted(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + bed := seedBed(t, s, owner, g.ID) + create := history(t, s, owner, g.ID)[0] + + plant := seedOwnPlant(t, s, owner, 15) + if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, + }); err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + + cs, conflicts, err := s.RevertChangeSet(ctx, owner, create.ID, domain.SourceUI) + if err != nil { + t.Fatalf("RevertChangeSet: %v", err) + } + if len(conflicts) != 1 || conflicts[0].EntityID != bed.ID { + t.Fatalf("expected one conflict naming the bed, got %+v", conflicts) + } + if cs != nil { + t.Errorf("nothing should have been reverted, got change set %+v", cs) + } + if _, err := s.store.GetObject(ctx, bed.ID); err != nil { + t.Errorf("the planted bed was deleted anyway: %v", err) + } +} + +// TestHistoryDoesNotDuplicateMultiplyRevertedEntries — more than one change set +// can point at the same target (undo, redo, undo again). Looking that up with a +// LEFT JOIN would emit one duplicate row per revert and silently corrupt the +// page, so it is a scalar subquery instead. Written against the store because +// getting a target legitimately reverted twice through the service is hard: the +// second attempt conflicts, which is itself the correct behaviour. +func TestHistoryDoesNotDuplicateMultiplyRevertedEntries(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + ctx := context.Background() + + target := history(t, s, owner, g.ID) // garden create isn't recorded; start empty + if len(target) != 0 { + t.Fatalf("expected an empty history, got %+v", target) + } + base, err := s.store.WriteChangeSet(ctx, &domain.ChangeSet{ + GardenID: g.ID, ActorID: owner, Source: domain.SourceUI, Summary: "Base", + }, []domain.Revision{{EntityType: domain.EntityGarden, EntityID: g.ID, Op: domain.OpUpdate}}) + if err != nil { + t.Fatalf("WriteChangeSet: %v", err) + } + for i := 0; i < 2; i++ { + if _, err := s.store.WriteChangeSet(ctx, &domain.ChangeSet{ + GardenID: g.ID, ActorID: owner, Source: domain.SourceUI, Summary: "Undo", RevertsID: &base.ID, + }, []domain.Revision{{EntityType: domain.EntityGarden, EntityID: g.ID, Op: domain.OpUpdate}}); err != nil { + t.Fatalf("WriteChangeSet revert %d: %v", i, err) + } + } + + sets := history(t, s, owner, g.ID) + if len(sets) != 3 { + t.Fatalf("got %d change sets, want 3 — a join would have duplicated the base row: %+v", len(sets), sets) + } + seen := map[int64]int{} + for _, cs := range sets { + seen[cs.ID]++ + } + for id, n := range seen { + if n != 1 { + t.Errorf("change set %d appears %d times", id, n) + } + } + for _, cs := range sets { + if cs.ID == base.ID && (cs.RevertedByID == nil || *cs.RevertedByID == 0) { + t.Error("the base change set should be marked as reverted") + } + } +} + +// TestRevertSourceIsRecorded — an agent undoing its own work must be +// distinguishable from a person clicking undo, or the history badge lies. +func TestRevertSourceIsRecorded(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + ctx := context.Background() + + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{XCM: f64Ptr(300)}, bed.Version); err != nil { + t.Fatalf("UpdateObject: %v", err) + } + move := history(t, s, owner, g.ID)[0] + + undo, _, err := s.RevertChangeSet(ctx, owner, move.ID, domain.SourceAgent) + if err != nil { + t.Fatalf("revert: %v", err) + } + if undo.Source != domain.SourceAgent { + t.Errorf("source = %q, want agent", undo.Source) + } + if _, _, err := s.RevertChangeSet(ctx, owner, move.ID, "nonsense"); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("an unknown source should be rejected, got %v", err) + } +} + +// TestClearObjectOnlyClearsWhatItSnapshotted — the clear targets the exact ids it +// read, so a plop created between the read and the UPDATE can't be removed +// without a revision to undo it by. +func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + t.Fatalf("fill: %v", err) + } + before, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) + + n, err := s.ClearObject(ctx, owner, bed.ID) + if err != nil { + t.Fatalf("ClearObject: %v", err) + } + clear := history(t, s, owner, g.ID)[0] + revs := revisionsOf(t, s, clear.ID) + // Every row the clear touched has a revision; the count and the revisions agree. + if n != len(before) || len(revs) != n { + t.Errorf("cleared %d of %d with %d revisions — all three should match", n, len(before), len(revs)) + } +} diff --git a/internal/store/plantings.go b/internal/store/plantings.go index 9a705ce..14c16f7 100644 --- a/internal/store/plantings.go +++ b/internal/store/plantings.go @@ -102,15 +102,28 @@ func (d *DB) RestorePlanting(ctx context.Context, p *domain.Planting) (*domain.P return restored, nil } -// ClearObjectPlantings soft-removes every active plop in an object in one UPDATE +// ClearObjectPlantings soft-removes the given plops of an object in one UPDATE // (sets removed_at=date, bumps version) and returns how many rows it affected. -func (d *DB) ClearObjectPlantings(ctx context.Context, objectID int64, date string) (int, error) { +// +// It takes explicit ids rather than clearing "every active plop" so the caller +// can snapshot exactly the rows it is about to change. Clearing by predicate +// would let a plop created between the caller's read and this UPDATE be removed +// without a revision — cleared, but with no way to undo it. +func (d *DB) ClearObjectPlantings(ctx context.Context, objectID int64, date string, ids []int64) (int, error) { + if len(ids) == 0 { + return 0, nil + } + args := make([]any, 0, len(ids)+2) + args = append(args, date, objectID) + for _, id := range ids { + args = append(args, id) + } res, err := d.sql.ExecContext(ctx, `UPDATE plantings SET removed_at = ?, version = version + 1, updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now') - WHERE object_id = ? AND removed_at IS NULL`, - date, objectID, + WHERE object_id = ? AND removed_at IS NULL AND id IN (`+placeholders(len(ids))+`)`, + args..., ) if err != nil { return 0, fmt.Errorf("store: clear object plantings: %w", err) diff --git a/internal/store/revisions.go b/internal/store/revisions.go index 1f788c2..669ebab 100644 --- a/internal/store/revisions.go +++ b/internal/store/revisions.go @@ -79,11 +79,15 @@ func (d *DB) WriteChangeSet(ctx context.Context, cs *domain.ChangeSet, revs []do // it (if any), and per-(entity,op) counts — everything the history list renders, // without a second round-trip per row. Always a non-nil slice. func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset int) ([]domain.ChangeSet, error) { + // The "was this reverted?" lookup is a scalar subquery, NOT a LEFT JOIN: a + // change set can be reverted more than once (undo, redo, undo again), and a + // join would then emit one duplicate row per revert and silently corrupt the + // page. MIN(id) names the first revert, which is the one worth showing. rows, err := d.sql.QueryContext(ctx, - `SELECT `+qualifyColumns("cs", changeSetColumns)+`, u.display_name, rev.id + `SELECT `+qualifyColumns("cs", changeSetColumns)+`, u.display_name, + (SELECT MIN(r.id) FROM change_sets r WHERE r.reverts_id = cs.id) FROM change_sets cs JOIN users u ON u.id = cs.actor_id - LEFT JOIN change_sets rev ON rev.reverts_id = cs.id WHERE cs.garden_id = ? ORDER BY cs.id DESC LIMIT ? OFFSET ?`, -- 2.54.0