Gardens CRUD + service-layer conventions (actor, version guard, 409) (#7) #26
@@ -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
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,7 +1,6 @@
|
|||||||
package api
|
package api
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"errors"
|
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
"net/url"
|
||||||
@@ -210,29 +209,3 @@ func (h *handlers) clearSessionCookie(c *gin.Context) {
|
|||||||
func mustActor(c *gin.Context) *domain.User {
|
func mustActor(c *gin.Context) *domain.User {
|
||||||
return c.MustGet(actorKey).(*domain.User)
|
return c.MustGet(actorKey).(*domain.User)
|
||||||
}
|
}
|
||||||
|
|
||||||
// writeServiceError maps a service-layer sentinel error to pansy's JSON error
|
|
||||||
// envelope. Login failures never leak which of email/password was wrong.
|
|
||||||
func writeServiceError(c *gin.Context, err error) {
|
|
||||||
switch {
|
|
||||||
case errors.Is(err, domain.ErrInvalidCredentials):
|
|
||||||
writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
|
|
||||||
case errors.Is(err, domain.ErrEmailTaken):
|
|
||||||
writeAPIError(c, http.StatusConflict, "EMAIL_TAKEN", "an account with that email already exists")
|
|
||||||
case errors.Is(err, domain.ErrRegistrationClosed):
|
|
||||||
writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed")
|
|
||||||
case errors.Is(err, domain.ErrLocalAuthDisabled):
|
|
||||||
writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled")
|
|
||||||
case errors.Is(err, domain.ErrOIDCNoEmail):
|
|
||||||
writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email")
|
|
||||||
case errors.Is(err, domain.ErrOIDCEmailUnverified):
|
|
||||||
writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified")
|
|
||||||
case errors.Is(err, domain.ErrOIDCIdentityConflict):
|
|
||||||
writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account")
|
|
||||||
case errors.Is(err, domain.ErrInvalidInput):
|
|
||||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
|
|
||||||
default:
|
|
||||||
slog.Error("api: unhandled service error", "error", err)
|
|
||||||
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -0,0 +1,73 @@
|
|||||||
|
package api
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
"log/slog"
|
||||||
|
"net/http"
|
||||||
|
"strconv"
|
||||||
|
|
||||||
|
"github.com/gin-gonic/gin"
|
||||||
|
|
||||||
|
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||||
|
)
|
||||||
|
|
||||||
|
// writeServiceError maps a service-layer sentinel error to pansy's JSON error
|
||||||
|
// envelope ({"error":{"code","message"}}). One mapper serves every handler
|
||||||
|
// (auth and resources) so the status/code for a given sentinel is defined once.
|
||||||
|
//
|
||||||
|
// ErrNotFound covers both a genuinely missing row and one the actor may not see
|
||||||
|
// (existence is masked). ErrVersionConflict here is a fallback that omits the
|
||||||
|
// current row — handlers that can produce one special-case it with
|
||||||
|
// writeVersionConflict before falling through here. Login failures never leak
|
||||||
|
// which of email/password was wrong.
|
||||||
|
func writeServiceError(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.ErrVersionConflict):
|
||||||
|
writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")
|
||||||
|
case errors.Is(err, domain.ErrInvalidCredentials):
|
||||||
|
writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
|
||||||
|
case errors.Is(err, domain.ErrEmailTaken):
|
||||||
|
writeAPIError(c, http.StatusConflict, "EMAIL_TAKEN", "an account with that email already exists")
|
||||||
|
case errors.Is(err, domain.ErrRegistrationClosed):
|
||||||
|
writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed")
|
||||||
|
case errors.Is(err, domain.ErrLocalAuthDisabled):
|
||||||
|
writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled")
|
||||||
|
case errors.Is(err, domain.ErrOIDCNoEmail):
|
||||||
|
writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email")
|
||||||
|
case errors.Is(err, domain.ErrOIDCEmailUnverified):
|
||||||
|
writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified")
|
||||||
|
case errors.Is(err, domain.ErrOIDCIdentityConflict):
|
||||||
|
writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account")
|
||||||
|
case errors.Is(err, domain.ErrInvalidInput):
|
||||||
|
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
|
||||||
|
default:
|
||||||
|
slog.Error("api: unhandled service error", "error", err)
|
||||||
|
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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) {
|
||||||
|
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
|
||||||
|
}
|
||||||
@@ -0,0 +1,111 @@
|
|||||||
|
package api
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
"net/http"
|
||||||
|
|
||||||
|
"github.com/gin-gonic/gin"
|
||||||
|
|
||||||
|
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||||
|
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
|
||||||
|
)
|
||||||
|
|
||||||
|
// gardenFields is the shared mutable field set for the gardens endpoints.
|
||||||
|
// Dimensions are centimeters (the API is metric-only; imperial is a display
|
||||||
|
// concern). On create, omitted dimensions default server-side.
|
||||||
|
type gardenFields struct {
|
||||||
|
Name string `json:"name" binding:"required"`
|
||||||
|
WidthCM float64 `json:"widthCm"`
|
||||||
|
HeightCM float64 `json:"heightCm"`
|
||||||
|
|
|||||||
|
UnitPref string `json:"unitPref"`
|
||||||
|
Notes string `json:"notes"`
|
||||||
|
}
|
||||||
|
|
||||||
|
func (f gardenFields) toInput() service.GardenInput {
|
||||||
|
return service.GardenInput{Name: f.Name, WidthCM: f.WidthCM, HeightCM: f.HeightCM, UnitPref: f.UnitPref, Notes: f.Notes}
|
||||||
|
}
|
||||||
|
|
||||||
|
// gardenCreateRequest / gardenUpdateRequest are the JSON bodies. Update adds a
|
||||||
|
// required current version (>= 1) for the optimistic-concurrency guard.
|
||||||
|
type gardenCreateRequest struct {
|
||||||
|
gardenFields
|
||||||
|
}
|
||||||
|
|
||||||
|
gitea-actions
commented
🟠 Negative version passes binding and causes misleading 409 conflict error-handling, maintainability · flagged by 4 models
🪰 Gadfly · advisory 🟠 **Negative version passes binding and causes misleading 409 conflict**
_error-handling, maintainability · flagged by 4 models_
- **`internal/api/gardens.go:33` / `internal/service/gardens.go:95` — negative version yields misleading 409** `gardenUpdateRequest.Version` only carries `binding:"required"`, which for an `int64` rejects zero but **allows negatives**. A negative version reaches the store, fails the `WHERE version = ?` guard, and triggers the re-read path, so the handler returns **409 VERSION_CONFLICT** instead of 400. The client cannot retry from a negative version. *Fix:* validate `version >= 1` in `parseIDPar…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
|
type gardenUpdateRequest struct {
|
||||||
|
gardenFields
|
||||||
|
Version int64 `json:"version" binding:"required,min=1"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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 {
|
||||||
|
writeServiceError(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 {
|
||||||
|
writeServiceError(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 {
|
||||||
|
writeServiceError(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
|
||||||
|
}
|
||||||
|
writeServiceError(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 {
|
||||||
|
writeServiceError(c, err)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
c.Status(http.StatusNoContent)
|
||||||
|
}
|
||||||
@@ -0,0 +1,182 @@
|
|||||||
|
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 TestGardenUpdateRejectsBadVersion(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))
|
||||||
|
|
||||||
|
// A negative (or zero) version is invalid input (400), not a version conflict.
|
||||||
|
for _, v := range []int{0, -1} {
|
||||||
|
body := map[string]any{"name": "x", "widthCm": 100, "heightCm": 100, "unitPref": "metric", "version": v}
|
||||||
|
if w := doJSON(t, r, http.MethodPatch, gardenPath(id), body, cookie); w.Code != http.StatusBadRequest {
|
||||||
|
t.Errorf("version %d patch = %d, want 400", v, w.Code)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func gardenPath(id int64) string {
|
||||||
|
return "/api/v1/gardens/" + strconv.FormatInt(id, 10)
|
||||||
|
}
|
||||||
@@ -0,0 +1,172 @@
|
|||||||
|
package service
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"math"
|
||||||
|
"strings"
|
||||||
|
|
||||||
|
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||||
|
)
|
||||||
|
|
||||||
|
// Garden sizing and field bounds. A new garden with no dimensions defaults to a
|
||||||
|
// 10 m square; dimensions must be finite and within [1 cm, 100 m] — a generous
|
||||||
|
// sanity range that also rejects NaN/Inf and absurd values. Name/notes are
|
||||||
|
// length-capped so untrusted input can't balloon storage.
|
||||||
|
const (
|
||||||
|
gitea-actions
commented
🔴 maxGardenCM constant is 1 km instead of documented 100 m correctness, error-handling, maintainability · flagged by 5 models
🪰 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>
|
|||||||
|
defaultGardenCM = 1000 // 10 m
|
||||||
|
minGardenCM = 1 // 1 cm
|
||||||
|
maxGardenCM = 10_000 // 100 m
|
||||||
|
maxGardenNameLen = 200
|
||||||
|
maxGardenNotesLen = 10_000
|
||||||
|
)
|
||||||
|
|
||||||
|
// 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)
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 Unbounded Name/Notes length from untrusted input (memory/storage DoS) maintainability, security · flagged by 1 model
🪰 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>
|
|||||||
|
|
||||||
|
// 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) {
|
||||||
|
name := strings.TrimSpace(in.Name)
|
||||||
|
if name == "" || len(name) > maxGardenNameLen {
|
||||||
|
return nil, domain.ErrInvalidInput
|
||||||
|
}
|
||||||
|
notes := strings.TrimSpace(in.Notes)
|
||||||
|
if len(notes) > maxGardenNotesLen {
|
||||||
|
return nil, domain.ErrInvalidInput
|
||||||
|
}
|
||||||
|
|
||||||
|
// 0 means "unset": defaulted on create, rejected on update. Anything else
|
||||||
|
// must be a finite length in range — this also rejects negatives, NaN, Inf,
|
||||||
|
// and subnormal-tiny values (which are > 0 but below 1 cm).
|
||||||
|
gitea-actions
commented
🔴 NaN dimension values bypass validation bounds error-handling, security · flagged by 2 models
🪰 Gadfly · advisory 🔴 **NaN dimension values bypass validation bounds**
_error-handling, security · flagged by 2 models_
- **`internal/service/gardens.go:136` — NaN bypasses dimension validation** The check `width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM` uses ordered comparisons that are **both false for NaN**, so a client sending `{"widthCm":0,"heightCm":NaN}` (or any NaN combination) passes validation on create (after defaulting the non-NaN field) and on update. NaN is then stored in the SQLite `REAL` column. *Fix:* add an explicit NaN guard with `math.IsNaN` before the bounds check.
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
|
width, height := in.WidthCM, in.HeightCM
|
||||||
|
if applyDefaults {
|
||||||
|
if width == 0 {
|
||||||
|
width = defaultGardenCM
|
||||||
|
}
|
||||||
|
if height == 0 {
|
||||||
|
height = defaultGardenCM
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !validDimensionCM(width) || !validDimensionCM(height) {
|
||||||
|
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: notes,
|
||||||
|
}, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// validDimensionCM reports whether v is a finite length within the allowed
|
||||||
|
// garden range. Guards against NaN/Inf (which slip past naive < / > comparisons)
|
||||||
|
// and subnormal-tiny positives.
|
||||||
|
func validDimensionCM(v float64) bool {
|
||||||
|
return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= minGardenCM && v <= maxGardenCM
|
||||||
|
}
|
||||||
@@ -0,0 +1,205 @@
|
|||||||
|
package service
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"errors"
|
||||||
|
"math"
|
||||||
|
"strings"
|
||||||
|
"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: strings.Repeat("a", maxGardenNameLen+1)}, // name too long
|
||||||
|
{Name: "X", Notes: strings.Repeat("b", maxGardenNotesLen+1)}, // notes too long
|
||||||
|
{Name: "X", UnitPref: "furlongs"}, // bad unit
|
||||||
|
{Name: "X", WidthCM: -5}, // negative (defaults only fill exact 0)
|
||||||
|
{Name: "X", WidthCM: maxGardenCM + 1}, // over the cap
|
||||||
|
{Name: "X", HeightCM: maxGardenCM * 10}, // over the cap
|
||||||
|
{Name: "X", WidthCM: math.NaN()}, // NaN slips past naive < / >
|
||||||
|
{Name: "X", HeightCM: math.Inf(1)}, // +Inf
|
||||||
|
{Name: "X", WidthCM: math.SmallestNonzeroFloat64}, // subnormal, > 0 but < 1 cm
|
||||||
|
}
|
||||||
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// A dimension exactly at the cap is valid.
|
||||||
|
if _, err := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Big", WidthCM: maxGardenCM, HeightCM: maxGardenCM}); err != nil {
|
||||||
|
t.Errorf("dimension at cap should be valid: %v", 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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,134 @@
|
|||||||
|
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
|
||||||
|
}
|
||||||
|
|
||||||
|
// maxGardensListed caps ListGardensForOwner as a defensive backstop against a
|
||||||
|
// pathologically large result. At household scale a user has a handful of
|
||||||
|
// gardens, so this is never hit; genuine pagination is post-v1 if ever needed.
|
||||||
|
gitea-actions
commented
⚪ CreateGarden does INSERT + a second SELECT instead of INSERT ... RETURNING (double round-trip per create) performance · flagged by 1 model
🪰 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>
|
|||||||
|
const maxGardensListed = 1000
|
||||||
|
|
||||||
|
// 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) {
|
||||||
|
created, err := scanGarden(d.sql.QueryRowContext(ctx,
|
||||||
|
`INSERT INTO gardens (owner_id, name, width_cm, height_cm, unit_pref, notes)
|
||||||
|
VALUES (?, ?, ?, ?, ?, ?)
|
||||||
|
RETURNING `+gardenColumns,
|
||||||
|
g.OwnerID, g.Name, g.WidthCM, g.HeightCM, g.UnitPref, g.Notes,
|
||||||
|
))
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("store: insert garden: %w", err)
|
||||||
|
}
|
||||||
|
return created, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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
|
||||||
|
gitea-actions
commented
🟠 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
🪰 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>
|
|||||||
|
// added in #16.
|
||||||
|
func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.Garden, error) {
|
||||||
|
rows, err := d.sql.QueryContext(ctx,
|
||||||
|
`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC LIMIT ?`,
|
||||||
|
ownerID, maxGardensListed,
|
||||||
|
)
|
||||||
|
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
|
||||||
|
}
|
||||||
🟠 gardenCreateRequest and gardenUpdateRequest duplicate identical fields and toInput methods
maintainability · flagged by 1 model
internal/api/gardens.go:19-41—gardenCreateRequestandgardenUpdateRequestduplicate five identical fields and theirtoInput()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 maketoInput()a method on the base so the translation lives in one place.🪰 Gadfly · advisory