diff --git a/internal/api/api.go b/internal/api/api.go index e742187..8fe83d8 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -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) diff --git a/internal/api/gardens.go b/internal/api/gardens.go index 584adc2..233bff0 100644 --- a/internal/api/gardens.go +++ b/internal/api/gardens.go @@ -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 + // 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 { diff --git a/internal/api/gardens_test.go b/internal/api/gardens_test.go index 1c37240..b272ac1 100644 --- a/internal/api/gardens_test.go +++ b/internal/api/gardens_test.go @@ -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, "a@example.com") + + 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) + } +} diff --git a/internal/service/gardens_test.go b/internal/service/gardens_test.go index 09c07a3..d9d4fc2 100644 --- a/internal/service/gardens_test.go +++ b/internal/service/gardens_test.go @@ -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 { + 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, "") if err != nil { diff --git a/internal/store/gardens.go b/internal/store/gardens.go index e89e9ea..20e5b8a 100644 --- a/internal/store/gardens.go +++ b/internal/store/gardens.go @@ -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)...)) 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) } } diff --git a/internal/store/objects.go b/internal/store/objects.go index 05c467c..03cf57b 100644 --- a/internal/store/objects.go +++ b/internal/store/objects.go @@ -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) } diff --git a/internal/store/plantings.go b/internal/store/plantings.go index 6f8f45a..1af1c9b 100644 --- a/internal/store/plantings.go +++ b/internal/store/plantings.go @@ -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) } diff --git a/internal/store/sqlite.go b/internal/store/sqlite.go index ce79897..cec5e35 100644 --- a/internal/store/sqlite.go +++ b/internal/store/sqlite.go @@ -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. diff --git a/internal/store/users.go b/internal/store/users.go index ad5244b..bceef92 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -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 // returns it as int64, which does not convert straight to bool), so it is read // into an int64 and mapped. diff --git a/web/src/components/gardens/CopyGardenModal.tsx b/web/src/components/gardens/CopyGardenModal.tsx index 5535507..0e0637c 100644 --- a/web/src/components/gardens/CopyGardenModal.tsx +++ b/web/src/components/gardens/CopyGardenModal.tsx @@ -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)) const [error, setError] = useState(null) async function onSubmit(e: React.FormEvent) { diff --git a/web/src/lib/gardens.test.ts b/web/src/lib/gardens.test.ts new file mode 100644 index 0000000..6e32b53 --- /dev/null +++ b/web/src/lib/gardens.test.ts @@ -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)') + }) +}) diff --git a/web/src/lib/gardens.ts b/web/src/lib/gardens.ts index 774627f..8ce8ff9 100644 --- a/web/src/lib/gardens.ts +++ b/web/src/lib/gardens.ts @@ -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 " (copy)". The copy does