Gardens CRUD + service-layer conventions (actor, version guard, 409) (#7) #26

Merged
steve merged 2 commits from phase-2-gardens-api into main 2026-07-18 22:40:50 +00:00
6 changed files with 814 additions and 0 deletions
Showing only changes of commit f39ed52868 - Show all commits
+9
View File
@@ -73,6 +73,15 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
slog.Warn("api: OIDC is configured but PANSY_BASE_URL is unset; OIDC disabled (an absolute redirect URI is required)") slog.Warn("api: OIDC is configured but PANSY_BASE_URL is unset; OIDC disabled (an absolute redirect URI is required)")
} }
// Feature resources sit behind requireAuth, which resolves the session cookie
// to the actor the service layer's permission checks key off.
gardens := v1.Group("/gardens", h.requireAuth())
gardens.GET("", h.listGardens)
gardens.POST("", h.createGarden)
gardens.GET("/:id", h.getGarden)
gardens.PATCH("/:id", h.updateGarden)
gardens.DELETE("/:id", h.deleteGarden)
return r return r
} }
+158
View File
@@ -0,0 +1,158 @@
package api
import (
"errors"
"log/slog"
"net/http"
"strconv"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// gardenCreateRequest / gardenUpdateRequest are the JSON bodies for the gardens
// endpoints. Dimensions are centimeters (the API is metric-only; imperial is a
// display concern). On create, omitted dimensions default server-side; on
// update, version is required and every field is replaced.
type gardenCreateRequest struct {
Review

🟠 gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods

maintainability · flagged by 1 model

  • internal/api/gardens.go:19-41gardenCreateRequest and gardenUpdateRequest duplicate five identical fields and their toInput() methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., gardenFields) and embed it, or make toInput() a method on the base so the translation lives in one place.

🪰 Gadfly · advisory

🟠 **gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:19-41`** — `gardenCreateRequest` and `gardenUpdateRequest` duplicate five identical fields and their `toInput()` methods are byte-for-byte copies. This is unnecessary duplication within the same file; adding a field in the future requires touching four places. Use a shared base struct (e.g., `gardenFields`) and embed it, or make `toInput()` a method on the base so the translation lives in one place. <sub>🪰 Gadfly · advisory</sub>
Name string `json:"name" binding:"required"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
UnitPref string `json:"unitPref"`
Notes string `json:"notes"`
}
type gardenUpdateRequest struct {
Name string `json:"name" binding:"required"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
UnitPref string `json:"unitPref"`
Notes string `json:"notes"`
Version int64 `json:"version" binding:"required"`
}
func (r gardenCreateRequest) toInput() service.GardenInput {
return service.GardenInput{Name: r.Name, WidthCM: r.WidthCM, HeightCM: r.HeightCM, UnitPref: r.UnitPref, Notes: r.Notes}
}
func (r gardenUpdateRequest) toInput() service.GardenInput {
return service.GardenInput{Name: r.Name, WidthCM: r.WidthCM, HeightCM: r.HeightCM, UnitPref: r.UnitPref, Notes: r.Notes}
}
// listGardens returns the actor's gardens as a JSON array (always an array,
// never null).
func (h *handlers) listGardens(c *gin.Context) {
gardens, err := h.svc.ListGardens(c.Request.Context(), mustActor(c).ID)
if err != nil {
writeResourceError(c, err)
return
}
c.JSON(http.StatusOK, gardens)
}
func (h *handlers) createGarden(c *gin.Context) {
var req gardenCreateRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a garden name is required")
return
}
g, err := h.svc.CreateGarden(c.Request.Context(), mustActor(c).ID, req.toInput())
if err != nil {
writeResourceError(c, err)
return
}
c.JSON(http.StatusCreated, g)
}
func (h *handlers) getGarden(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
g, err := h.svc.GetGarden(c.Request.Context(), mustActor(c).ID, id)
if err != nil {
writeResourceError(c, err)
return
}
c.JSON(http.StatusOK, g)
}
func (h *handlers) updateGarden(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
var req gardenUpdateRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "name and a current version are required")
return
}
g, err := h.svc.UpdateGarden(c.Request.Context(), mustActor(c).ID, id, req.toInput(), req.Version)
if err != nil {
// On a version conflict the service returns the current row so the client
// can rebase; everything else is a plain error.
if errors.Is(err, domain.ErrVersionConflict) {
writeVersionConflict(c, g)
return
}
writeResourceError(c, err)
return
}
c.JSON(http.StatusOK, g)
}
func (h *handlers) deleteGarden(c *gin.Context) {
id, ok := parseIDParam(c, "id")
if !ok {
return
}
if err := h.svc.DeleteGarden(c.Request.Context(), mustActor(c).ID, id); err != nil {
writeResourceError(c, err)
return
}
c.Status(http.StatusNoContent)
}
// parseIDParam reads a positive int64 path parameter, writing a 400 and
// returning ok=false on a malformed value.
func parseIDParam(c *gin.Context, name string) (int64, bool) {
Review

🟠 parseIDParam is a generic utility placed in a feature-specific file

maintainability · flagged by 1 model

  • internal/api/gardens.go:119-126parseIDParam is a generic path-parameter utility (any :id route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to api.go (or a shared API util file) so every handler can reach it without cross-importing gardens code.

🪰 Gadfly · advisory

🟠 **parseIDParam is a generic utility placed in a feature-specific file** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:119-126`** — `parseIDParam` is a generic path-parameter utility (any `:id` route needs it), but it is defined in the gardens handler file. Future resource endpoints will either duplicate it or import it awkwardly from a feature-specific file. Move it to `api.go` (or a shared API util file) so every handler can reach it without cross-importing gardens code. <sub>🪰 Gadfly · advisory</sub>
id, err := strconv.ParseInt(c.Param(name), 10, 64)
if err != nil || id < 1 {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid id")
return 0, false
}
return id, true
}
// writeVersionConflict writes the 409 envelope for an optimistic-concurrency
// failure: the standard error object plus the current server row under
// "current", so the client can rebase its edit onto the fresh version and retry.
// This shape is the contract for every version-guarded (mutable) resource.
func writeVersionConflict(c *gin.Context, current any) {
c.JSON(http.StatusConflict, gin.H{
"error": gin.H{"code": "VERSION_CONFLICT", "message": "the resource was modified; refetch and retry"},
"current": current,
})
}
// writeResourceError maps the service-layer sentinel errors shared by every
Review

