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 ?`,