- Reject a degenerate (zero-area or inverted) fill rect with 400 instead of
silently planting a single plop at the object centre. An empty `"rect": {}`
decodes to all-zeros and is caught the same way. New fillRect.degenerate()
and a table test covering {}, zero-width, zero-height, and inverted.
- Make the rect a named type (fillRect) rather than an anonymous inline struct,
matching the rest of internal/api, and bind the pointer to a local in the
handler so the deref is visibly guarded rather than reading req.Rect.MinX
under an invariant from a line above.
- ops_test: drop gid from the unpack instead of the `_ = gid` shim.
- ClearBedModal: drop the `const n = plopCount` no-op alias.
Not taken: moving createPlantAPI/makeFillPlant to a shared helper — that
consolidation spans this branch and #88 and is cleanest once both land, as
noted in the PR. Reusing an objectPlantingsPath helper: there isn't one in this
package, and the one inline use doesn't earn a new helper on its own.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
This commit is contained in:
+32
-12
@@ -19,6 +19,24 @@ import (
|
||||
// service methods `internal/agent/tools.go` calls, so the permission checks and
|
||||
// the one-change-set-per-operation guarantee come along unchanged.
|
||||
|
||||
// fillRect is an explicit rectangle in the object's local frame, the alternative
|
||||
// to a compass name. A named type (not an inline anonymous struct) to match the
|
||||
// rest of internal/api and so it can carry its own validity check.
|
||||
type fillRect struct {
|
||||
MinX float64 `json:"minXCm"`
|
||||
MinY float64 `json:"minYCm"`
|
||||
MaxX float64 `json:"maxXCm"`
|
||||
MaxY float64 `json:"maxYCm"`
|
||||
}
|
||||
|
||||
// degenerate reports whether the rect encloses no area. Such a rect (including
|
||||
// the all-zeros an empty `"rect": {}` decodes to) would otherwise slip through
|
||||
// and plant a single plop at the object's centre — a surprising result for what
|
||||
// is really malformed input.
|
||||
func (r fillRect) degenerate() bool {
|
||||
return r.MaxX <= r.MinX || r.MaxY <= r.MinY
|
||||
}
|
||||
|
||||
// fillRequest is the body for POST /objects/:id/fill.
|
||||
//
|
||||
// A region is given EITHER by compass name ("ne", "south half", "all") or as an
|
||||
@@ -27,14 +45,9 @@ import (
|
||||
// Exactly one must be supplied — accepting both and silently preferring one
|
||||
// would make a client bug look like a geometry bug.
|
||||
type fillRequest struct {
|
||||
PlantID int64 `json:"plantId" binding:"required"`
|
||||
Region string `json:"region"`
|
||||
Rect *struct {
|
||||
MinX float64 `json:"minXCm"`
|
||||
MinY float64 `json:"minYCm"`
|
||||
MaxX float64 `json:"maxXCm"`
|
||||
MaxY float64 `json:"maxYCm"`
|
||||
} `json:"rect"`
|
||||
PlantID int64 `json:"plantId" binding:"required"`
|
||||
Region string `json:"region"`
|
||||
Rect *fillRect `json:"rect"`
|
||||
// SpacingOverrideCM plants tighter or looser than the plant's mature spacing
|
||||
// without editing the catalog entry.
|
||||
SpacingOverrideCM *float64 `json:"spacingOverrideCm"`
|
||||
@@ -62,11 +75,18 @@ func (h *handlers) fillObject(c *gin.Context) {
|
||||
created []domain.Planting
|
||||
err error
|
||||
)
|
||||
if named {
|
||||
created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM)
|
||||
} else {
|
||||
region := service.Region{MinX: req.Rect.MinX, MinY: req.Rect.MinY, MaxX: req.Rect.MaxX, MaxY: req.Rect.MaxY}
|
||||
if rect := req.Rect; rect != nil {
|
||||
// Reject a zero-area rect here rather than let it plant one stray plop.
|
||||
// (Binding the pointer to `rect` also keeps the deref visibly guarded,
|
||||
// instead of reading req.Rect.MinX under an invariant from a line above.)
|
||||
if rect.degenerate() {
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "rect must enclose a positive area")
|
||||
return
|
||||
}
|
||||
region := service.Region{MinX: rect.MinX, MinY: rect.MinY, MaxX: rect.MaxX, MaxY: rect.MaxY}
|
||||
created, err = h.svc.FillRegion(c.Request.Context(), actor, id, region, req.PlantID, req.SpacingOverrideCM)
|
||||
} else {
|
||||
created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM)
|
||||
}
|
||||
if err != nil {
|
||||
writeServiceError(c, err)
|
||||
|
||||
@@ -61,7 +61,7 @@ func countChangeSets(t *testing.T, r *gin.Engine, cookie *http.Cookie, gardenID
|
||||
func TestFillAndClearAPI(t *testing.T) {
|
||||
r := authEngine(t, localCfg())
|
||||
cookie := registerAndCookie(t, r, "[email protected]")
|
||||
gid, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20)
|
||||
_, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20)
|
||||
|
||||
// Fill by compass name.
|
||||
w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
|
||||
@@ -96,7 +96,6 @@ func TestFillAndClearAPI(t *testing.T) {
|
||||
if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 {
|
||||
t.Errorf("second clear removed %d, want 0", n)
|
||||
}
|
||||
_ = gid
|
||||
}
|
||||
|
||||
// TestFillRegionSelectionAPI: exactly one of region/rect, and a rect fills only
|
||||
@@ -125,6 +124,20 @@ func TestFillRegionSelectionAPI(t *testing.T) {
|
||||
t.Errorf("bad region name = %d, want 400", w.Code)
|
||||
}
|
||||
|
||||
// A zero-area rect is malformed input, not "plant one at the centre". An empty
|
||||
// `"rect": {}` decodes to all-zeros and must be caught the same way.
|
||||
for _, rect := range []map[string]any{
|
||||
{}, // {} → 0,0,0,0
|
||||
{"minXCm": 10, "minYCm": 10, "maxXCm": 10, "maxYCm": 50}, // zero width
|
||||
{"minXCm": 10, "minYCm": 50, "maxXCm": 50, "maxYCm": 50}, // zero height
|
||||
{"minXCm": 50, "minYCm": 50, "maxXCm": 10, "maxYCm": 10}, // inverted
|
||||
} {
|
||||
if w := doJSON(t, r, http.MethodPost, fillPath(objID),
|
||||
map[string]any{"plantId": plantID, "rect": rect}, cookie); w.Code != http.StatusBadRequest {
|
||||
t.Errorf("degenerate rect %v = %d, want 400", rect, w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// A rect confined to the NE corner produces plops only there. Local frame:
|
||||
// +x east, -y north.
|
||||
w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{
|
||||
|
||||
@@ -18,12 +18,12 @@ export function ClearBedModal({
|
||||
onClose: () => void
|
||||
}) {
|
||||
const clear = useClearObject(gardenId)
|
||||
const n = plopCount
|
||||
return (
|
||||
<Modal title="Clear bed" onClose={onClose} busy={clear.isPending}>
|
||||
<div className="flex flex-col gap-4">
|
||||
<p className="text-sm text-muted">
|
||||
Remove all <span className="font-medium text-fg">{n}</span> {n === 1 ? 'plant' : 'plants'} from{' '}
|
||||
Remove all <span className="font-medium text-fg">{plopCount}</span>{' '}
|
||||
{plopCount === 1 ? 'plant' : 'plants'} from{' '}
|
||||
<span className="font-medium text-fg">{objectName}</span>? They're marked removed but kept in history.
|
||||
</p>
|
||||
<div className="flex justify-end gap-2">
|
||||
@@ -33,7 +33,7 @@ export function ClearBedModal({
|
||||
<Button
|
||||
type="button"
|
||||
variant="danger"
|
||||
disabled={clear.isPending || n === 0}
|
||||
disabled={clear.isPending || plopCount === 0}
|
||||
onClick={() => clear.mutate(objectId, { onSuccess: onClose })}
|
||||
>
|
||||
{clear.isPending ? 'Clearing…' : 'Clear bed'}
|
||||
|
||||
Reference in New Issue
Block a user