From 8f736048b9aab32b5c22206723fb2dc7e3a6e1a3 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 22:39:48 -0400 Subject: [PATCH] Address Gadfly review on #14: bounds trap, date order, count cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - UpdatePlanting: only re-check the object-bounds when the position is actually being moved. A plop orphaned outside its object by a later resize stays editable/removable instead of becoming a row you can't fix or delete. - finalizePlanting: reject removed_at before planted_at. - derivedCount: guard Inf (not just NaN) and cap the result at maxExplicitCount (1e6) — the same ceiling a manual override honors — so a huge radius / tiny spacing can't overflow a 32-bit int or return an absurd value. - Refresh stale docs that referenced #14 as not-yet-landed (FullGarden, ListActivePlantingsForGarden, ListReferencedPlants) and note the date-only layout beside timeLayout. Deliberately did NOT add a plantable re-check to Update/Delete: existing plops must stay editable/removable even if their object was later marked non-plantable (same trap as the bounds case). Tests: derived-count cap, removed-before-planted rejection, and edit/move of a plop orphaned by an object shrink. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- internal/api/plantings_test.go | 2 +- internal/service/objects.go | 4 +- internal/service/plantings.go | 37 ++++++++++++---- internal/service/plantings_test.go | 68 +++++++++++++++++++++++++++++- internal/service/service.go | 8 ++-- internal/store/plantings.go | 3 +- internal/store/plants.go | 4 +- 7 files changed, 106 insertions(+), 20 deletions(-) diff --git a/internal/api/plantings_test.go b/internal/api/plantings_test.go index 5126240..5275377 100644 --- a/internal/api/plantings_test.go +++ b/internal/api/plantings_test.go @@ -51,7 +51,7 @@ func TestPlantingCreatePatchFullDelete(t *testing.T) { } // Soft-remove (clear-bed style) → it leaves /full. - w = doJSON(t, r, http.MethodPatch, plantingPath(pid), map[string]any{"removedAt": "2026-07-01", "version": 1}, cookie) + w = doJSON(t, r, http.MethodPatch, plantingPath(pid), map[string]any{"removedAt": "2030-06-01", "version": 1}, cookie) if w.Code != http.StatusOK { t.Fatalf("soft-remove: status %d, body %s", w.Code, w.Body.String()) } diff --git a/internal/service/objects.go b/internal/service/objects.go index e8cab1b..2e27bb0 100644 --- a/internal/service/objects.go +++ b/internal/service/objects.go @@ -69,8 +69,8 @@ type ObjectPatch struct { } // FullGarden is the one-shot editor payload: the garden plus everything drawn in -// it. Plantings/Plants stay empty until #14 populates plantings; the shape is -// fixed now so the frontend types against it once. +// it — objects, active plantings (each with its DerivedCount filled in), and the +// plants those plantings reference. type FullGarden struct { Garden *domain.Garden `json:"garden"` Objects []domain.GardenObject `json:"objects"` diff --git a/internal/service/plantings.go b/internal/service/plantings.go index 88068e4..7396d82 100644 --- a/internal/service/plantings.go +++ b/internal/service/plantings.go @@ -21,10 +21,12 @@ const ( // derivedCount is the plant count implied by a plop's area and the plant's // mature spacing: max(1, round(π·r² / spacing²)). It is the single source of the -// formula (also feeds #15's display and #19's FillRegion). A non-positive radius -// or spacing collapses to the floor of 1. +// formula (also feeds #15's display and #19's FillRegion). A non-positive or +// non-finite radius/spacing collapses to the floor of 1; the result is capped at +// maxExplicitCount so a huge radius / tiny spacing can't yield an absurd (or, on +// a 32-bit int, overflowing) value — the same ceiling a manual override honors. func derivedCount(radiusCM, spacingCM float64) int { - if radiusCM <= 0 || spacingCM <= 0 || math.IsNaN(radiusCM) || math.IsNaN(spacingCM) { + if radiusCM <= 0 || spacingCM <= 0 || !isFinite(radiusCM) || !isFinite(spacingCM) { return 1 } area := math.Pi * radiusCM * radiusCM @@ -32,6 +34,9 @@ func derivedCount(radiusCM, spacingCM float64) int { if n < 1 { return 1 } + if n > maxExplicitCount { + return maxExplicitCount + } return n } @@ -93,7 +98,7 @@ func (s *Service) CreatePlanting(ctx context.Context, actorID, objectID int64, i today := s.now().UTC().Format(dateLayout) p.PlantedAt = &today } - if err := finalizePlanting(p, o); err != nil { + if err := finalizePlanting(p, o, true); err != nil { return nil, err } created, err := s.store.CreatePlanting(ctx, p) @@ -122,7 +127,12 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64, if err != nil { return nil, err } - if err := finalizePlanting(pl, o); err != nil { + // Only re-check the object bounds when the position is actually being moved. + // A plop left outside its object's bounds by a later object resize must still + // be editable (radius, dates, soft-remove) — otherwise it becomes a row you + // can neither fix nor remove. + checkBounds := patch.XCM != nil || patch.YCM != nil + if err := finalizePlanting(pl, o, checkBounds); err != nil { return nil, err } pl.Version = version @@ -207,8 +217,11 @@ func applyPlantingPatch(pl *domain.Planting, p PlantingPatch) { } // finalizePlanting validates a built/merged plop against its parent object. -// Shared by create and update so both enforce the same invariants. -func finalizePlanting(p *domain.Planting, o *domain.GardenObject) error { +// Shared by create and update so both enforce the same invariants. checkBounds +// gates the center-in-object-bounds test: create always checks it, update only +// when the position is being moved (so a resize that orphans a plop outside the +// shrunken object doesn't block editing/removing it). +func finalizePlanting(p *domain.Planting, o *domain.GardenObject, checkBounds bool) error { if !isFinite(p.RadiusCM) || p.RadiusCM <= 0 || p.RadiusCM > maxRadiusCM { return domain.ErrInvalidInput } @@ -217,7 +230,7 @@ func finalizePlanting(p *domain.Planting, o *domain.GardenObject) error { } // Loose bounds: the plop's CENTER must sit within the object's unrotated // local bounds (origin at object center); the radius may overhang. - if math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2 { + if checkBounds && (math.Abs(p.XCM) > o.WidthCM/2 || math.Abs(p.YCM) > o.HeightCM/2) { return domain.ErrInvalidInput } if p.Count != nil && (*p.Count < 1 || *p.Count > maxExplicitCount) { @@ -229,6 +242,14 @@ func finalizePlanting(p *domain.Planting, o *domain.GardenObject) error { if !validDatePtr(p.PlantedAt) || !validDatePtr(p.RemovedAt) { return domain.ErrInvalidInput } + // A plop can't be removed before it was planted (nonsensical interval). + if p.PlantedAt != nil && p.RemovedAt != nil { + planted, _ := time.Parse(dateLayout, *p.PlantedAt) + removed, _ := time.Parse(dateLayout, *p.RemovedAt) + if removed.Before(planted) { + return domain.ErrInvalidInput + } + } return nil } diff --git a/internal/service/plantings_test.go b/internal/service/plantings_test.go index 63c9c29..437511b 100644 --- a/internal/service/plantings_test.go +++ b/internal/service/plantings_test.go @@ -51,6 +51,14 @@ func TestDerivedCountFormula(t *testing.T) { } } +func TestDerivedCountCapped(t *testing.T) { + // A huge radius with a tiny spacing would produce an absurd (32-bit + // overflowing) count; it's capped at the same ceiling as a manual override. + if got := derivedCount(10_000, 0.1); got != maxExplicitCount { + t.Errorf("derivedCount(10000, 0.1) = %d, want cap %d", got, maxExplicitCount) + } +} + func TestCreatePlantingDefaultsAndDerivedCount(t *testing.T) { s := newTestService(t, openConfig()) owner := seedUser(t, s, "a@example.com") @@ -230,8 +238,8 @@ func TestPlantingSoftRemoveLeavesFull(t *testing.T) { t.Errorf("full.Plants = %d, want 1 referenced plant", len(full.Plants)) } - // Soft-remove → it leaves /full. - rm := "2026-07-01" + // Soft-remove → it leaves /full. (On/after today's default planted_at.) + rm := "2030-06-01" if _, err := s.UpdatePlanting(context.Background(), owner, pl.ID, PlantingPatch{SetRemovedAt: true, RemovedAt: &rm}, pl.Version); err != nil { t.Fatalf("soft-remove: %v", err) @@ -242,6 +250,62 @@ func TestPlantingSoftRemoveLeavesFull(t *testing.T) { } } +func TestPlantingRemovedBeforePlantedRejected(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, 10) + + // planted_at defaults to today; a removed_at in the distant past is invalid. + pl, _ := s.CreatePlanting(context.Background(), owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 10, + }) + past := "2000-01-01" + if _, err := s.UpdatePlanting(context.Background(), owner, pl.ID, + PlantingPatch{SetRemovedAt: true, RemovedAt: &past}, pl.Version); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("removed-before-planted err = %v, want ErrInvalidInput", err) + } +} + +func TestUpdatePlantingEditableAfterObjectShrinks(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) // 200×200 → local bounds ±100 + plant := seedOwnPlant(t, s, owner, 10) + + // A plop near the bed's edge. + pl, _ := s.CreatePlanting(context.Background(), owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 90, YCM: 0, RadiusCM: 10, + }) + + // Shrink the bed to 100×100 (local bounds ±50): the plop's center (90) is now + // outside the object. + w, h := 100.0, 100.0 + if _, err := s.UpdateObject(context.Background(), owner, bed.ID, ObjectPatch{WidthCM: &w, HeightCM: &h}, bed.Version); err != nil { + t.Fatalf("shrink bed: %v", err) + } + + // Editing the out-of-bounds plop without moving it (radius here) must still + // work — otherwise it'd be un-removable. + nr := 15.0 + updated, err := s.UpdatePlanting(context.Background(), owner, pl.ID, PlantingPatch{RadiusCM: &nr}, pl.Version) + if err != nil { + t.Fatalf("radius edit on orphaned plop should work: %v", err) + } + // But actually moving it still enforces the new bounds. + outside := 90.0 + if _, err := s.UpdatePlanting(context.Background(), owner, pl.ID, PlantingPatch{XCM: &outside}, updated.Version); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("moving to out-of-bounds should be rejected: %v", err) + } + // Moving it back inside works. + inside := 10.0 + if _, err := s.UpdatePlanting(context.Background(), owner, pl.ID, PlantingPatch{XCM: &inside}, updated.Version); err != nil { + t.Errorf("moving in-bounds should work: %v", err) + } +} + func TestPlantingVersionConflict(t *testing.T) { s := newTestService(t, openConfig()) owner := seedUser(t, s, "a@example.com") diff --git a/internal/service/service.go b/internal/service/service.go index b33cedd..2c7a0d6 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -20,9 +20,11 @@ import ( "gitea.stevedudenhoeffer.com/steve/pansy/internal/store" ) -// timeLayout is the ISO-8601 UTC format used for every timestamp pansy stores. -// It matches the schema's strftime('%Y-%m-%dT%H:%M:%SZ') so string comparison -// (e.g. session expiry) is equivalent to time comparison. +// timeLayout is the ISO-8601 UTC format used for every full timestamp pansy +// stores (created_at/updated_at/expires_at). It matches the schema's +// strftime('%Y-%m-%dT%H:%M:%SZ') so string comparison (e.g. session expiry) is +// equivalent to time comparison. Date-only fields (planting planted_at/ +// removed_at) use dateLayout instead. const timeLayout = "2006-01-02T15:04:05Z" // sessionTTL is how long a session lives from its last use (sliding expiry). diff --git a/internal/store/plantings.go b/internal/store/plantings.go index 2a9f5f9..a60f430 100644 --- a/internal/store/plantings.go +++ b/internal/store/plantings.go @@ -28,8 +28,7 @@ func scanPlanting(s scanner) (*domain.Planting, error) { // ListActivePlantingsForGarden returns every currently-planted plop (removed_at // IS NULL) across all objects in a garden — the editor's one-shot load. Always a -// non-nil slice (empty until plantings CRUD lands in #14). Plop CRUD itself is -// #14; this is only the /full read side. +// non-nil slice. The service fills each row's DerivedCount; this is the raw read. func (d *DB) ListActivePlantingsForGarden(ctx context.Context, gardenID int64) ([]domain.Planting, error) { rows, err := d.sql.QueryContext(ctx, `SELECT `+qualifyColumns("pl", plantingColumns)+` FROM plantings pl diff --git a/internal/store/plants.go b/internal/store/plants.go index 14f4744..e1b53ee 100644 --- a/internal/store/plants.go +++ b/internal/store/plants.go @@ -26,8 +26,8 @@ func scanPlant(s scanner) (*domain.Plant, error) { // ListReferencedPlants returns the distinct plants used by a garden's active // plantings — the catalog subset the editor needs to render them. Always a -// non-nil slice (empty until plantings exist, #14). Full plant CRUD/seeding is -// #12; this is only the /full read side. +// non-nil slice (empty when the garden has no active plantings). This is the +// /full read side; full plant CRUD/seeding lives in plants.go / plantings.go. func (d *DB) ListReferencedPlants(ctx context.Context, gardenID int64) ([]domain.Plant, error) { rows, err := d.sql.QueryContext(ctx, `SELECT DISTINCT `+qualifyColumns("p", plantColumns)+` FROM plants p