🟠 writeResourceError described as shared by every resource but defined in gardens.go

maintainability · flagged by 2 models

  • internal/api/gardens.go:139-157writeResourceError is described as "shared by every resource," yet it lives in gardens.go. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to writeAPIError in a central API file (e.g., api.go or spa.go) so later issues don't leave it stranded or duplicate it.

🪰 Gadfly · advisory

🟠 **writeResourceError described as shared by every resource but defined in gardens.go** _maintainability · flagged by 2 models_ - **`internal/api/gardens.go:139-157`** — `writeResourceError` is described as "shared by every resource," yet it lives in `gardens.go`. If it is truly meant to be shared by objects, plants, and every later resource, it should sit next to `writeAPIError` in a central API file (e.g., `api.go` or `spa.go`) so later issues don't leave it stranded or duplicate it. <sub>🪰 Gadfly · advisory</sub>
// resource to pansy's JSON error envelope. ErrNotFound is used both for a
// genuinely missing row and for one the actor may not see (existence is masked).
func writeResourceError(c *gin.Context, err error) {
switch {
case errors.Is(err, domain.ErrNotFound):
writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found")
case errors.Is(err, domain.ErrForbidden):
writeAPIError(c, http.StatusForbidden, "FORBIDDEN", "you don't have access")
case errors.Is(err, domain.ErrInvalidInput):
Review

🟡 writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError

maintainability · flagged by 1 model

  • internal/api/gardens.go:148-156 vs internal/api/auth.go:232-236 — duplicated error-mapping tail. writeResourceError and writeServiceError both map ErrInvalidInput → 400 INVALID_INPUT "invalid input" and share an identical default branch, including the verbatim slog.Error("api: unhandled service error", "error", err) line. The auth-specific sentinels genuinely belong only in writeServiceError, so the split is fine — but the shared tail (InvalidInput + default slog+500) could b…

🪰 Gadfly · advisory

🟡 **writeResourceError duplicates ErrInvalidInput mapping + default slog line from writeServiceError** _maintainability · flagged by 1 model_ - **`internal/api/gardens.go:148-156` vs `internal/api/auth.go:232-236` — duplicated error-mapping tail.** `writeResourceError` and `writeServiceError` both map `ErrInvalidInput → 400 INVALID_INPUT "invalid input"` and share an identical default branch, including the verbatim `slog.Error("api: unhandled service error", "error", err)` line. The auth-specific sentinels genuinely belong only in `writeServiceError`, so the split is fine — but the shared tail (InvalidInput + default slog+500) could b… <sub>🪰 Gadfly · advisory</sub>
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
case errors.Is(err, domain.ErrVersionConflict):
// Reached only if a caller forgot to special-case the conflict (which
// needs the current row); still return a coherent 409.
writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")
Review

🟡 409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **409 envelope shape diverges between writeVersionConflict and writeResourceError's ErrVersionConflict branch** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
default:
slog.Error("api: unhandled service error", "error", err)
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
}
}
+167
View File
@@ -0,0 +1,167 @@
package api
import (
"encoding/json"
"net/http"
"strconv"
"testing"
"github.com/gin-gonic/gin"
)
// registerAndCookie creates a user via the API and returns its session cookie.
func registerAndCookie(t *testing.T, r *gin.Engine, email string) *http.Cookie {
t.Helper()
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": email, "displayName": email, "password": "password123"}, nil)
if w.Code != http.StatusOK {
t.Fatalf("register %s: status %d, body %s", email, w.Code, w.Body.String())
}
return sessionCookieFrom(t, w)
}
func decodeGarden(t *testing.T, body []byte) map[string]any {
t.Helper()
var g map[string]any
if err := json.Unmarshal(body, &g); err != nil {
t.Fatalf("decode garden: %v (body %s)", err, body)
}
return g
}
func TestGardenCRUDFlow(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
// Create (defaults applied) → 201.
w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"name": "Backyard"}, cookie)
if w.Code != http.StatusCreated {
t.Fatalf("create status = %d, body %s", w.Code, w.Body.String())
}
created := decodeGarden(t, w.Body.Bytes())
id := int64(created["id"].(float64))
if created["widthCm"].(float64) != 1000 || created["unitPref"].(string) != "metric" {
t.Errorf("defaults not applied: %+v", created)
}
// List → array with one garden.
w = doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("list status = %d", w.Code)
}
var list []map[string]any
if err := json.Unmarshal(w.Body.Bytes(), &list); err != nil {
t.Fatalf("decode list: %v (body %s)", err, w.Body.String())
}
if len(list) != 1 {
t.Errorf("list len = %d, want 1", len(list))
}
// Get → 200.
w = doJSON(t, r, http.MethodGet, gardenPath(id), nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("get status = %d", w.Code)
}
// Patch with the current version → 200, version bumped.
w = doJSON(t, r, http.MethodPatch, gardenPath(id),
map[string]any{"name": "Front", "widthCm": 200, "heightCm": 400, "unitPref": "imperial", "version": 1}, cookie)
if w.Code != http.StatusOK {
t.Fatalf("patch status = %d, body %s", w.Code, w.Body.String())
}
patched := decodeGarden(t, w.Body.Bytes())
if patched["name"].(string) != "Front" || patched["version"].(float64) != 2 {
t.Errorf("patch didn't persist/bump: %+v", patched)
}
// Delete → 204, then get → 404.
w = doJSON(t, r, http.MethodDelete, gardenPath(id), nil, cookie)
if w.Code != http.StatusNoContent {
t.Fatalf("delete status = %d", w.Code)
}
w = doJSON(t, r, http.MethodGet, gardenPath(id), nil, cookie)
if w.Code != http.StatusNotFound {
t.Errorf("get after delete = %d, want 404", w.Code)
}
}
func TestGardenVersionConflictEnvelope(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": "Yard"}, cookie)
id := int64(decodeGarden(t, w.Body.Bytes())["id"].(float64))
body := map[string]any{"name": "Yard2", "widthCm": 1000, "heightCm": 1000, "unitPref": "metric", "version": 1}
// First patch at version 1 succeeds (→ version 2).
if w := doJSON(t, r, http.MethodPatch, gardenPath(id), body, cookie); w.Code != http.StatusOK {
t.Fatalf("first patch status = %d", w.Code)
}
// Second patch still at version 1 conflicts.
w = doJSON(t, r, http.MethodPatch, gardenPath(id), body, cookie)
if w.Code != http.StatusConflict {
t.Fatalf("stale patch status = %d, want 409", w.Code)
}
var env struct {
Error struct{ Code string } `json:"error"`
Current map[string]any `json:"current"`
}
if err := json.Unmarshal(w.Body.Bytes(), &env); err != nil {
t.Fatalf("decode conflict envelope: %v (body %s)", err, w.Body.String())
}
if env.Error.Code != "VERSION_CONFLICT" {
t.Errorf("error code = %q, want VERSION_CONFLICT", env.Error.Code)
}
if env.Current == nil || env.Current["version"].(float64) != 2 {
t.Errorf("conflict body missing current row at version 2: %+v", env.Current)
}
}
func TestGardenCrossUserIsNotFound(t *testing.T) {
r := authEngine(t, localCfg())
alice := registerAndCookie(t, r, "[email protected]")
bob := registerAndCookie(t, r, "[email protected]")
w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"name": "Alice's"}, alice)
id := int64(decodeGarden(t, w.Body.Bytes())["id"].(float64))
// Bob sees a 404 (existence masked), not a 403.
if w := doJSON(t, r, http.MethodGet, gardenPath(id), nil, bob); w.Code != http.StatusNotFound {
t.Errorf("bob get = %d, want 404", w.Code)
}
if w := doJSON(t, r, http.MethodDelete, gardenPath(id), nil, bob); w.Code != http.StatusNotFound {
t.Errorf("bob delete = %d, want 404", w.Code)
}
// Bob's own list is empty.
w = doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, bob)
if w.Body.String() != "[]" {
t.Errorf("bob list = %s, want []", w.Body.String())
}
}
func TestGardenRequiresAuth(t *testing.T) {
r := authEngine(t, localCfg())
if w := doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, nil); w.Code != http.StatusUnauthorized {
t.Errorf("unauthenticated list = %d, want 401", w.Code)
}
if w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"name": "X"}, nil); w.Code != http.StatusUnauthorized {
t.Errorf("unauthenticated create = %d, want 401", w.Code)
}
}
func TestGardenCreateValidation(t *testing.T) {
r := authEngine(t, localCfg())
cookie := registerAndCookie(t, r, "[email protected]")
// Missing name → 400.
if w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"widthCm": 100}, cookie); w.Code != http.StatusBadRequest {
t.Errorf("no-name create = %d, want 400", w.Code)
}
// Bad id path → 400.
if w := doJSON(t, r, http.MethodGet, "/api/v1/gardens/not-a-number", nil, cookie); w.Code != http.StatusBadRequest {
t.Errorf("bad id get = %d, want 400", w.Code)
}
}
func gardenPath(id int64) string {
return "/api/v1/gardens/" + strconv.FormatInt(id, 10)
}
+155
View File
@@ -0,0 +1,155 @@
package service
import (
"context"
"strings"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// Garden sizing bounds. A new garden with no dimensions defaults to a 10 m
// square; the max is a generous sanity cap (100 m) that also guards against
// absurd or overflow-y values.
const (
defaultGardenCM = 1000
maxGardenCM = 100_000
Review

🔴 maxGardenCM constant is 1 km instead of documented 100 m

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

  • internal/service/gardens.go:15maxGardenCM is 100_000 cm (1 km), but the comment and PR description both state a 100 m sanity cap. 100 m = 10,000 cm, so the constant is off by a factor of 10. This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap. Fix: Change maxGardenCM = 100_000 to maxGardenCM = 10_000.

🪰 Gadfly · advisory

🔴 **maxGardenCM constant is 1 km instead of documented 100 m** _correctness, error-handling, maintainability · flagged by 5 models_ - **`internal/service/gardens.go:15`** — `maxGardenCM` is `100_000` cm (1 km), but the comment and PR description both state a **100 m** sanity cap. 100 m = 10,000 cm, so the constant is off by a factor of 10. This means the validation happily accepts gardens up to 1 km in size, which violates the documented contract and defeats the purpose of the sanity cap. **Fix:** Change `maxGardenCM = 100_000` to `maxGardenCM = 10_000`. <sub>🪰 Gadfly · advisory</sub>
)
// gardenRole ranks a user's access to a garden. Higher includes lower
// (owner can do anything an editor can, etc.). Until sharing (#16), the only
// role anyone holds is owner, on their own gardens.
type gardenRole int
const (
roleNone gardenRole = iota
roleViewer
roleEditor
roleOwner
)
// GardenInput is the mutable field set for creating or updating a garden.
type GardenInput struct {
Name string
WidthCM float64
HeightCM float64
UnitPref string
Notes string
}
// requireGardenRole loads a garden and enforces that the actor holds at least
// min. It is THE place authorization for a garden is decided — REST handlers and
// future agent tools both funnel through here, so neither can skip a check.
//
// A user with no role at all gets ErrNotFound rather than ErrForbidden: we don't
// reveal that a garden exists to someone with no access. A user who has some
// access but not enough (e.g. a viewer trying to edit, once #16 lands) gets
// ErrForbidden. #16 extends effectiveRole to consult garden_shares.
func (s *Service) requireGardenRole(ctx context.Context, actorID, gardenID int64, min gardenRole) (*domain.Garden, error) {
g, err := s.store.GetGarden(ctx, gardenID)
if err != nil {
return nil, err // ErrNotFound or a real error
}
role := effectiveGardenRole(actorID, g)
if role == roleNone {
return nil, domain.ErrNotFound
}
if role < min {
return nil, domain.ErrForbidden
}
return g, nil
}
// effectiveGardenRole is the actor's role on a garden. Owner is implicit via
// gardens.owner_id; share-based viewer/editor roles are added in #16.
func effectiveGardenRole(actorID int64, g *domain.Garden) gardenRole {
if g.OwnerID == actorID {
return roleOwner
}
return roleNone
}
// CreateGarden creates a garden owned by the actor. Missing dimensions default
// to a 10 m square; unit defaults to metric.
func (s *Service) CreateGarden(ctx context.Context, actorID int64, in GardenInput) (*domain.Garden, error) {
g, err := gardenFromInput(in, true)
if err != nil {
return nil, err
}
g.OwnerID = actorID
return s.store.CreateGarden(ctx, g)
}
// GetGarden returns a garden the actor may at least view.
func (s *Service) GetGarden(ctx context.Context, actorID, gardenID int64) (*domain.Garden, error) {
return s.requireGardenRole(ctx, actorID, gardenID, roleViewer)
}
// ListGardens returns the gardens the actor can see. Owned-only until #16.
func (s *Service) ListGardens(ctx context.Context, actorID int64) ([]domain.Garden, error) {
return s.store.ListGardensForOwner(ctx, actorID)
}
// UpdateGarden applies a version-guarded update; the actor must be at least an
// editor. On a version mismatch it returns (current garden, ErrVersionConflict)
// so the handler can return the fresh row for the client to rebase.
func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in GardenInput, version int64) (*domain.Garden, error) {
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleEditor); err != nil {
return nil, err
}
g, err := gardenFromInput(in, false) // no defaults: an update states every field
if err != nil {
return nil, err
}
g.ID = gardenID
g.Version = version
return s.store.UpdateGarden(ctx, g)
}
// 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 {
return err
}
return s.store.DeleteGarden(ctx, gardenID)
}
// gardenFromInput validates and normalizes input into a domain.Garden (without
// identity/ownership fields). With applyDefaults, absent dimensions/unit are
// filled in (create); without it, every field must be supplied (update).
func gardenFromInput(in GardenInput, applyDefaults bool) (*domain.Garden, error) {
Review

🟠 Unbounded Name/Notes length from untrusted input (memory/storage DoS)

maintainability, security · flagged by 1 model

  • internal/service/gardens.go:119-154 — no length bound on Name/Notes from untrusted input. gardenFromInput only TrimSpaces and rejects empty; it never caps the length of Name or Notes. gardenCreateRequest/gardenUpdateRequest use only binding:"required" with no max= tag. I grepped the repo for any MaxBytesReader/global body limit and found none (the only max= binding in the codebase is on auth.go passwords), so an authenticated user can POST a garden with a multi-…

🪰 Gadfly · advisory

🟠 **Unbounded Name/Notes length from untrusted input (memory/storage DoS)** _maintainability, security · flagged by 1 model_ - **`internal/service/gardens.go:119-154` — no length bound on `Name`/`Notes` from untrusted input.** `gardenFromInput` only `TrimSpace`s and rejects empty; it never caps the length of `Name` or `Notes`. `gardenCreateRequest`/`gardenUpdateRequest` use only `binding:"required"` with no `max=` tag. I grepped the repo for any `MaxBytesReader`/global body limit and found none (the only `max=` binding in the codebase is on `auth.go` passwords), so an authenticated user can POST a garden with a multi-… <sub>🪰 Gadfly · advisory</sub>
name := strings.TrimSpace(in.Name)
if name == "" {
return nil, domain.ErrInvalidInput
}
// 0 means "unset": defaulted on create, rejected on update. A negative value
// is always an explicit error (never silently corrected to the default).
width, height := in.WidthCM, in.HeightCM
if applyDefaults {
if width == 0 {
width = defaultGardenCM
}
if height == 0 {
height = defaultGardenCM
}
}
if width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM {
return nil, domain.ErrInvalidInput
}
unit := in.UnitPref
if unit == "" && applyDefaults {
unit = domain.UnitMetric
}
if unit != domain.UnitMetric && unit != domain.UnitImperial {
return nil, domain.ErrInvalidInput
}
return &domain.Garden{
Name: name,
WidthCM: width,
HeightCM: height,
UnitPref: unit,
Notes: strings.TrimSpace(in.Notes),
}, nil
}
+193
View File
@@ -0,0 +1,193 @@
package service
import (
"context"
"errors"
"testing"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// seedUser registers a local user and returns its id.
func seedUser(t *testing.T, s *Service, email string) int64 {
t.Helper()
u := mustRegister(t, s, email, email, "password123")
return u.ID
}
func TestCreateGardenAppliesDefaults(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, err := s.CreateGarden(context.Background(), owner, GardenInput{Name: " Backyard "})
if err != nil {
t.Fatalf("CreateGarden: %v", err)
}
if g.Name != "Backyard" {
t.Errorf("name = %q, want trimmed 'Backyard'", g.Name)
}
if g.WidthCM != defaultGardenCM || g.HeightCM != defaultGardenCM {
t.Errorf("dimensions = %vx%v, want %d default", g.WidthCM, g.HeightCM, defaultGardenCM)
}
if g.UnitPref != domain.UnitMetric {
t.Errorf("unit = %q, want metric", g.UnitPref)
}
if g.OwnerID != owner {
t.Errorf("owner = %d, want %d", g.OwnerID, owner)
}
if g.Version != 1 {
t.Errorf("version = %d, want 1", g.Version)
}
}
func TestCreateGardenValidation(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
cases := []GardenInput{
{Name: " "}, // blank name
{Name: "X", UnitPref: "furlongs"}, // bad unit
{Name: "X", WidthCM: -5}, // non-positive dim (defaults only fill 0)
{Name: "X", WidthCM: maxGardenCM + 1}, // over the cap
{Name: "X", HeightCM: maxGardenCM * 10}, // over the cap
}
for i, in := range cases {
if _, err := s.CreateGarden(context.Background(), owner, in); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("case %d: err = %v, want ErrInvalidInput", i, err)
}
}
}
func TestListGardensOwnedOnly(t *testing.T) {
s := newTestService(t, openConfig())
alice := seedUser(t, s, "[email protected]")
bob := seedUser(t, s, "[email protected]")
if _, err := s.CreateGarden(context.Background(), alice, GardenInput{Name: "Alice A"}); err != nil {
t.Fatal(err)
}
if _, err := s.CreateGarden(context.Background(), alice, GardenInput{Name: "Alice B"}); err != nil {
t.Fatal(err)
}
if _, err := s.CreateGarden(context.Background(), bob, GardenInput{Name: "Bob A"}); err != nil {
t.Fatal(err)
}
aliceGardens, err := s.ListGardens(context.Background(), alice)
if err != nil {
t.Fatalf("ListGardens: %v", err)
}
if len(aliceGardens) != 2 {
t.Errorf("alice sees %d gardens, want 2", len(aliceGardens))
}
for _, g := range aliceGardens {
if g.OwnerID != alice {
t.Errorf("alice's list contains a garden owned by %d", g.OwnerID)
}
}
// A user with no gardens gets a non-nil empty list.
carol := seedUser(t, s, "[email protected]")
if gs, err := s.ListGardens(context.Background(), carol); err != nil || gs == nil || len(gs) != 0 {
t.Errorf("carol list = (%v, %v), want empty non-nil", gs, err)
}
}
func TestUpdateGardenHappyPathBumpsVersion(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Yard"})
updated, err := s.UpdateGarden(context.Background(), owner, g.ID,
GardenInput{Name: "Front Yard", WidthCM: 200, HeightCM: 400, UnitPref: domain.UnitImperial, Notes: "sunny"}, g.Version)
if err != nil {
t.Fatalf("UpdateGarden: %v", err)
}
if updated.Name != "Front Yard" || updated.WidthCM != 200 || updated.UnitPref != domain.UnitImperial {
t.Errorf("update didn't persist: %+v", updated)
}
if updated.Version != g.Version+1 {
t.Errorf("version = %d, want %d", updated.Version, g.Version+1)
}
}
func TestUpdateGardenVersionConflictReturnsCurrent(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Yard"})
// First update succeeds and bumps the version to 2.
if _, err := s.UpdateGarden(context.Background(), owner, g.ID,
GardenInput{Name: "Yard v2", WidthCM: 1000, HeightCM: 1000, UnitPref: domain.UnitMetric}, g.Version); err != nil {
t.Fatalf("first update: %v", err)
}
// A second update at the stale version 1 conflicts and returns the current row.
current, err := s.UpdateGarden(context.Background(), owner, g.ID,
GardenInput{Name: "Yard v3", WidthCM: 1000, HeightCM: 1000, UnitPref: domain.UnitMetric}, g.Version)
if !errors.Is(err, domain.ErrVersionConflict) {
t.Fatalf("stale update err = %v, want ErrVersionConflict", err)
}
if current == nil || current.Name != "Yard v2" || current.Version != 2 {
t.Errorf("conflict didn't return the current row: %+v", current)
}
// Retrying with the fresh version succeeds.
if _, err := s.UpdateGarden(context.Background(), owner, g.ID,
GardenInput{Name: "Yard v3", WidthCM: 1000, HeightCM: 1000, UnitPref: domain.UnitMetric}, current.Version); err != nil {
t.Errorf("retry with fresh version failed: %v", err)
}
}
func TestUpdateGardenRejectsMissingDimensions(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Yard"})
// Update states every field; a 0 dimension is invalid (no defaulting on update).
if _, err := s.UpdateGarden(context.Background(), owner, g.ID,
GardenInput{Name: "Yard", UnitPref: domain.UnitMetric}, g.Version); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("update with 0 dims err = %v, want ErrInvalidInput", err)
}
}
func TestCrossUserAccessIsNotFound(t *testing.T) {
s := newTestService(t, openConfig())
alice := seedUser(t, s, "[email protected]")
bob := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(context.Background(), alice, GardenInput{Name: "Alice's"})
// Bob must not learn Alice's garden exists: every access is ErrNotFound.
if _, err := s.GetGarden(context.Background(), bob, g.ID); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("bob get err = %v, want ErrNotFound", err)
}
if _, err := s.UpdateGarden(context.Background(), bob, g.ID,
GardenInput{Name: "hijack", WidthCM: 100, HeightCM: 100, UnitPref: domain.UnitMetric}, g.Version); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("bob update err = %v, want ErrNotFound", err)
}
if err := s.DeleteGarden(context.Background(), bob, g.ID); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("bob delete err = %v, want ErrNotFound", err)
}
// Alice's garden is untouched.
if still, err := s.GetGarden(context.Background(), alice, g.ID); err != nil || still.Name != "Alice's" {
t.Errorf("alice's garden was affected: %+v %v", still, err)
}
}
func TestDeleteGarden(t *testing.T) {
s := newTestService(t, openConfig())
owner := seedUser(t, s, "[email protected]")
g, _ := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Yard"})
if err := s.DeleteGarden(context.Background(), owner, g.ID); err != nil {
t.Fatalf("DeleteGarden: %v", err)
}
if _, err := s.GetGarden(context.Background(), owner, g.ID); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("garden still present after delete: %v", err)
}
// Deleting again is ErrNotFound.
if err := s.DeleteGarden(context.Background(), owner, g.ID); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("re-delete err = %v, want ErrNotFound", err)
}
}
+132
View File
@@ -0,0 +1,132 @@
package store
import (
"context"
"database/sql"
"errors"
"fmt"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// gardenColumns lists the gardens columns in the fixed order scanGarden expects.
const gardenColumns = `id, owner_id, name, width_cm, height_cm, unit_pref, notes, version, created_at, updated_at`
func scanGarden(s scanner) (*domain.Garden, error) {
var g domain.Garden
if err := s.Scan(
&g.ID, &g.OwnerID, &g.Name, &g.WidthCM, &g.HeightCM,
&g.UnitPref, &g.Notes, &g.Version, &g.CreatedAt, &g.UpdatedAt,
); err != nil {
return nil, err
}
return &g, nil
}
// CreateGarden inserts a garden (owner_id, name, dimensions, unit, notes already
// set and validated by the service) and returns the stored row.
func (d *DB) CreateGarden(ctx context.Context, g *domain.Garden) (*domain.Garden, error) {
Review

CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create)

performance · flagged by 1 model

  • internal/store/gardens.go:28CreateGarden does an INSERT then a second GetGarden round-trip to return the stored row (insert id → SELECT by id). SQLite supports INSERT … RETURNING, so this could be one statement. The same pattern is already used in users.go:CreateUser, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost.

🪰 Gadfly · advisory

⚪ **CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create)** _performance · flagged by 1 model_ - `internal/store/gardens.go:28` — `CreateGarden` does an `INSERT` then a second `GetGarden` round-trip to return the stored row (insert id → SELECT by id). SQLite supports `INSERT … RETURNING`, so this could be one statement. The same pattern is already used in `users.go:CreateUser`, so it's consistent with house style and not on a hot path; mentioning only because it doubles the round-trips per create. Low impact, unverified whether any caller cares about the cost. <sub>🪰 Gadfly · advisory</sub>
res, err := d.sql.ExecContext(ctx,
`INSERT INTO gardens (owner_id, name, width_cm, height_cm, unit_pref, notes)
VALUES (?, ?, ?, ?, ?, ?)`,
g.OwnerID, g.Name, g.WidthCM, g.HeightCM, g.UnitPref, g.Notes,
)
if err != nil {
return nil, fmt.Errorf("store: insert garden: %w", err)
}
id, err := res.LastInsertId()
if err != nil {
return nil, fmt.Errorf("store: garden insert id: %w", err)
}
return d.GetGarden(ctx, id)
}
// GetGarden returns the garden with the given id, or domain.ErrNotFound.
func (d *DB) GetGarden(ctx context.Context, id int64) (*domain.Garden, error) {
g, err := scanGarden(d.sql.QueryRowContext(ctx,
`SELECT `+gardenColumns+` FROM gardens WHERE id = ?`, id))
if errors.Is(err, sql.ErrNoRows) {
return nil, domain.ErrNotFound
}
if err != nil {
return nil, fmt.Errorf("store: get garden: %w", err)
}
return g, nil
}
// ListGardensForOwner returns the gardens owned by ownerID, newest first. The
// slice is always non-nil (an empty list, not null). Shared-with-me gardens are
// added in #16.
func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.Garden, error) {
Review

🟠 ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy

performance · flagged by 2 models

  • internal/store/gardens.go:60-82ListGardensForOwner issues an unbounded SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC with no LIMIT. The GET /api/v1/gardens list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and ListGardensForOwner materializes the full []domain.Garden in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request…

🪰 Gadfly · advisory

🟠 **ListGardensForOwner has no LIMIT/pagination — unbounded result set and allocation on a hot list endpoint, and this is the convention later list resources copy** _performance · flagged by 2 models_ - `internal/store/gardens.go:60-82` — `ListGardensForOwner` issues an unbounded `SELECT … WHERE owner_id = ? ORDER BY created_at DESC, id DESC` with no `LIMIT`. The `GET /api/v1/gardens` list endpoint (hot, cookie-authed, one row per garden) returns the owner's entire garden set every call, and `ListGardensForOwner` materializes the full `[]domain.Garden` in memory before returning. As an owner accumulates gardens (and later, in #16, shared gardens are unioned in), the result set and per-request… <sub>🪰 Gadfly · advisory</sub>
rows, err := d.sql.QueryContext(ctx,
`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC`,
ownerID,
)
if err != nil {
return nil, fmt.Errorf("store: list gardens: %w", err)
}
defer rows.Close()
gardens := []domain.Garden{}
for rows.Next() {
g, err := scanGarden(rows)
if err != nil {
return nil, fmt.Errorf("store: scan garden: %w", err)
}
gardens = append(gardens, *g)
}
if err := rows.Err(); err != nil {
return nil, fmt.Errorf("store: iterate gardens: %w", err)
}
return gardens, nil
}
// UpdateGarden applies a version-guarded update (optimistic concurrency): the row
// is written only if its stored version matches g.Version, and version is
// incremented on success. It returns:
// - the updated row, on success;
// - (current row, domain.ErrVersionConflict) when the version didn't match, so
// the caller can hand the fresh row back to the client to rebase;
// - (nil, domain.ErrNotFound) if the row no longer exists.
//
// This is the sync contract every mutable resource in pansy follows.
func (d *DB) UpdateGarden(ctx context.Context, g *domain.Garden) (*domain.Garden, error) {
updated, err := scanGarden(d.sql.QueryRowContext(ctx,
`UPDATE gardens
SET name = ?, width_cm = ?, height_cm = ?, unit_pref = ?, notes = ?,
version = version + 1,
updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
WHERE id = ? AND version = ?
RETURNING `+gardenColumns,
g.Name, g.WidthCM, g.HeightCM, g.UnitPref, g.Notes, g.ID, g.Version,
))
if errors.Is(err, sql.ErrNoRows) {
// No row matched: distinguish "gone" from "stale version".
current, gerr := d.GetGarden(ctx, g.ID)
if gerr != nil {
return nil, gerr // ErrNotFound (or a real error)
}
return current, domain.ErrVersionConflict
}
if err != nil {
return nil, fmt.Errorf("store: update garden: %w", err)
}
return updated, nil
}
// DeleteGarden removes a garden (and, via ON DELETE CASCADE, its objects/shares).
// Returns domain.ErrNotFound if no row was deleted.
func (d *DB) DeleteGarden(ctx context.Context, id int64) error {
res, err := d.sql.ExecContext(ctx, `DELETE FROM gardens WHERE id = ?`, id)
if err != nil {
return fmt.Errorf("store: delete garden: %w", err)
}
n, err := res.RowsAffected()
if err != nil {
return fmt.Errorf("store: garden delete rows: %w", err)
}
if n == 0 {
return domain.ErrNotFound
}
return nil
}