Copy a garden (deep duplicate) from the gardens list #46

Merged
steve merged 2 commits from feat/copy-garden into main 2026-07-21 03:58:40 +00:00
12 changed files with 154 additions and 57 deletions
Showing only changes of commit e1ec93ddff - Show all commits
+1 -1
View File
@@ -81,7 +81,7 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
gardens.GET("/:id", h.getGarden)
gardens.PATCH("/:id", h.updateGarden)
gardens.DELETE("/:id", h.deleteGarden)
gardens.POST("/:id/copy", h.copyGarden) // duplicate a garden the actor owns
gardens.POST("/:id/copy", h.copyGarden) // duplicate a garden the actor owns
gardens.GET("/:id/full", h.getGardenFull) // one-shot editor load
gardens.POST("/:id/objects", h.createObject)
+7 -6
View File
@@ -2,6 +2,7 @@ package api
import (
"errors"
"io"
"net/http"
"github.com/gin-gonic/gin"
@@ -115,13 +116,13 @@ func (h *handlers) copyGarden(c *gin.Context) {
return
}
// The body is optional — POST with no body means "copy under the default
// name" — so only decode when one was actually sent.
// name". An absent body surfaces as io.EOF from the decoder, which is the
// reliable signal: Content-Length is -1 (not 0) for a chunked request, so
Outdated
Review

🟠 Inconsistent optional-body pattern uses ContentLength instead of io.EOF

error-handling, maintainability · flagged by 4 models

  • internal/api/gardens.go:120 — The copyGarden handler checks c.Request.ContentLength != 0 to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in internal/api/public.go:56: try c.ShouldBindJSON(&req) and ignore io.EOF. The ContentLength guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave ContentLength == -1 even with no body, which would cause the handler to incorre…

🪰 Gadfly · advisory

🟠 **Inconsistent optional-body pattern uses ContentLength instead of io.EOF** _error-handling, maintainability · flagged by 4 models_ - `internal/api/gardens.go:120` — The `copyGarden` handler checks `c.Request.ContentLength != 0` to decide whether to parse an optional JSON body, but the codebase already has an established pattern for this in `internal/api/public.go:56`: try `c.ShouldBindJSON(&req)` and ignore `io.EOF`. The `ContentLength` guard is both inconsistent with surrounding code and less robust (e.g., chunked encoding or HTTP/2 can leave `ContentLength == -1` even with no body, which would cause the handler to incorre… <sub>🪰 Gadfly · advisory</sub>
// testing the length would misread an empty chunked body as malformed.
var req gardenCopyRequest
if c.Request.ContentLength != 0 {
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "name must be a string")
return
}
if err := c.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "name must be a string")
return
}
g, err := h.svc.CopyGarden(c.Request.Context(), mustActor(c).ID, id, req.Name)
if err != nil {
+32
View File
@@ -2,8 +2,11 @@ package api
import (
"encoding/json"
"io"
"net/http"
"net/http/httptest"
"strconv"
"strings"
"testing"
"github.com/gin-gonic/gin"
@@ -247,3 +250,32 @@ func TestCopyGardenEndpoint(t *testing.T) {
t.Errorf("bad body status = %d, want 400", w.Code)
}
}
// A body of unknown length (chunked transfer, Content-Length -1) that turns out
// to be empty must take the default-name path, not fail as malformed.
func TestCopyGardenEmptyChunkedBody(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"name": "Home"}, cookie)
id := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
// A plain io.Reader (not a *strings.Reader/*bytes.Buffer) leaves
// ContentLength at -1, which is what a chunked request looks like server-side.
req := httptest.NewRequest(http.MethodPost,
"/api/v1/gardens/"+strconv.FormatInt(id, 10)+"/copy", io.NopCloser(strings.NewReader("")))
req.Header.Set("Content-Type", "application/json")
req.AddCookie(cookie)
if req.ContentLength != -1 {
t.Fatalf("test setup: ContentLength = %d, want -1 (unknown)", req.ContentLength)
}
rec := httptest.NewRecorder()
r.ServeHTTP(rec, req)
if rec.Code != http.StatusCreated {
t.Fatalf("status = %d, want 201; body %s", rec.Code, rec.Body.String())
}
if got := decodeMap(t, rec.Body.Bytes())["name"]; got != "Home (copy)" {
t.Errorf("name = %v, want the derived 'Home (copy)'", got)
}
}
+2 -4
View File
@@ -345,9 +345,8 @@ func TestCopyGardenSkipsRemovedPlantings(t *testing.T) {
bed := seedBed(t, s, owner, src.ID)
plant := seedOwnPlant(t, s, owner, 20)
kept, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, RadiusCM: 10})
if err != nil {
t.Fatalf("CreatePlanting(kept): %v", err)
if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, RadiusCM: 10}); err != nil {
Outdated
Review

🟡 Misleadingly-named dead variable kept in TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not kept

maintainability · flagged by 1 model

  • internal/service/gardens_test.go:348 (TestCopyGardenSkipsRemovedPlantings) — the local var kept (declared line 348, silenced via _ = kept at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: ClearObjectPlantings (internal/store/plantings.go:76-92), invoked via ClearObject (internal/service/ops.go:221-227), soft-removes every active plop on the object…

🪰 Gadfly · advisory

🟡 **Misleadingly-named dead variable `kept` in TestCopyGardenSkipsRemovedPlantings — it is actually cleared, not kept** _maintainability · flagged by 1 model_ - `internal/service/gardens_test.go:348` (`TestCopyGardenSkipsRemovedPlantings`) — the local var `kept` (declared line 348, silenced via `_ = kept` at line 364) is misleadingly named and is dead code. It's created, never asserted on, and then discarded just to silence "declared and not used." Worse, the name actively misleads: `ClearObjectPlantings` (`internal/store/plantings.go:76-92`), invoked via `ClearObject` (`internal/service/ops.go:221-227`), soft-removes *every* active plop on the object… <sub>🪰 Gadfly · advisory</sub>
t.Fatalf("CreatePlanting(first): %v", err)
}
if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, XCM: 50, RadiusCM: 10}); err != nil {
t.Fatalf("CreatePlanting(cleared): %v", err)
@@ -361,7 +360,6 @@ func TestCopyGardenSkipsRemovedPlantings(t *testing.T) {
if err != nil {
t.Fatalf("CreatePlanting(replanted): %v", err)
}
_ = kept
dup, err := s.CopyGarden(ctx, owner, src.ID, "")
Review

🟡 Unused variable suppressed with _ = kept instead of using _ in original assignment

maintainability · flagged by 3 models

  • internal/service/gardens_test.go:364 (in TestCopyGardenSkipsRemovedPlantings) — The test assigns a planting to kept only to suppress it with _ = kept. Using _, err := s.CreatePlanting(...) on that line avoids the unused-variable workaround.

🪰 Gadfly · advisory

🟡 **Unused variable suppressed with _ = kept instead of using _ in original assignment** _maintainability · flagged by 3 models_ - `internal/service/gardens_test.go:364` (in `TestCopyGardenSkipsRemovedPlantings`) — The test assigns a planting to `kept` only to suppress it with `_ = kept`. Using `_, err := s.CreatePlanting(...)` on that line avoids the unused-variable workaround. <sub>🪰 Gadfly · advisory</sub>
if err != nil {
+2 -15
View File
@@ -213,16 +213,7 @@ func (d *DB) CopyGarden(ctx context.Context, srcID, ownerID int64, name string)
// re-parented onto the right new object.
objectIDs := make(map[int64]int64, len(srcObjects))
for _, o := range srcObjects {
copied, err := scanObject(tx.QueryRowContext(ctx,
`INSERT INTO garden_objects
(garden_id, kind, name, shape, points, x_cm, y_cm, width_cm, height_cm,
rotation_deg, z_index, plantable, color, props, grid_size_cm, snap_to_grid, notes)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
RETURNING `+objectColumns,
created.ID, o.Kind, o.Name, o.Shape, o.Points, o.XCM, o.YCM, o.WidthCM, o.HeightCM,
o.RotationDeg, o.ZIndex, boolToInt(o.Plantable), o.Color, o.Props,
o.GridSizeCM, boolToInt(o.SnapToGrid), o.Notes,
))
copied, err := scanObject(tx.QueryRowContext(ctx, objectInsert, objectInsertArgs(created.ID, &o)...))
Outdated
Review

🟠 CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips

maintainability, performance · flagged by 2 models

  • internal/store/gardens.go:216-253CopyGarden executes one INSERT ... RETURNING per source object and one INSERT per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (maxGardensListed at internal/store/gardens.go:50 only bounds the list endpoint). The DB is opened in WAL mode with busy_timeout(5000) (internal/store/sqlite.go:20-21), so only one writer can proceed app-wide; every other user's wri…

🪰 Gadfly · advisory

🟠 **CopyGarden inserts objects/plantings one row at a time inside a single write transaction, serializing all other writers under WAL for O(n) round-trips** _maintainability, performance · flagged by 2 models_ - `internal/store/gardens.go:216-253` — `CopyGarden` executes one `INSERT ... RETURNING` per source object and one `INSERT` per active planting, sequentially, inside a single write transaction. There is no cap on how many objects/plantings a garden can hold (`maxGardensListed` at `internal/store/gardens.go:50` only bounds the *list* endpoint). The DB is opened in WAL mode with `busy_timeout(5000)` (`internal/store/sqlite.go:20-21`), so only one writer can proceed app-wide; every other user's wri… <sub>🪰 Gadfly · advisory</sub>
if err != nil {
return nil, fmt.Errorf("store: copy object: %w", err)
}
@@ -243,11 +234,7 @@ 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)
}
if _, err := tx.ExecContext(ctx,
`INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
objectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt,
); err != nil {
if _, err := scanPlanting(tx.QueryRowContext(ctx, plantingInsert, plantingInsertArgs(objectID, &p)...)); err != nil {
return nil, fmt.Errorf("store: copy planting: %w", err)
}
}
+21 -10
View File
@@ -31,19 +31,30 @@ func scanObject(s scanner) (*domain.GardenObject, error) {
return &o, nil
}
// objectInsert inserts one garden_objects row and returns it. It is shared by
// CreateObject and CopyGarden's in-transaction copy, with objectInsertArgs
// supplying its parameters, so a column added to the table can't be wired into
// one path and silently dropped from the other.
const objectInsert = `INSERT INTO garden_objects
(garden_id, kind, name, shape, points, x_cm, y_cm, width_cm, height_cm,
rotation_deg, z_index, plantable, color, props, grid_size_cm, snap_to_grid, notes)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
RETURNING ` + objectColumns
// objectInsertArgs builds objectInsert's parameters, placing o in gardenID (which
// is o's own garden on create, and the new garden when copying).
func objectInsertArgs(gardenID int64, o *domain.GardenObject) []any {
return []any{
gardenID, o.Kind, o.Name, o.Shape, o.Points, o.XCM, o.YCM, o.WidthCM, o.HeightCM,
o.RotationDeg, o.ZIndex, boolToInt(o.Plantable), o.Color, o.Props,
o.GridSizeCM, boolToInt(o.SnapToGrid), o.Notes,
}
}
// CreateObject inserts a garden object (fields already validated by the service)
// and returns the stored row.
func (d *DB) CreateObject(ctx context.Context, o *domain.GardenObject) (*domain.GardenObject, error) {
created, err := scanObject(d.sql.QueryRowContext(ctx,
`INSERT INTO garden_objects
(garden_id, kind, name, shape, points, x_cm, y_cm, width_cm, height_cm,
rotation_deg, z_index, plantable, color, props, grid_size_cm, snap_to_grid, notes)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
RETURNING `+objectColumns,
o.GardenID, o.Kind, o.Name, o.Shape, o.Points, o.XCM, o.YCM, o.WidthCM, o.HeightCM,
o.RotationDeg, o.ZIndex, boolToInt(o.Plantable), o.Color, o.Props,
o.GridSizeCM, boolToInt(o.SnapToGrid), o.Notes,
))
created, err := scanObject(d.sql.QueryRowContext(ctx, objectInsert, objectInsertArgs(o.GardenID, o)...))
if err != nil {
return nil, fmt.Errorf("store: insert object: %w", err)
}
+18 -13
View File
@@ -104,16 +104,25 @@ func (d *DB) GetPlanting(ctx context.Context, id int64) (*domain.Planting, error
return p, nil
}
// plantingInsert inserts one plantings row and returns it. Shared by
// CreatePlanting, the CreatePlantings batch and CopyGarden's in-transaction copy
// — with plantingInsertArgs supplying its parameters — so all three stay in step
// with the table. removed_at is never set here: a new plop is active, and "clear
// bed" sets removed_at later via UpdatePlanting.
const plantingInsert = `INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?)
RETURNING ` + plantingColumns
// plantingInsertArgs builds plantingInsert's parameters, parenting p to objectID
// (p's own object normally, and the copied object when copying a garden).
func plantingInsertArgs(objectID int64, p *domain.Planting) []any {
return []any{objectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt}
}
// CreatePlanting inserts a plop (fields already validated by the service) and
// returns the stored row. removed_at is always NULL on create — a new plop is
// active; "clear bed" sets removed_at later via UpdatePlanting.
// returns the stored row.
func (d *DB) CreatePlanting(ctx context.Context, p *domain.Planting) (*domain.Planting, error) {
created, err := scanPlanting(d.sql.QueryRowContext(ctx,
`INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?)
RETURNING `+plantingColumns,
p.ObjectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt,
))
created, err := scanPlanting(d.sql.QueryRowContext(ctx, plantingInsert, plantingInsertArgs(p.ObjectID, p)...))
if err != nil {
return nil, fmt.Errorf("store: insert planting: %w", err)
}
@@ -132,13 +141,9 @@ func (d *DB) CreatePlantings(ctx context.Context, plantings []*domain.Planting)
}
defer tx.Rollback() //nolint:errcheck // no-op after a successful commit
const stmt = `INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm, count, label, planted_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?)
RETURNING ` + plantingColumns
out := make([]domain.Planting, 0, len(plantings))
for _, p := range plantings {
created, err := scanPlanting(tx.QueryRowContext(ctx, stmt,
p.ObjectID, p.PlantID, p.XCM, p.YCM, p.RadiusCM, p.Count, p.Label, p.PlantedAt))
created, err := scanPlanting(tx.QueryRowContext(ctx, plantingInsert, plantingInsertArgs(p.ObjectID, p)...))
if err != nil {
return nil, fmt.Errorf("store: insert planting (batch): %w", err)
}
+8
View File
@@ -5,6 +5,7 @@
package store
import (
"context"
"database/sql"
"fmt"
"net/url"
@@ -99,6 +100,13 @@ func pragmaName(p string) string {
return strings.ToLower(strings.TrimSpace(p))
}
// queryer is the read surface shared by *sql.DB and *sql.Tx, so a list helper can
// serve both a standalone read and one inside a transaction. (Its single-row
// counterpart is `scanner`, in users.go.)
type queryer interface {
QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error)
}
// qualifyColumns prefixes each comma-separated column in cols with "alias." so a
// shared column list can be used in a JOIN — e.g. qualifyColumns("pl", "id, x")
// → "pl.id, pl.x". Whitespace/newlines in the list are trimmed.
-6
View File
@@ -19,12 +19,6 @@ type scanner interface {
Scan(dest ...any) error
}
// queryer is the read surface shared by *sql.DB and *sql.Tx, so a list helper can
// serve both a standalone read and one inside a transaction.
type queryer interface {
QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error)
}
// scanUser reads one users row. is_admin is stored as INTEGER 0/1 (the driver
Outdated
Review

Generic queryer interface declared in users.go rather than a shared store file

maintainability · flagged by 1 model

  • internal/store/gardens.go:217 and internal/store/plantings.go:112/:135 — the INSERT INTO garden_objects … and INSERT INTO plantings … column lists are duplicated verbatim between CreateObject/CreatePlanting and the in-transaction copy in CopyGarden. The copy can't call CreateObject/CreatePlanting directly because those bind to d.sql rather than tx, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / t…

🪰 Gadfly · advisory

⚪ **Generic queryer interface declared in users.go rather than a shared store file** _maintainability · flagged by 1 model_ - `internal/store/gardens.go:217` and `internal/store/plantings.go:112`/`:135` — the `INSERT INTO garden_objects …` and `INSERT INTO plantings …` column lists are duplicated verbatim between `CreateObject`/`CreatePlanting` and the in-transaction copy in `CopyGarden`. The copy can't call `CreateObject`/`CreatePlanting` directly because those bind to `d.sql` rather than `tx`, so the duplication is a deliberate tradeoff, but it means a future schema/column change must be edited in two (objects) / t… <sub>🪰 Gadfly · advisory</sub>
// returns it as int64, which does not convert straight to bool), so it is read
// into an int64 and mapped.
@@ -6,7 +6,7 @@ import { Modal } from '@/components/ui/Modal'
import { TextField } from '@/components/ui/TextField'
import { toast } from '@/components/ui/toast'
import { errorMessage } from '@/lib/api'
import { useCopyGarden, type Garden } from '@/lib/gardens'
import { defaultCopyName, useCopyGarden, type Garden } from '@/lib/gardens'
/**
* Duplicate a garden under a new name. The name is prefilled with the server's
@@ -17,7 +17,7 @@ import { useCopyGarden, type Garden } from '@/lib/gardens'
export function CopyGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const copy = useCopyGarden()
const navigate = useNavigate()
const [name, setName] = useState(`${garden.name} (copy)`)
const [name, setName] = useState(() => defaultCopyName(garden.name))
Outdated
Review

🟠 Client-side copy-name derivation diverges from server truncation logic

correctness, error-handling, maintainability · flagged by 3 models

  • web/src/components/gardens/CopyGardenModal.tsx:20 — The modal pre-derives the copy name with `${garden.name} (copy)`, duplicating the server's copyName() logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed maxGardenNameLen and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. Fix: either share the truncation logic (e.g., a common util) or prefill with a name capped the…

🪰 Gadfly · advisory

🟠 **Client-side copy-name derivation diverges from server truncation logic** _correctness, error-handling, maintainability · flagged by 3 models_ - `web/src/components/gardens/CopyGardenModal.tsx:20` — The modal pre-derives the copy name with `` `${garden.name} (copy)` ``, duplicating the server's `copyName()` logic but without the byte-cap truncation. If the source name is long, the prefilled default can exceed `maxGardenNameLen` and will be rejected by the service when submitted unchanged. The client and server derivations are out of sync. **Fix:** either share the truncation logic (e.g., a common util) or prefill with a name capped the… <sub>🪰 Gadfly · advisory</sub>
const [error, setError] = useState<string | null>(null)
async function onSubmit(e: React.FormEvent) {
+31
View File
@@ -0,0 +1,31 @@
import { describe, expect, it } from 'vitest'
import { defaultCopyName } from './gardens'
// The server caps a garden name at 200 BYTES and derives a copy's name the same
// way (copyName in internal/service/gardens.go). These cases pin the mirror: an
// over-long prefill would come back as a 400 rather than a copy.
describe('defaultCopyName', () => {
it('appends the suffix to a short name', () => {
expect(defaultCopyName('Home')).toBe('Home (copy)')
})
it('keeps a derived name within the 200-byte cap', () => {
const name = 'a'.repeat(200)
const derived = defaultCopyName(name)
expect(new TextEncoder().encode(derived).length).toBeLessThanOrEqual(200)
expect(derived.endsWith(' (copy)')).toBe(true)
})
it('truncates multi-byte names on a code-point boundary', () => {
const name = 'é'.repeat(100) // 200 bytes, exactly at the cap
const derived = defaultCopyName(name)
expect(new TextEncoder().encode(derived).length).toBeLessThanOrEqual(200)
// No replacement character: nothing was cut mid-code-point.
expect(derived).not.toContain('')
})
it('does not leave a dangling space before the suffix', () => {
const name = 'x'.repeat(192) + ' y' // truncation lands on the space
expect(defaultCopyName(name)).toBe('x'.repeat(192) + ' (copy)')
})
})
+30
View File
@@ -86,6 +86,36 @@ export function useUpdateGarden() {
})
}
// Mirrors the server's garden-name cap (maxGardenNameLen in
// internal/service/gardens.go). It is a BYTE cap, not a character count.
const MAX_GARDEN_NAME_BYTES = 200
const COPY_SUFFIX = ' (copy)'
/** Trim s to at most maxBytes of UTF-8, never splitting a code point. */
function truncateUtf8(s: string, maxBytes: number): string {
const enc = new TextEncoder()
if (enc.encode(s).length <= maxBytes) return s
let out = ''
let used = 0
for (const ch of s) {
const size = enc.encode(ch).length
if (used + size > maxBytes) break
out += ch
used += size
}
return out
}
/**
* The name a copy gets by default. Mirrors the server's copyName (same file as
* the cap above) so the prefilled value is one the server will accept — without
* the byte-capped truncation, copying a garden whose name is already at the cap
* would prefill an over-long name and fail with a 400.
*/
export function defaultCopyName(name: string): string {
return truncateUtf8(name, MAX_GARDEN_NAME_BYTES - COPY_SUFFIX.length).trim() + COPY_SUFFIX
}
/**
* Duplicate a garden the actor owns, including its objects and current plantings.
* An omitted/blank name lets the server derive "<source> (copy)". The copy does