diff --git a/internal/api/errors.go b/internal/api/errors.go index 2a16c31..83bc538 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -1,6 +1,7 @@ package api import ( + "encoding/json" "errors" "log/slog" "net/http" @@ -69,6 +70,26 @@ func writeVersionConflict(c *gin.Context, current any) { }) } +// parseNullable decodes any JSON value into (value, present) for a nullable +// column: absent → (nil, false); explicit null → (nil, true); anything else → +// the decoded value. That three-way distinction is what lets a PATCH tell "clear +// this to NULL" apart from "leave it alone", and every nullable field in the API +// needs it, so it lives here rather than in whichever file happened to want it +// first. +func parseNullable[T any](raw json.RawMessage) (*T, bool, error) { + if len(raw) == 0 { + return nil, false, nil + } + if string(raw) == "null" { + return nil, true, nil + } + var v T + if err := json.Unmarshal(raw, &v); err != nil { + return nil, false, err + } + return &v, true, nil +} + // 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 { diff --git a/internal/api/seed_lots.go b/internal/api/seed_lots.go index 9460cc8..d70419d 100644 --- a/internal/api/seed_lots.go +++ b/internal/api/seed_lots.go @@ -81,22 +81,6 @@ func (r seedLotUpdateRequest) toPatch() (service.SeedLotPatch, error) { return p, nil } -// parseNullable decodes any JSON value into (value, present) for a nullable -// column. Absent → (nil, false); null → (nil, true); otherwise the decoded value. -func parseNullable[T any](raw json.RawMessage) (*T, bool, error) { - if len(raw) == 0 { - return nil, false, nil - } - if string(raw) == "null" { - return nil, true, nil - } - var v T - if err := json.Unmarshal(raw, &v); err != nil { - return nil, false, err - } - return &v, true, nil -} - func (h *handlers) listSeedLots(c *gin.Context) { var plantID *int64 if raw := c.Query("plantId"); raw != "" { diff --git a/internal/service/plantings.go b/internal/service/plantings.go index b81c5fe..246f749 100644 --- a/internal/service/plantings.go +++ b/internal/service/plantings.go @@ -107,7 +107,7 @@ func (s *Service) CreatePlanting(ctx context.Context, actorID, objectID int64, i if err := finalizePlanting(p, o, true); err != nil { return nil, err } - if err := s.seedLotForPlanting(ctx, actorID, p.SeedLotID, p.PlantID); err != nil { + if err := s.checkSeedLotForPlanting(ctx, actorID, p.SeedLotID, p.PlantID); err != nil { return nil, err } created, err := s.store.CreatePlanting(ctx, p) @@ -159,7 +159,7 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64, } // Re-checked on every update, not just when the lot changes: a plant swap can // leave a previously-valid attribution pointing at a lot of the wrong variety. - if err := s.seedLotForPlanting(ctx, actorID, pl.SeedLotID, pl.PlantID); err != nil { + if err := s.checkSeedLotForPlanting(ctx, actorID, pl.SeedLotID, pl.PlantID); err != nil { return nil, err } pl.Version = version diff --git a/internal/service/plants.go b/internal/service/plants.go index 5b90abf..c664b7d 100644 --- a/internal/service/plants.go +++ b/internal/service/plants.go @@ -18,6 +18,7 @@ const ( minPlantSpacingCM = 0.1 maxPlantSpacingCM = 10_000 // 100 m — headroom for large trees/shrubs maxDaysToMaturity = 3650 // ~10 years + maxPlantVendorLen = 200 ) // plantCategories is the set of valid category enum values (mirrors the schema @@ -211,7 +212,7 @@ func finalizePlant(p *domain.Plant) error { if len(p.Notes) > maxPlantNotesLen { return domain.ErrInvalidInput } - if len(p.Vendor) > maxLotTextLen { + if len(p.Vendor) > maxPlantVendorLen { return domain.ErrInvalidInput } // Rendered as a clickable link, so the scheme check is load-bearing (see diff --git a/internal/service/revisions.go b/internal/service/revisions.go index d71afa2..ee59555 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -417,6 +417,9 @@ type revertOps[T any] struct { // 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) + // beforeRestore fixes up a snapshot whose references may have gone stale + // while it sat in history. + beforeRestore func(ctx context.Context, row *T) 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) @@ -445,6 +448,11 @@ func revertEntity[T any]( if err := unsnapshot(r.Before, &target); err != nil { return nil, nil, err } + if ops.beforeRestore != nil { + if err := ops.beforeRestore(ctx, &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. @@ -547,6 +555,22 @@ func plantingRevertOps(s *Service) revertOps[domain.Planting] { get: s.store.GetPlanting, update: s.store.UpdatePlanting, restore: s.store.RestorePlanting, + // A snapshot can name a seed lot that has since been deleted. The live + // rows got their link nulled by ON DELETE SET NULL, but the snapshot + // still holds the old id, and restoring it would violate the foreign key + // and abort the whole revert. Drop the dangling link instead: the plant + // really was in the ground, which is the part worth restoring. + beforeRestore: func(ctx context.Context, p *domain.Planting) error { + if p.SeedLotID == nil { + return nil + } + if _, err := s.store.GetSeedLot(ctx, *p.SeedLotID); errors.Is(err, domain.ErrNotFound) { + p.SeedLotID = nil + } else if err != nil { + return err + } + return nil + }, remove: func(ctx context.Context, p *domain.Planting) ([]change, error) { if err := s.store.DeletePlanting(ctx, p.ID); err != nil { return nil, err diff --git a/internal/service/seed_lots.go b/internal/service/seed_lots.go index 0962dac..9bfa8fc 100644 --- a/internal/service/seed_lots.go +++ b/internal/service/seed_lots.go @@ -2,6 +2,7 @@ package service import ( "context" + "log/slog" "net/url" "strings" @@ -90,11 +91,19 @@ func (s *Service) ListSeedLots(ctx context.Context, actorID int64, plantID *int6 // when you plant — drifts the moment anything edits or removes a planting behind // its back, and once it has drifted there is no way to tell what the true number // was. Attribution plus arithmetic can always be recomputed and audited. +// Remaining may go NEGATIVE, and that is deliberate: it means more was planted +// than the lot recorded buying, which is a real thing that happens (a +// miscounted packet, a lot entered after the fact). Clamping it to zero would +// hide the discrepancy at exactly the moment it is worth seeing. func (s *Service) fillRemaining(ctx context.Context, actorID int64, lots []domain.SeedLot) error { if len(lots) == 0 { return nil } - uses, err := s.store.SeedLotUsage(ctx, actorID) + ids := make([]int64, 0, len(lots)) + for i := range lots { + ids = append(ids, lots[i].ID) + } + uses, err := s.store.SeedLotUsage(ctx, actorID, ids) if err != nil { return err } @@ -115,11 +124,12 @@ func (s *Service) fillRemaining(ctx context.Context, actorID int64, lots []domai // ownSeedLot loads a lot and enforces that the actor owns it. Someone else's lot // is ErrNotFound, not ErrForbidden — existence stays masked, same as gardens and -// other users' plants. +// other users' plants. A store failure that isn't "no such row" passes through +// unchanged; it is not an authorization answer. func (s *Service) ownSeedLot(ctx context.Context, actorID, lotID int64) (*domain.SeedLot, error) { l, err := s.store.GetSeedLot(ctx, lotID) if err != nil { - return nil, err // ErrNotFound + return nil, err // ErrNotFound for a missing row; anything else is real } if l.OwnerID != actorID { return nil, domain.ErrNotFound @@ -133,11 +143,7 @@ func (s *Service) GetSeedLot(ctx context.Context, actorID, lotID int64) (*domain if err != nil { return nil, err } - one := []domain.SeedLot{*l} - if err := s.fillRemaining(ctx, actorID, one); err != nil { - return nil, err - } - return &one[0], nil + return s.withRemaining(ctx, actorID, l) } // CreateSeedLot records a purchase owned by the actor. The plant must be one the @@ -165,7 +171,23 @@ func (s *Service) CreateSeedLot(ctx context.Context, actorID int64, in SeedLotIn if err := finalizeSeedLot(l); err != nil { return nil, err } - return s.store.CreateSeedLot(ctx, l) + created, err := s.store.CreateSeedLot(ctx, l) + if err != nil { + return nil, err + } + // A fresh lot has used nothing, but say so explicitly rather than letting the + // zero value stand in: an unfilled Remaining reads as "none left" on a packet + // you just bought. + return s.withRemaining(ctx, actorID, created) +} + +// withRemaining fills Used/Remaining on a single lot. +func (s *Service) withRemaining(ctx context.Context, actorID int64, l *domain.SeedLot) (*domain.SeedLot, error) { + one := []domain.SeedLot{*l} + if err := s.fillRemaining(ctx, actorID, one); err != nil { + return nil, err + } + return &one[0], nil } // UpdateSeedLot applies a partial, version-guarded patch to the actor's own lot. @@ -181,13 +203,24 @@ func (s *Service) UpdateSeedLot(ctx context.Context, actorID, lotID int64, patch l.Version = version updated, err := s.store.UpdateSeedLot(ctx, l) if err != nil { + // The conflict path carries the CURRENT row back to the client to rebase + // on, so it needs its computed fields too — a 409 body reporting + // remaining=0 would be worse than no number at all. + if updated != nil { + if filled, ferr := s.withRemaining(ctx, actorID, updated); ferr == nil { + updated = filled + } + } return updated, err } - one := []domain.SeedLot{*updated} - if ferr := s.fillRemaining(ctx, actorID, one); ferr != nil { - return nil, ferr + // The write succeeded. If the follow-up read fails, return the row without + // its computed fields rather than reporting a failed update that applied. + filled, ferr := s.withRemaining(ctx, actorID, updated) + if ferr != nil { + slog.Error("service: lot updated but remaining could not be computed", "error", ferr, "lot", lotID) + return updated, nil } - return &one[0], nil + return filled, nil } // DeleteSeedLot removes the actor's own lot. Plantings attributed to it survive @@ -200,11 +233,11 @@ func (s *Service) DeleteSeedLot(ctx context.Context, actorID, lotID int64) error return s.store.DeleteSeedLot(ctx, lotID) } -// seedLotForPlanting validates a planting's lot attribution: the lot must be the -// actor's, and must be a lot OF the plant being planted. Attributing a garlic +// checkSeedLotForPlanting validates a planting's lot attribution: the lot must be +// the actor's, and must be a lot OF the plant being planted. Attributing a garlic // planting to a tomato lot is a data error that would silently corrupt every // remaining count downstream, so it's refused rather than stored. -func (s *Service) seedLotForPlanting(ctx context.Context, actorID int64, lotID *int64, plantID int64) error { +func (s *Service) checkSeedLotForPlanting(ctx context.Context, actorID int64, lotID *int64, plantID int64) error { if lotID == nil { return nil } diff --git a/internal/service/seed_lots_test.go b/internal/service/seed_lots_test.go index cb7dadb..e7a2eed 100644 --- a/internal/service/seed_lots_test.go +++ b/internal/service/seed_lots_test.go @@ -53,11 +53,11 @@ func TestTwoLotsOfOnePlantTrackIndependently(t *testing.T) { johnnys := seedLot(t, s, owner, plant.ID, 100, func(in *SeedLotInput) { in.Vendor = "Johnny's" in.SourceURL = "https://www.johnnyseeds.com/vegetables/garlic/german-red-garlic" - in.PackedForYear = intPtr(2026) + in.PackedForYear = ptrInt(2026) }) fedco := seedLot(t, s, owner, plant.ID, 50, func(in *SeedLotInput) { in.Vendor = "Fedco" - in.PackedForYear = intPtr(2025) + in.PackedForYear = ptrInt(2025) }) // Plant 12 out of the Johnny's lot only. @@ -342,8 +342,6 @@ func TestSeedLotUnitAndQuantityValidation(t *testing.T) { } } -func intPtr(v int) *int { return &v } - // TestDeletingAPlantWithLotsIsRefused — seed_lots.plant_id is ON DELETE CASCADE, // so without a guard, deleting a plant would silently destroy the purchase // records attached to it: vendor, cost, germination rate, gone with no warning. @@ -351,7 +349,7 @@ func TestDeletingAPlantWithLotsIsRefused(t *testing.T) { s := newTestService(t, openConfig()) owner := seedUser(t, s, "a@example.com") plant := seedOwnPlant(t, s, owner, 15) - lot := seedLot(t, s, owner, plant.ID, 100, func(in *SeedLotInput) { in.CostCents = intPtr(499) }) + lot := seedLot(t, s, owner, plant.ID, 100, func(in *SeedLotInput) { in.CostCents = ptrInt(499) }) ctx := context.Background() if err := s.DeletePlant(ctx, owner, plant.ID); !errors.Is(err, domain.ErrPlantInUse) { @@ -369,3 +367,153 @@ func TestDeletingAPlantWithLotsIsRefused(t *testing.T) { t.Errorf("DeletePlant after clearing lots: %v", err) } } + +// TestFreshLotReportsItsFullQuantity — an unfilled Remaining reads as "none +// left" on a packet you just bought, which is the opposite of the truth. +func TestFreshLotReportsItsFullQuantity(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + plant := seedOwnPlant(t, s, owner, 15) + + lot := seedLot(t, s, owner, plant.ID, 100, nil) + if lot.Remaining != 100 || lot.Used != 0 { + t.Errorf("fresh lot: used=%v remaining=%v, want 0/100", lot.Used, lot.Remaining) + } +} + +// TestVersionConflictCarriesRemaining — the 409 body is what the client rebases +// on, so it needs the computed fields too. +func TestVersionConflictCarriesRemaining(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) + lot := seedLot(t, s, owner, plant.ID, 100, nil) + ctx := context.Background() + + ten := 10 + if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, Count: &ten, SeedLotID: &lot.ID, + }); err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + if _, err := s.UpdateSeedLot(ctx, owner, lot.ID, SeedLotPatch{Notes: strPtr("first")}, lot.Version); err != nil { + t.Fatalf("first update: %v", err) + } + current, err := s.UpdateSeedLot(ctx, owner, lot.ID, SeedLotPatch{Notes: strPtr("stale")}, lot.Version) + if !errors.Is(err, domain.ErrVersionConflict) { + t.Fatalf("err = %v, want ErrVersionConflict", err) + } + if current == nil || current.Remaining != 90 || current.Used != 10 { + t.Errorf("conflict row: used=%v remaining=%v, want 10/90", current.Used, current.Remaining) + } +} + +// TestRemainingGoesNegativeRatherThanHidingIt — planting more than you recorded +// buying is a real thing; clamping to zero would hide the discrepancy at exactly +// the moment it is worth seeing. +func TestRemainingGoesNegativeRatherThanHidingIt(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) + lot := seedLot(t, s, owner, plant.ID, 5, nil) + ctx := context.Background() + + twenty := 20 + if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, Count: &twenty, SeedLotID: &lot.ID, + }); err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + got, err := s.GetSeedLot(ctx, owner, lot.ID) + if err != nil { + t.Fatalf("GetSeedLot: %v", err) + } + if got.Remaining != -15 { + t.Errorf("remaining = %v, want -15 (20 planted from a lot of 5)", got.Remaining) + } +} + +// TestCopiedGardenDoesNotInheritSeedLots — a copy is a plan, not a second +// planting out of the same packet. Carrying the link would double-count the +// original lot's usage and quietly attach a private lot to a garden that may +// later be shared. +func TestCopiedGardenDoesNotInheritSeedLots(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) + lot := seedLot(t, s, owner, plant.ID, 100, nil) + ctx := context.Background() + + ten := 10 + if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, Count: &ten, SeedLotID: &lot.ID, + }); err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + + copied, err := s.CopyGarden(ctx, owner, g.ID, "Copy") + if err != nil { + t.Fatalf("CopyGarden: %v", err) + } + full, err := s.GardenFull(ctx, owner, copied.ID) + if err != nil { + t.Fatalf("GardenFull: %v", err) + } + if len(full.Plantings) != 1 { + t.Fatalf("copy has %d plantings, want 1", len(full.Plantings)) + } + if full.Plantings[0].SeedLotID != nil { + t.Errorf("the copy inherited seed lot %v", *full.Plantings[0].SeedLotID) + } + // And the original lot's usage is unchanged: still 10, not 20. + got, _ := s.GetSeedLot(ctx, owner, lot.ID) + if got.Used != 10 { + t.Errorf("copying double-counted the lot: used = %v, want 10", got.Used) + } +} + +// TestRevertRestoresAPlantingWhoseLotIsGone — a snapshot can name a lot that has +// since been deleted; restoring it verbatim would violate the FK and abort the +// whole revert. +func TestRevertRestoresAPlantingWhoseLotIsGone(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) + lot := seedLot(t, s, owner, plant.ID, 100, nil) + ctx := context.Background() + + pl, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, SeedLotID: &lot.ID, + }) + if err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + if err := s.DeletePlanting(ctx, owner, pl.ID); err != nil { + t.Fatalf("DeletePlanting: %v", err) + } + del := history(t, s, owner, g.ID)[0] + + // The lot goes away while the deletion sits in history. + if err := s.DeleteSeedLot(ctx, owner, lot.ID); err != nil { + t.Fatalf("DeleteSeedLot: %v", err) + } + + 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) + } + restored, err := s.store.GetPlanting(ctx, pl.ID) + if err != nil { + t.Fatalf("planting not restored: %v", err) + } + if restored.SeedLotID != nil { + t.Errorf("a dangling lot reference was restored: %v", *restored.SeedLotID) + } +} diff --git a/internal/store/gardens.go b/internal/store/gardens.go index 20e5b8a..68fa56b 100644 --- a/internal/store/gardens.go +++ b/internal/store/gardens.go @@ -234,6 +234,11 @@ func (d *DB) CopyGarden(ctx context.Context, srcID, ownerID int64, name string) // Unreachable: every active planting joins an object we just copied. return nil, fmt.Errorf("store: copy planting %d: source object %d not copied", p.ID, p.ObjectID) } + // A copy must not inherit the source's seed-lot attribution. The copied + // plops are a plan, not a second planting out of the same packet, and + // carrying the link would double-count that lot's usage — and quietly + // attach a private lot to a garden that may later be shared. + p.SeedLotID = nil if _, err := scanPlanting(tx.QueryRowContext(ctx, plantingInsert, plantingInsertArgs(objectID, &p)...)); err != nil { return nil, fmt.Errorf("store: copy planting: %w", err) } diff --git a/internal/store/seed_lots.go b/internal/store/seed_lots.go index a26e4b5..688a6cb 100644 --- a/internal/store/seed_lots.go +++ b/internal/store/seed_lots.go @@ -9,6 +9,10 @@ import ( "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" ) +// maxSeedLotsPerRead caps a lot listing. Far past any real seed shelf; it exists +// so the read is bounded rather than trusting it to stay small. +const maxSeedLotsPerRead = 1000 + // seedLotColumns lists seed_lots columns in the order scanSeedLot expects. const seedLotColumns = `id, owner_id, plant_id, vendor, source_url, sku, lot_code, purchased_at, packed_for_year, quantity, unit, cost_cents, germination_pct, notes, @@ -67,8 +71,11 @@ func (d *DB) ListSeedLotsForOwner(ctx context.Context, ownerID int64, plantID *i query += ` AND plant_id = ?` args = append(args, *plantID) } - // NULL purchased_at sorts last: an undated lot is the least useful to see first. - query += ` ORDER BY purchased_at IS NULL, purchased_at DESC, id DESC` + // NULL purchased_at sorts last: an undated lot is the least useful to see + // first. Capped like every other list read so one runaway account can't ask + // for an unbounded result set. + query += ` ORDER BY purchased_at IS NULL, purchased_at DESC, id DESC LIMIT ?` + args = append(args, maxSeedLotsPerRead) rows, err := d.sql.QueryContext(ctx, query, args...) if err != nil { @@ -161,21 +168,35 @@ type SeedLotUse struct { SpacingCM float64 } -// SeedLotUsage returns the ACTIVE plantings attributed to any of the owner's -// lots — the "used" half of a lot's remaining quantity. +// SeedLotUsage returns the ACTIVE plantings attributed to the given lots — the +// "used" half of a lot's remaining quantity. Scoped to the lots the caller +// actually needs, so reading one lot doesn't scan every planting the owner has. // // It returns the raw ingredients rather than a SUM, because the effective count // of a planting is its explicit count when it has one and the derived // π·r²/spacing² formula otherwise. That formula lives in the service; reproducing // it in SQL would guarantee the two drift apart. -func (d *DB) SeedLotUsage(ctx context.Context, ownerID int64) ([]SeedLotUse, error) { +// +// ownerID is still in the WHERE clause even though the ids come from rows the +// service already checked: a scoping query should not depend on its caller +// having checked scope. +func (d *DB) SeedLotUsage(ctx context.Context, ownerID int64, lotIDs []int64) ([]SeedLotUse, error) { + if len(lotIDs) == 0 { + return nil, nil + } + args := make([]any, 0, len(lotIDs)+1) + args = append(args, ownerID) + for _, id := range lotIDs { + args = append(args, id) + } rows, err := d.sql.QueryContext(ctx, `SELECT pl.seed_lot_id, pl.radius_cm, pl.count, p.spacing_cm FROM plantings pl JOIN seed_lots sl ON sl.id = pl.seed_lot_id JOIN plants p ON p.id = pl.plant_id - WHERE sl.owner_id = ? AND pl.removed_at IS NULL`, - ownerID) + WHERE sl.owner_id = ? AND pl.removed_at IS NULL + AND sl.id IN (`+placeholders(len(lotIDs))+`)`, + args...) if err != nil { return nil, fmt.Errorf("store: seed lot usage: %w", err) }