Address Gadfly review on #14: bounds trap, date order, count cap
Build image / build-and-push (push) Successful in 5s
Build image / build-and-push (push) Successful in 5s
- 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) <[email protected]> Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user