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
15 changed files with 752 additions and 52 deletions
+1
View File
@@ -52,6 +52,7 @@ POST /auth/register | /auth/login | /auth/logout GET /auth/me GET /auth
GET /auth/oidc/login GET /auth/oidc/callback (authorization-code + PKCE)
GET,POST /gardens GET,PATCH,DELETE /gardens/:id
GET /gardens/:id/full ← one-shot editor load: garden + objects + plantings + referenced plants
POST /gardens/:id/copy ← deep-copy a garden you own (objects + active plops; not shares/link)
POST /gardens/:id/objects PATCH,DELETE /objects/:id
POST /objects/:id/plantings PATCH,DELETE /plantings/:id
GET,POST /plants PATCH,DELETE /plants/:id (own plants only)
+1
View File
@@ -81,6 +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.GET("/:id/full", h.getGardenFull) // one-shot editor load
gardens.POST("/:id/objects", h.createObject)
+29
View File
@@ -2,6 +2,7 @@ package api
import (
"errors"
"io"
"net/http"
"github.com/gin-gonic/gin"
@@ -103,6 +104,34 @@ func (h *handlers) updateGarden(c *gin.Context) {
c.JSON(http.StatusOK, g)
}
// gardenCopyRequest is the (optional) body of POST /gardens/:id/copy. A blank or
// absent name lets the service derive "<source> (copy)".
type gardenCopyRequest struct {
Name string `json:"name"`
}
func (h *handlers) copyGarden(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
// The body is optional — POST with no body means "copy under the default
// 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 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 {
writeServiceError(c, err)
return
}
c.JSON(http.StatusCreated, g)
}
func (h *handlers) deleteGarden(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
+98
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"
@@ -181,3 +184,98 @@ func TestGardenUpdateRejectsBadVersion(t *testing.T) {
func gardenPath(id int64) string {
return "/api/v1/gardens/" + strconv.FormatInt(id, 10)
}
func TestCopyGardenEndpoint(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", "widthCm": 400, "heightCm": 400}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("create status = %d, body %s", w.Code, w.Body.String())
}
src := decodeMap(t, w.Body.Bytes())
id := int64(src["id"].(float64))
path := "/api/v1/gardens/" + strconv.FormatInt(id, 10) + "/copy"
// No body at all → 201 with the derived name.
w = doJSON(t, r, http.MethodPost, path, nil, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("copy status = %d, body %s", w.Code, w.Body.String())
}
dup := decodeMap(t, w.Body.Bytes())
if dup["name"] != "Home (copy)" {
t.Errorf("name = %v, want 'Home (copy)'", dup["name"])
}
if int64(dup["id"].(float64)) == id {
t.Error("copy reused the source id")
}
if dup["myRole"] != "owner" {
t.Errorf("myRole = %v, want owner", dup["myRole"])
}
// An explicit name is honored.
w = doJSON(t, r, http.MethodPost, path, map[string]any{"name": "Plan B"}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("named copy status = %d, body %s", w.Code, w.Body.String())
}
if got := decodeMap(t, w.Body.Bytes())["name"]; got != "Plan B" {
t.Errorf("name = %v, want 'Plan B'", got)
}
// Both copies plus the source show up in the list.
w = doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, cookie)
var list []map[string]any
if err := json.Unmarshal(w.Body.Bytes(), &list); err != nil {
t.Fatalf("decode list: %v", err)
}
if len(list) != 3 {
t.Errorf("list has %d gardens, want 3", len(list))
}
// Unauthenticated → 401; someone else's garden → 404.
if w := doJSON(t, r, http.MethodPost, path, nil, nil); w.Code != http.StatusUnauthorized {
t.Errorf("anon copy status = %d, want 401", w.Code)
}
other := registerAndCookie(t, r, "[email protected]")
if w := doJSON(t, r, http.MethodPost, path, nil, other); w.Code != http.StatusNotFound {
t.Errorf("stranger copy status = %d, want 404", w.Code)
}
// A non-numeric id is a 400, and a malformed body is rejected.
if w := doJSON(t, r, http.MethodPost, "/api/v1/gardens/not-a-number/copy", nil, cookie); w.Code != http.StatusBadRequest {
t.Errorf("bad id status = %d, want 400", w.Code)
}
if w := doJSON(t, r, http.MethodPost, path, map[string]any{"name": 42}, cookie); w.Code != http.StatusBadRequest {
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)
}
}
+42
View File
@@ -156,6 +156,48 @@ func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in
return updated, err
}
// CopyGarden duplicates a garden into a new one owned by the actor, carrying
// over its dimensions, grid settings, objects and currently-planted plops. A
// blank name defaults to "<source> (copy)".
//
// Owner-only, deliberately: a copy's plops keep pointing at the SOURCE's
// plant_ids, and a custom plant is owned by one user. Letting a viewer copy
// someone else's garden would hand them a garden referencing plants that aren't
// in their catalog — and would block the original owner from ever deleting those
// plants (plantings.plant_id is ON DELETE RESTRICT). Copying a shared garden
// needs a plant-cloning policy first; until then the owner's own copy is the
// case that's unambiguously correct.
func (s *Service) CopyGarden(ctx context.Context, actorID, gardenID int64, name string) (*domain.Garden, error) {
src, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner)
if err != nil {
return nil, err
}
name = strings.TrimSpace(name)
if name == "" {
name = copyName(src.Name)
}
if len(name) > maxGardenNameLen {
return nil, domain.ErrInvalidInput
}
created, err := s.store.CopyGarden(ctx, gardenID, actorID, name)
if err != nil {
return nil, err
}
created.MyRole = roleOwner.String() // the copier owns the copy
return created, nil
}
// copyName derives the default name for a copy. A name already at the length cap
// is trimmed to make room for the suffix, on a rune boundary so the result stays
// valid UTF-8.
func copyName(src string) string {
const suffix = " (copy)"
if room := maxGardenNameLen - len(suffix); len(src) > room {
src = strings.TrimSpace(strings.ToValidUTF8(src[:room], ""))
}
return src + suffix
}
// DeleteGarden removes a garden; only the owner may.
func (s *Service) DeleteGarden(ctx context.Context, actorID, gardenID int64) error {
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil {
+279
View File
@@ -6,6 +6,7 @@ import (
"math"
"strings"
"testing"
"unicode/utf8"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
@@ -249,3 +250,281 @@ func TestDeleteGarden(t *testing.T) {
t.Errorf("re-delete err = %v, want ErrNotFound", err)
}
}
func TestCopyGardenDuplicatesContents(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
src, err := s.CreateGarden(ctx, owner, GardenInput{
Name: "Home", WidthCM: 732, HeightCM: 732, UnitPref: domain.UnitImperial,
Notes: "the real one", GridSizeCM: 30, SnapToGrid: true,
})
if err != nil {
t.Fatalf("CreateGarden: %v", err)
}
bed := seedBed(t, s, owner, src.ID)
plant := seedOwnPlant(t, s, owner, 20)
plop, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, XCM: 10, YCM: -20, RadiusCM: 15})
if err != nil {
t.Fatalf("CreatePlanting: %v", err)
}
dup, err := s.CopyGarden(ctx, owner, src.ID, "")
if err != nil {
t.Fatalf("CopyGarden: %v", err)
}
if dup.ID == src.ID {
t.Fatal("copy reused the source id")
}
if dup.Name != "Home (copy)" {
t.Errorf("name = %q, want 'Home (copy)'", dup.Name)
}
if dup.OwnerID != owner {
t.Errorf("owner = %d, want %d", dup.OwnerID, owner)
}
if dup.Version != 1 {
t.Errorf("version = %d, want a fresh 1", dup.Version)
}
if dup.WidthCM != src.WidthCM || dup.HeightCM != src.HeightCM ||
dup.UnitPref != src.UnitPref || dup.Notes != src.Notes ||
dup.GridSizeCM != src.GridSizeCM || dup.SnapToGrid != src.SnapToGrid {
t.Errorf("settings not carried over: %+v vs source %+v", dup, src)
}
full, err := s.GardenFull(ctx, owner, dup.ID)
if err != nil {
t.Fatalf("GardenFull(copy): %v", err)
}
if len(full.Objects) != 1 {
t.Fatalf("copy has %d objects, want 1", len(full.Objects))
}
got := full.Objects[0]
if got.ID == bed.ID {
t.Error("copied object reused the source object id")
}
if got.GardenID != dup.ID {
t.Errorf("copied object garden = %d, want the copy %d", got.GardenID, dup.ID)
}
if got.Kind != bed.Kind || got.XCM != bed.XCM || got.YCM != bed.YCM ||
got.WidthCM != bed.WidthCM || got.HeightCM != bed.HeightCM || got.Plantable != bed.Plantable {
t.Errorf("copied object differs: %+v vs source %+v", got, bed)
}
if len(full.Plantings) != 1 {
t.Fatalf("copy has %d plantings, want 1", len(full.Plantings))
}
gotPlop := full.Plantings[0]
if gotPlop.ID == plop.ID {
t.Error("copied planting reused the source planting id")
}
if gotPlop.ObjectID != got.ID {
t.Errorf("copied planting parent = %d, want the copied object %d", gotPlop.ObjectID, got.ID)
}
if gotPlop.PlantID != plop.PlantID || gotPlop.XCM != plop.XCM || gotPlop.YCM != plop.YCM ||
gotPlop.RadiusCM != plop.RadiusCM {
t.Errorf("copied planting differs: %+v vs source %+v", gotPlop, plop)
}
// The source is untouched by the copy.
srcFull, err := s.GardenFull(ctx, owner, src.ID)
if err != nil {
t.Fatalf("GardenFull(source): %v", err)
}
if len(srcFull.Objects) != 1 || len(srcFull.Plantings) != 1 {
t.Errorf("source changed: %d objects, %d plantings", len(srcFull.Objects), len(srcFull.Plantings))
}
}
func TestCopyGardenSkipsRemovedPlantings(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
src := seedGarden(t, s, owner)
bed := seedBed(t, s, owner, src.ID)
plant := seedOwnPlant(t, s, owner, 20)
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)
}
// Clear the bed, then re-plant one plop: the copy should carry the active plop
// only, not the soft-removed history.
if _, err := s.ClearObject(ctx, owner, bed.ID); err != nil {
t.Fatalf("ClearObject: %v", err)
}
replanted, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, XCM: 25, RadiusCM: 10})
if err != nil {
t.Fatalf("CreatePlanting(replanted): %v", err)
}
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 {
t.Fatalf("CopyGarden: %v", err)
}
full, err := s.GardenFull(ctx, owner, dup.ID)
if err != nil {
t.Fatalf("GardenFull: %v", err)
}
if len(full.Plantings) != 1 {
t.Fatalf("copy has %d plantings, want only the 1 active one", len(full.Plantings))
}
if full.Plantings[0].XCM != replanted.XCM {
t.Errorf("copied plop x = %v, want the active %v", full.Plantings[0].XCM, replanted.XCM)
}
}
func TestCopyGardenNaming(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, err := s.CreateGarden(ctx, owner, GardenInput{Name: "Yard"})
if err != nil {
t.Fatalf("CreateGarden: %v", err)
}
// An explicit name wins, and is trimmed like any other garden name.
dup, err := s.CopyGarden(ctx, owner, g.ID, " Experiment ")
if err != nil {
t.Fatalf("CopyGarden: %v", err)
}
if dup.Name != "Experiment" {
t.Errorf("name = %q, want trimmed 'Experiment'", dup.Name)
}
// An over-long explicit name is rejected rather than silently truncated.
if _, err := s.CopyGarden(ctx, owner, g.ID, strings.Repeat("a", maxGardenNameLen+1)); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("long name err = %v, want ErrInvalidInput", err)
}
// A source already at the cap still yields a valid derived name.
long, err := s.CreateGarden(ctx, owner, GardenInput{Name: strings.Repeat("b", maxGardenNameLen)})
if err != nil {
t.Fatalf("CreateGarden(long): %v", err)
}
dupLong, err := s.CopyGarden(ctx, owner, long.ID, "")
if err != nil {
t.Fatalf("CopyGarden(long): %v", err)
}
if len(dupLong.Name) > maxGardenNameLen {
t.Errorf("derived name is %d bytes, over the %d cap", len(dupLong.Name), maxGardenNameLen)
}
if !strings.HasSuffix(dupLong.Name, " (copy)") {
t.Errorf("derived name = %q, want a ' (copy)' suffix", dupLong.Name)
}
}
func TestCopyGardenNameRuneBoundary(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
// A name at the cap made of multi-byte runes: truncating for the suffix must
// not split one and leave invalid UTF-8.
name := strings.Repeat("é", maxGardenNameLen/2) // 2 bytes each == the cap
g, err := s.CreateGarden(ctx, owner, GardenInput{Name: name})
if err != nil {
t.Fatalf("CreateGarden: %v", err)
}
dup, err := s.CopyGarden(ctx, owner, g.ID, "")
if err != nil {
t.Fatalf("CopyGarden: %v", err)
}
if !utf8.ValidString(dup.Name) {
t.Errorf("derived name %q is not valid UTF-8", dup.Name)
}
if len(dup.Name) > maxGardenNameLen {
t.Errorf("derived name is %d bytes, over the %d cap", len(dup.Name), maxGardenNameLen)
}
}
func TestCopyGardenPermissions(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
alice := seedUser(t, s, "[email protected]")
bob := seedUser(t, s, "[email protected]")
g := seedGarden(t, s, alice)
// A stranger can't see the garden at all, let alone copy it.
if _, err := s.CopyGarden(ctx, bob, g.ID, ""); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("stranger copy err = %v, want ErrNotFound", err)
}
// An editor may edit contents but not duplicate the garden (see CopyGarden's
// note on cross-owner plant references).
if _, err := s.AddShare(ctx, alice, g.ID, "[email protected]", domain.RoleEditor); err != nil {
t.Fatalf("AddShare: %v", err)
}
if _, err := s.CopyGarden(ctx, bob, g.ID, ""); !errors.Is(err, domain.ErrForbidden) {
t.Errorf("editor copy err = %v, want ErrForbidden", err)
}
// A missing garden is ErrNotFound.
if _, err := s.CopyGarden(ctx, alice, g.ID+9999, ""); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("missing garden err = %v, want ErrNotFound", err)
}
}
func TestCopyGardenDoesNotInheritPublicLink(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g := seedGarden(t, s, owner)
link, err := s.EnablePublicShareLink(ctx, owner, g.ID, false)
if err != nil {
t.Fatalf("EnablePublicShareLink: %v", err)
}
if link.Token == "" {
t.Fatal("no token issued")
}
dup, err := s.CopyGarden(ctx, owner, g.ID, "")
if err != nil {
t.Fatalf("CopyGarden: %v", err)
}
// The copy must start private: no token of its own...
dupLink, err := s.GetPublicShareLink(ctx, owner, dup.ID)
if err != nil {
t.Fatalf("GetPublicShareLink(copy): %v", err)
}
if dupLink.Token != "" {
t.Errorf("copy inherited a public token %q", dupLink.Token)
}
// ...and the source's token still resolves to the source, not the copy.
pub, err := s.PublicGarden(ctx, link.Token)
if err != nil {
t.Fatalf("PublicGarden: %v", err)
}
if pub.Garden.ID != g.ID {
t.Errorf("source token resolves to garden %d, want the source %d", pub.Garden.ID, g.ID)
}
}
func TestCopyGardenDoesNotInheritShares(t *testing.T) {
ctx := context.Background()
s := newTestService(t, openConfig())
alice := seedUser(t, s, "[email protected]")
bob := seedUser(t, s, "[email protected]")
g := seedGarden(t, s, alice)
if _, err := s.AddShare(ctx, alice, g.ID, "[email protected]", domain.RoleEditor); err != nil {
t.Fatalf("AddShare: %v", err)
}
dup, err := s.CopyGarden(ctx, alice, g.ID, "")
if err != nil {
t.Fatalf("CopyGarden: %v", err)
}
// Bob keeps his access to the original but gets none to the copy.
if _, err := s.GetGarden(ctx, bob, g.ID); err != nil {
t.Errorf("bob lost access to the source: %v", err)
}
if _, err := s.GetGarden(ctx, bob, dup.ID); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("bob can see the copy: err = %v, want ErrNotFound", err)
}
}
+82
View File
@@ -163,6 +163,88 @@ func (d *DB) DeleteGarden(ctx context.Context, id int64) error {
return nil
}
// CopyGarden deep-copies garden srcID into a new garden owned by ownerID and
// named name, returning the new row. Objects are copied with their geometry,
// grid settings and z-order, and each object's ACTIVE plantings (removed_at IS
// NULL) are re-parented onto the copied object.
//
// Deliberately not copied:
// - public_token — the share link is a capability granted to the original, so a
// copy must not silently inherit a live public URL;
// - garden_shares — a copy is private to whoever made it;
// - removed plantings — the copy is a fresh layout, not the original's history.
//
// The whole copy runs in one transaction, so a failure part-way leaves no
// half-populated garden behind. Returns domain.ErrNotFound if srcID is gone.
func (d *DB) CopyGarden(ctx context.Context, srcID, ownerID int64, name string) (*domain.Garden, error) {
tx, err := d.sql.BeginTx(ctx, nil)
if err != nil {
return nil, fmt.Errorf("store: begin copy garden tx: %w", err)
}
defer tx.Rollback() //nolint:errcheck // no-op after a successful commit
// INSERT ... SELECT copies the source's own columns; owner and name are
// supplied. If the source is gone the SELECT is empty, nothing is inserted,
// and RETURNING yields no rows.
created, err := scanGarden(tx.QueryRowContext(ctx,
`INSERT INTO gardens (owner_id, name, width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid)
SELECT ?, ?, width_cm, height_cm, unit_pref, notes, grid_size_cm, snap_to_grid
FROM gardens WHERE id = ?
RETURNING `+gardenColumns,
ownerID, name, srcID,
))
if errors.Is(err, sql.ErrNoRows) {
return nil, domain.ErrNotFound
}
if err != nil {
return nil, fmt.Errorf("store: copy garden: %w", err)
}
// Read the source's objects fully before inserting: SQLite can't have an open
// cursor and a write on the same connection, and the pool is pinned to one
// connection for in-memory DBs.
srcObjects, err := queryObjects(ctx, tx,
`SELECT `+objectColumns+` FROM garden_objects WHERE garden_id = ? ORDER BY id`, srcID)
if err != nil {
return nil, err
}
// objectIDs maps a source object id to its copy, so plantings can be
// re-parented onto the right new object.
objectIDs := make(map[int64]int64, len(srcObjects))
for _, o := range srcObjects {
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)
}
objectIDs[o.ID] = copied.ID
}
srcPlantings, err := queryPlantings(ctx, tx,
`SELECT `+qualifyColumns("pl", plantingColumns)+` FROM plantings pl
JOIN garden_objects o ON o.id = pl.object_id
WHERE o.garden_id = ? AND pl.removed_at IS NULL
ORDER BY pl.id`, srcID)
if err != nil {
return nil, err
}
for _, p := range srcPlantings {
objectID, ok := objectIDs[p.ObjectID]
if !ok {
// 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 := scanPlanting(tx.QueryRowContext(ctx, plantingInsert, plantingInsertArgs(objectID, &p)...)); err != nil {
return nil, fmt.Errorf("store: copy planting: %w", err)
}
}
if err := tx.Commit(); err != nil {
return nil, fmt.Errorf("store: commit garden copy: %w", err)
}
return created, nil
}
// GetGardenByPublicToken returns the garden whose public_token matches, or
// domain.ErrNotFound. Possession of the token is the capability, so there is no
// owner/actor check here — the service exposes this only for the unauthenticated
+31 -13
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)
}
@@ -66,10 +77,17 @@ func (d *DB) GetObject(ctx context.Context, id int64) (*domain.GardenObject, err
// ListObjectsForGarden returns a garden's objects, bottom-to-top (z_index then
// id). Always a non-nil slice.
func (d *DB) ListObjectsForGarden(ctx context.Context, gardenID int64) ([]domain.GardenObject, error) {
rows, err := d.sql.QueryContext(ctx,
return queryObjects(ctx, d.sql,
`SELECT `+objectColumns+` FROM garden_objects WHERE garden_id = ? ORDER BY z_index, id`,
gardenID,
)
gardenID)
}
// queryObjects runs an object query and scans every row. The result set is fully
// drained and the cursor closed before returning, so a caller inside a
// transaction may write on the same connection afterwards. Always a non-nil
// slice.
func queryObjects(ctx context.Context, q queryer, query string, args ...any) ([]domain.GardenObject, error) {
rows, err := q.QueryContext(ctx, query, args...)
if err != nil {
return nil, fmt.Errorf("store: list objects: %w", err)
}
+29 -36
View File
@@ -30,13 +30,19 @@ func scanPlanting(s scanner) (*domain.Planting, error) {
// IS NULL) across all objects in a garden — the editor's one-shot load. Always a
// 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,
return queryPlantings(ctx, d.sql,
`SELECT `+qualifyColumns("pl", plantingColumns)+` FROM plantings pl
JOIN garden_objects o ON o.id = pl.object_id
WHERE o.garden_id = ? AND pl.removed_at IS NULL
ORDER BY pl.id`,
gardenID,
)
gardenID)
}
// queryPlantings runs a planting query and scans every row. Like queryObjects it
// drains and closes the cursor before returning, so a transaction caller may
// write afterwards. Always a non-nil slice.
func queryPlantings(ctx context.Context, q queryer, query string, args ...any) ([]domain.Planting, error) {
rows, err := q.QueryContext(ctx, query, args...)
if err != nil {
return nil, fmt.Errorf("store: list plantings: %w", err)
}
@@ -60,27 +66,9 @@ func (d *DB) ListActivePlantingsForGarden(ctx context.Context, gardenID int64) (
// (removed_at IS NULL). Always a non-nil slice. Used by FillRegion to avoid
// stacking new plops inside existing ones.
func (d *DB) ListActivePlantingsForObject(ctx context.Context, objectID int64) ([]domain.Planting, error) {
rows, err := d.sql.QueryContext(ctx,
return queryPlantings(ctx, d.sql,
`SELECT `+plantingColumns+` FROM plantings WHERE object_id = ? AND removed_at IS NULL ORDER BY id`,
objectID,
)
if err != nil {
return nil, fmt.Errorf("store: list object plantings: %w", err)
}
defer rows.Close()
plantings := []domain.Planting{}
for rows.Next() {
p, err := scanPlanting(rows)
if err != nil {
return nil, fmt.Errorf("store: scan planting: %w", err)
}
plantings = append(plantings, *p)
}
if err := rows.Err(); err != nil {
return nil, fmt.Errorf("store: iterate plantings: %w", err)
}
return plantings, nil
objectID)
}
// ClearObjectPlantings soft-removes every active plop in an object in one UPDATE
@@ -116,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)
}
@@ -144,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.
@@ -0,0 +1,64 @@
import { useState } from 'react'
import { useNavigate } from '@tanstack/react-router'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { Modal } from '@/components/ui/Modal'
import { TextField } from '@/components/ui/TextField'
import { toast } from '@/components/ui/toast'
import { errorMessage } from '@/lib/api'
import { defaultCopyName, useCopyGarden, type Garden } from '@/lib/gardens'
/**
* Duplicate a garden under a new name. The name is prefilled with the server's
* own default so what you see is what you get; the beds and everything currently
* planted in them come along, while the source's share link and shares do not.
* On success we land in the copy — the point of copying is to start editing it.
*/
export function CopyGardenModal({ garden, onClose }: { garden: Garden; onClose: () => void }) {
const copy = useCopyGarden()
const navigate = useNavigate()
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) {
e.preventDefault()
setError(null)
try {
const created = await copy.mutateAsync({ id: garden.id, name: name.trim() })
toast.info(`Copied to “${created.name}”.`)
onClose()
navigate({ to: '/gardens/$gardenId', params: { gardenId: String(created.id) } })
} catch (err) {
setError(errorMessage(err, 'Could not copy the garden.'))
}
}
return (
<Modal title="Copy garden" onClose={onClose} busy={copy.isPending}>
<form onSubmit={onSubmit} className="flex flex-col gap-4">
<p className="text-sm text-muted">
Make a copy of <span className="font-medium text-fg">{garden.name}</span> with its beds and
everything planted in them. The copy is private the original's share link and people you've
shared it with aren't carried over.
</p>
<TextField
label="Name"
name="name"
required
autoFocus
value={name}
onChange={(e) => setName(e.target.value)}
/>
{error && <Alert>{error}</Alert>}
<div className="flex justify-end gap-2">
<Button type="button" variant="ghost" onClick={onClose} disabled={copy.isPending}>
Cancel
</Button>
<Button type="submit" disabled={copy.isPending || name.trim() === ''}>
{copy.isPending ? 'Copying' : 'Copy garden'}
</Button>
</div>
</form>
</Modal>
)
}
+8 -2
View File
@@ -5,14 +5,16 @@ import { cardActionClass, cardDangerClass } from '@/components/ui/cardActions'
/**
* One garden as a card: the body links into the editor. The footer differs by
* role — the owner gets Share / Edit / Delete; a recipient sees a "shared · role"
* badge and a Leave action (garden metadata edit + sharing are owner-only).
* role — the owner gets Share / Copy / Edit / Delete; a recipient sees a
* "shared · role" badge and a Leave action (garden metadata edit, sharing and
* copying are owner-only).
* Ownership is the authoritative ownerId==me check, not the my_role hint.
*/
export function GardenCard({
garden,
currentUserId,
onShare,
onCopy,
onEdit,
onDelete,
onLeave,
@@ -20,6 +22,7 @@ export function GardenCard({
garden: Garden
currentUserId?: number
onShare: () => void
onCopy: () => void
onEdit: () => void
onDelete: () => void
onLeave: () => void
@@ -51,6 +54,9 @@ export function GardenCard({
<button type="button" onClick={onShare} className={cardActionClass}>
Share
</button>
<button type="button" onClick={onCopy} className={cardActionClass}>
Copy
</button>
<button type="button" onClick={onEdit} className={cardActionClass}>
Edit
</button>
+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)')
})
})
+44
View File
@@ -86,6 +86,50 @@ 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
* not inherit the source's public link or shares.
*/
export function useCopyGarden() {
const qc = useQueryClient()
return useMutation({
mutationFn: async ({ id, name }: { id: number; name?: string }): Promise<Garden> =>
gardenSchema.parse(await api.post(`/gardens/${id}/copy`, { name: name ?? '' })),
onSuccess: () => qc.invalidateQueries({ queryKey: gardensKey }),
})
}
export function useDeleteGarden() {
const qc = useQueryClient()
return useMutation({
+5 -1
View File
@@ -1,6 +1,7 @@
import { useState } from 'react'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { CopyGardenModal } from '@/components/gardens/CopyGardenModal'
import { DeleteGardenModal } from '@/components/gardens/DeleteGardenModal'
import { GardenCard } from '@/components/gardens/GardenCard'
import { GardenFormModal } from '@/components/gardens/GardenFormModal'
@@ -10,12 +11,13 @@ import { useMe } from '@/lib/auth'
import { useGardens, type Garden } from '@/lib/gardens'
import { usePageTitle } from '@/lib/usePageTitle'
// Which modal is open, if any. edit/delete/share/leave carry the target garden.
// Which modal is open, if any. edit/delete/share/copy/leave carry the target garden.
type Dialog =
| { kind: 'create' }
| { kind: 'edit'; garden: Garden }
| { kind: 'delete'; garden: Garden }
| { kind: 'share'; garden: Garden }
| { kind: 'copy'; garden: Garden }
| { kind: 'leave'; garden: Garden }
| null
@@ -56,6 +58,7 @@ export function GardensPage() {
garden={g}
currentUserId={me.data?.id}
onShare={() => setDialog({ kind: 'share', garden: g })}
onCopy={() => setDialog({ kind: 'copy', garden: g })}
onEdit={() => setDialog({ kind: 'edit', garden: g })}
onDelete={() => setDialog({ kind: 'delete', garden: g })}
onLeave={() => setDialog({ kind: 'leave', garden: g })}
@@ -69,6 +72,7 @@ export function GardensPage() {
{dialog?.kind === 'edit' && <GardenFormModal garden={dialog.garden} onClose={close} />}
{dialog?.kind === 'delete' && <DeleteGardenModal garden={dialog.garden} onClose={close} />}
{dialog?.kind === 'share' && <ShareGardenModal garden={dialog.garden} onClose={close} />}
{dialog?.kind === 'copy' && <CopyGardenModal garden={dialog.garden} onClose={close} />}
{dialog?.kind === 'leave' && <LeaveGardenModal garden={dialog.garden} onClose={close} />}
</section>
)