diff --git a/server/cmd/multica/cmd_project.go b/server/cmd/multica/cmd_project.go index 47ac45600..5cfbc43bd 100644 --- a/server/cmd/multica/cmd_project.go +++ b/server/cmd/multica/cmd_project.go @@ -95,6 +95,18 @@ var validProjectStatuses = []string{ "planned", "in_progress", "paused", "completed", "cancelled", } +// validateProjectStatus rejects unknown statuses client-side so a typo fails +// fast with the valid list instead of a server round-trip and a 400. Shared by +// `project create`, `project update`, and `project status`. +func validateProjectStatus(status string) error { + for _, s := range validProjectStatuses { + if s == status { + return nil + } + } + return fmt.Errorf("invalid status %q; valid values: %s", status, strings.Join(validProjectStatuses, ", ")) +} + func init() { projectCmd.AddCommand(projectListCmd) projectCmd.AddCommand(projectGetCmd) @@ -303,6 +315,9 @@ func runProjectCreate(cmd *cobra.Command, _ []string) error { body["description"] = v } if v, _ := cmd.Flags().GetString("status"); v != "" { + if err := validateProjectStatus(v); err != nil { + return err + } body["status"] = v } if v, _ := cmd.Flags().GetString("icon"); v != "" { @@ -383,6 +398,9 @@ func runProjectUpdate(cmd *cobra.Command, args []string) error { } if cmd.Flags().Changed("status") { v, _ := cmd.Flags().GetString("status") + if err := validateProjectStatus(v); err != nil { + return err + } body["status"] = v } if cmd.Flags().Changed("icon") { @@ -449,15 +467,8 @@ func runProjectStatus(cmd *cobra.Command, args []string) error { id := args[0] status := args[1] - valid := false - for _, s := range validProjectStatuses { - if s == status { - valid = true - break - } - } - if !valid { - return fmt.Errorf("invalid status %q; valid values: %s", status, strings.Join(validProjectStatuses, ", ")) + if err := validateProjectStatus(status); err != nil { + return err } client, err := newAPIClient(cmd) diff --git a/server/cmd/multica/cmd_project_test.go b/server/cmd/multica/cmd_project_test.go index bd4fa4c1e..8ae73f9dc 100644 --- a/server/cmd/multica/cmd_project_test.go +++ b/server/cmd/multica/cmd_project_test.go @@ -1,11 +1,31 @@ package main import ( + "strings" "testing" "github.com/spf13/cobra" ) +// validateProjectStatus must accept the five DB-backed statuses and reject +// anything else with a message that lists the valid values. `project create`, +// `project update`, and `project status` all share it (#3925: `--status active` +// used to reach the server and 500 on the CHECK constraint). +func TestValidateProjectStatus(t *testing.T) { + for _, s := range validProjectStatuses { + if err := validateProjectStatus(s); err != nil { + t.Errorf("status %q should be valid, got: %v", s, err) + } + } + err := validateProjectStatus("active") + if err == nil { + t.Fatal("status \"active\" should be rejected") + } + if !strings.Contains(err.Error(), "planned") { + t.Errorf("error should list valid statuses, got: %v", err) + } +} + // newProjectResourceUpdateTestCmd mirrors the flag surface of // projectResourceUpdateCmd so unit tests can exercise the shortcut-flag plumbing // without spinning up a server. diff --git a/server/internal/handler/project.go b/server/internal/handler/project.go index d843de256..0e3a963e6 100644 --- a/server/internal/handler/project.go +++ b/server/internal/handler/project.go @@ -12,6 +12,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/logger" db "github.com/multica-ai/multica/server/pkg/db/generated" "github.com/multica-ai/multica/server/pkg/protocol" ) @@ -182,6 +183,41 @@ func (h *Handler) GetProject(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, resp) } +// validProjectStatuses / validProjectPriorities mirror the CHECK constraints on +// the project table (migrations 034, 035). CreateProject / UpdateProject +// pre-validate against these so an unknown enum value returns a clean 400 with +// the allowed list instead of surfacing the DB CHECK violation as a 500 — the +// exact mismatch reported in #3925 (`--status active`). +var validProjectStatuses = []string{"planned", "in_progress", "paused", "completed", "cancelled"} +var validProjectPriorities = []string{"urgent", "high", "medium", "low", "none"} + +// validateProjectEnum writes a 400 and returns false when value is not in +// allowed; the caller returns immediately on false. +func validateProjectEnum(w http.ResponseWriter, field, value string, allowed []string) bool { + for _, a := range allowed { + if value == a { + return true + } + } + writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid %s %q; valid values: %s", field, value, strings.Join(allowed, ", "))) + return false +} + +// writeProjectWriteError maps a failed project INSERT/UPDATE to an HTTP +// response. A CHECK constraint violation is a client error (400) — pre-validation +// already covers status/priority, so this backstops any other constrained column +// (e.g. lead_type). Anything else is a genuine server fault: log the underlying +// error so transient DB failures are diagnosable (#3925 had no server-side +// signal) and return 500. +func (h *Handler) writeProjectWriteError(w http.ResponseWriter, r *http.Request, err error, action string) { + if isCheckViolation(err) { + writeError(w, http.StatusBadRequest, "project "+action+" rejected: a field value failed a database constraint") + return + } + slog.Error("project "+action+" failed", append(logger.RequestAttrs(r), "error", err)...) + writeError(w, http.StatusInternalServerError, "failed to "+action+" project") +} + func (h *Handler) CreateProject(w http.ResponseWriter, r *http.Request) { var req CreateProjectRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -201,10 +237,16 @@ func (h *Handler) CreateProject(w http.ResponseWriter, r *http.Request) { if status == "" { status = "planned" } + if !validateProjectEnum(w, "status", status, validProjectStatuses) { + return + } priority := req.Priority if priority == "" { priority = "none" } + if !validateProjectEnum(w, "priority", priority, validProjectPriorities) { + return + } var leadType pgtype.Text var leadID pgtype.UUID if req.LeadType != nil { @@ -273,7 +315,7 @@ func (h *Handler) CreateProject(w http.ResponseWriter, r *http.Request) { if len(req.Resources) == 0 { project, err := h.Queries.CreateProject(r.Context(), createParams) if err != nil { - writeError(w, http.StatusInternalServerError, "failed to create project") + h.writeProjectWriteError(w, r, err, "create") return } resp := projectToResponse(project) @@ -293,7 +335,7 @@ func (h *Handler) CreateProject(w http.ResponseWriter, r *http.Request) { project, err := qtx.CreateProject(r.Context(), createParams) if err != nil { - writeError(w, http.StatusInternalServerError, "failed to create project") + h.writeProjectWriteError(w, r, err, "create") return } @@ -403,9 +445,15 @@ func (h *Handler) UpdateProject(w http.ResponseWriter, r *http.Request) { params.Title = pgtype.Text{String: *req.Title, Valid: true} } if req.Status != nil { + if !validateProjectEnum(w, "status", *req.Status, validProjectStatuses) { + return + } params.Status = pgtype.Text{String: *req.Status, Valid: true} } if req.Priority != nil { + if !validateProjectEnum(w, "priority", *req.Priority, validProjectPriorities) { + return + } params.Priority = pgtype.Text{String: *req.Priority, Valid: true} } if _, ok := rawFields["description"]; ok { @@ -442,7 +490,7 @@ func (h *Handler) UpdateProject(w http.ResponseWriter, r *http.Request) { } project, err := h.Queries.UpdateProject(r.Context(), params) if err != nil { - writeError(w, http.StatusInternalServerError, "failed to update project") + h.writeProjectWriteError(w, r, err, "update") return } resp := projectToResponse(project) diff --git a/server/internal/handler/project_validation_test.go b/server/internal/handler/project_validation_test.go new file mode 100644 index 000000000..9553c073a --- /dev/null +++ b/server/internal/handler/project_validation_test.go @@ -0,0 +1,93 @@ +package handler + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// An unknown project status must fail fast with a 400 and the valid list, not +// surface the DB CHECK violation as a 500 (#3925: `--status active`). +func TestCreateProjectInvalidStatusReturns400(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/projects?workspace_id="+testWorkspaceID, map[string]any{ + "title": "invalid status project", + "status": "active", + }) + testHandler.CreateProject(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for invalid status, got %d: %s", w.Code, w.Body.String()) + } + if body := w.Body.String(); !strings.Contains(body, "planned") { + t.Errorf("expected error to list valid statuses, got: %s", body) + } +} + +func TestCreateProjectInvalidPriorityReturns400(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/projects?workspace_id="+testWorkspaceID, map[string]any{ + "title": "invalid priority project", + "priority": "critical", + }) + testHandler.CreateProject(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for invalid priority, got %d: %s", w.Code, w.Body.String()) + } +} + +// A valid status still creates the project (the validation does not over-reject). +func TestCreateProjectValidStatusReturns201(t *testing.T) { + w := httptest.NewRecorder() + req := newRequest("POST", "/api/projects?workspace_id="+testWorkspaceID, map[string]any{ + "title": "valid status project", + "status": "in_progress", + }) + testHandler.CreateProject(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("expected 201 for valid status, got %d: %s", w.Code, w.Body.String()) + } + var project ProjectResponse + if err := json.NewDecoder(w.Body).Decode(&project); err != nil { + t.Fatalf("decode CreateProject: %v", err) + } + t.Cleanup(func() { + req := newRequest("DELETE", "/api/projects/"+project.ID, nil) + req = withURLParam(req, "id", project.ID) + testHandler.DeleteProject(httptest.NewRecorder(), req) + }) + if project.Status != "in_progress" { + t.Errorf("expected status in_progress, got %q", project.Status) + } +} + +// Updating to an unknown status is a 400, not a 500. +func TestUpdateProjectInvalidStatusReturns400(t *testing.T) { + // Seed a project to update. + w := httptest.NewRecorder() + req := newRequest("POST", "/api/projects?workspace_id="+testWorkspaceID, map[string]any{ + "title": "update validation project", + }) + testHandler.CreateProject(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("seed CreateProject: %d %s", w.Code, w.Body.String()) + } + var project ProjectResponse + if err := json.NewDecoder(w.Body).Decode(&project); err != nil { + t.Fatalf("decode CreateProject: %v", err) + } + t.Cleanup(func() { + req := newRequest("DELETE", "/api/projects/"+project.ID, nil) + req = withURLParam(req, "id", project.ID) + testHandler.DeleteProject(httptest.NewRecorder(), req) + }) + + w = httptest.NewRecorder() + req = newRequest("PUT", "/api/projects/"+project.ID, map[string]any{"status": "active"}) + req = withURLParam(req, "id", project.ID) + testHandler.UpdateProject(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for invalid update status, got %d: %s", w.Code, w.Body.String()) + } +}