diff --git a/veans/AGENTS.md b/veans/AGENTS.md index adc4d420c..1b0d6c902 100644 --- a/veans/AGENTS.md +++ b/veans/AGENTS.md @@ -46,9 +46,36 @@ this file is veans-specific. ## Vikunja wire-format gotchas -Most failures surface when crossing the JSON boundary. The list below is -what's bitten me; if a new endpoint behaves oddly, suspect one of these: +veans targets the Huma-backed **`/api/v2`** exclusively (`apiBasePath` in +`internal/client/client.go`). v1 is frozen, and the kanban-bucket CRUD veans +relies on only exists on v2. Most failures surface when crossing the JSON +boundary. The list below is what's bitten me; if a new endpoint behaves +oddly, suspect one of these: +- **Lists come wrapped in the standard envelope.** Every v2 list returns + `{"items":[...],"total":N,"page":N,"per_page":N,"total_pages":N}`, not a + bare array, and there is no `x-pagination-total-pages` header anymore. + Decode with the generic `Paginated[T]` helper. **Most lists are + server-paginated** — their model's `ReadAll` applies a 50-item page limit: + tasks, projects, labels, comments and bots. Page through those with + `doListAll` until `page >= total_pages`; returning only page 1 silently + truncates (>50 comments on a task is realistic). **Buckets and project + views are the exception**: their `ReadAll` takes `_ int, _ int` and returns + every row in one page, so fetch them with a single `doList` and unwrap + `.items` — paging those would re-fetch the full set and duplicate it. + Single-object responses (create/update/read of one entity) stay UNWRAPPED. +- **v2 flips the create/update verbs.** Creates are **POST** (v1 used PUT): + projects, labels, tokens, bot users, project shares, task create, + comments, relations, assignees, label-attach, bucket create. Task update + is **PATCH** (see below). The bucket-task move is **PUT**. +- **Task update is `PATCH /tasks/{id}` with `application/merge-patch+json`** + (`client.DoMerge` → `UpdateTask(*TaskPatch)`). Only the fields present in + the body are written; absent fields are left intact. Build the body from + `TaskPatch` (pointer fields, omitempty) — never a whole `client.Task`, + whose no-omitempty `done`/`title` would clobber those columns on every + call (this was issue #2962). +- **List search is `q`**, not v1's `s` (`ListParams.Q`). Task-list + `filter`/`expand`/`page`/`per_page` keep their names. - **`ProjectView.view_kind` and `bucket_configuration_mode` are strings**, not ints. The parent enums (`ProjectViewKind`, `BucketConfigurationModeKind`) have custom `MarshalJSON` that emits @@ -58,11 +85,12 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: `xorm:"-"` on it — the actual bucket lives in a separate `task_buckets` table. Fetch with `?expand=buckets` and use `task.CurrentBucketID(viewID)` to read it. -- **`POST /tasks/{id}` does NOT move tasks between buckets.** The - task↔bucket relation is row-shaped; use `client.MoveTaskToBucket()` - which hits `POST /projects/{p}/views/{v}/buckets/{b}/tasks`. The - Update path on the server only auto-moves on `done` flips. -- **Bot user creation is `PUT /user/bots`**, not `/bots` — the routes +- **Task updates do NOT move tasks between buckets.** The task↔bucket + relation is row-shaped; use `client.MoveTaskToBucket()` which hits + **`PUT /projects/{p}/views/{v}/buckets/{b}/tasks`** with a `{"task_id":N}` + body (project/view/bucket all come from the URL). The Update path on the + server only auto-moves on `done` flips. +- **Bot user creation is `POST /user/bots`**, not `/bots` — the routes are registered under the `/user` subgroup. Same prefix for `GET /user/bots`. - **`APIToken.expires_at` is required.** The struct field has @@ -88,6 +116,16 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: - `/projects/:project/views/:view/buckets/:bucket/tasks` → group `projects`, action `views_buckets_tasks` - `/tasks/:task/comments` → group `tasks_comments`, action `create` +- v1 and v2 deliberately share `(group, permission)` keys: + `pkg/models/api_routes.go` normalizes the inverted verbs (v2 POST-create + and v1 PUT-create both → `create`; v2 PUT/PATCH-update and v1 POST-update + both → `update`), and `CanDoAPIRoute` consults both route tables, treating + PATCH as an alias for the stored PUT. So `PermissionsForBot`'s scope map + authorizes the v2 calls unchanged, including the PATCH task update. +- The bucket-task MOVE (`PUT …/buckets/:bucket/tasks`) and the + buckets-with-tasks LIST (`GET …/buckets/tasks`) collide on subkey + `views_buckets_tasks`; which one gets the bare key vs `views_buckets_tasks_put` + depends on unspecified route-init order, so the bot requests **both**. - `client.PermissionsForBot()` calls `GET /routes` at runtime and grants only the intersection of what we want and what the server exposes. **Don't hard-code permission group names** — they drift @@ -96,9 +134,9 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: ## Bot ownership and token minting -- Creating a bot via `PUT /user/bots` automatically sets the bot's +- Creating a bot via `POST /user/bots` automatically sets the bot's `bot_owner_id` to the calling user. Only the owner can mint tokens - for the bot via `PUT /tokens` with `owner_id=`. The init + for the bot via `POST /tokens` with `owner_id=`. The init flow does these as a single human-JWT-authenticated batch. - Bots have no password and **cannot** authenticate via `POST /login`. After init, `veans login` re-authenticates as the human (not the @@ -115,9 +153,11 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: browser, and captures the callback. The `Shutdown` defer uses `context.WithoutCancel(ctx)` so cancellation at the outer scope still drains the loopback server cleanly. -- Token exchange is **JSON only**. Form-encoded POSTs to `/oauth/token` - fail; the standard `golang.org/x/oauth2` client speaks form encoding, - which is why we have a hand-rolled `client.ExchangeOAuthCode`. +- Token exchange goes out as **JSON**. v2's `/oauth/token` accepts both JSON + and form-encoded bodies (Huma picks the decoder off the `Content-Type` + header), but the standard `golang.org/x/oauth2` client hard-codes form + encoding and its own response shape, so we keep the hand-rolled + `client.ExchangeOAuthCode` that speaks JSON. ## Credential store diff --git a/veans/e2e/init_test.go b/veans/e2e/init_test.go index cc432d99a..972812169 100644 --- a/veans/e2e/init_test.go +++ b/veans/e2e/init_test.go @@ -102,11 +102,14 @@ func TestInit_HappyPath(t *testing.T) { t.Fatalf("bot %q not found on server", ws.BotUsername) } - // Project shared with the bot at write permission. - var shares []map[string]any + // Project shared with the bot at write permission. v2 lists come wrapped + // in the standard {items,...} envelope. + var shares struct { + Items []map[string]any `json:"items"` + } _ = h.AdminClient.Do(t.Context(), "GET", fmt.Sprintf("/projects/%d/users", project.ID), nil, nil, &shares) shareFound := false - for _, s := range shares { + for _, s := range shares.Items { if u, _ := s["username"].(string); u == ws.BotUsername { if p, _ := s["permission"].(float64); int(p) >= 1 { shareFound = true diff --git a/veans/internal/bootstrap/bootstrap.go b/veans/internal/bootstrap/bootstrap.go index a1251be89..5ea9a5e07 100644 --- a/veans/internal/bootstrap/bootstrap.go +++ b/veans/internal/bootstrap/bootstrap.go @@ -257,7 +257,7 @@ func Init(ctx context.Context, opts *Options) (*Result, error) { return nil, output.Wrap(output.CodeUnknown, err, "mint bot token: %v", err) } if mintedToken.Token == "" { - return nil, output.New(output.CodeUnknown, "PUT /tokens did not return a token plaintext — cannot continue") + return nil, output.New(output.CodeUnknown, "POST /tokens did not return a token plaintext — cannot continue") } // 11. Persist credentials. Discard human JWT immediately after. diff --git a/veans/internal/bootstrap/bootstrap_test.go b/veans/internal/bootstrap/bootstrap_test.go index 8c70fb333..6edf52e1b 100644 --- a/veans/internal/bootstrap/bootstrap_test.go +++ b/veans/internal/bootstrap/bootstrap_test.go @@ -211,8 +211,9 @@ func TestConfirmOverwriteExistingConfig(t *testing.T) { } // bucketServer is a minimal httptest server modelling -// GET/PUT /api/v1/projects/{p}/views/{v}/buckets. The caller pre-seeds -// existing buckets; PUT requests append to that list with a synthetic ID. +// GET/POST /api/v2/projects/{p}/views/{v}/buckets. The caller pre-seeds +// existing buckets; POST requests append to that list with a synthetic ID. +// GET returns the standard v2 list envelope; POST returns the bare bucket. type bucketServer struct { mu sync.Mutex existing []*client.Bucket @@ -232,7 +233,7 @@ func newBucketServer(seed []*client.Bucket) *bucketServer { func (s *bucketServer) handler() http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Path is /api/v1/projects/{p}/views/{v}/buckets. + // Path is /api/v2/projects/{p}/views/{v}/buckets. if !strings.HasSuffix(r.URL.Path, "/buckets") || !strings.Contains(r.URL.Path, "/views/") { http.Error(w, "unexpected path: "+r.URL.Path, http.StatusInternalServerError) return @@ -242,8 +243,15 @@ func (s *bucketServer) handler() http.Handler { switch r.Method { case http.MethodGet: w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(s.existing) - case http.MethodPut: + // v2 list envelope; the buckets list isn't server-paginated. + _ = json.NewEncoder(w).Encode(map[string]any{ + "items": s.existing, + "total": len(s.existing), + "page": 1, + "per_page": 50, + "total_pages": 1, + }) + case http.MethodPost: var b client.Bucket if err := json.NewDecoder(r.Body).Decode(&b); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) diff --git a/veans/internal/client/assignees.go b/veans/internal/client/assignees.go index 2e63b46db..6478acd01 100644 --- a/veans/internal/client/assignees.go +++ b/veans/internal/client/assignees.go @@ -23,5 +23,5 @@ import ( // AddAssignee assigns a user (typically the bot) to a task. func (c *Client) AddAssignee(ctx context.Context, taskID, userID int64) error { - return c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/assignees", taskID), nil, &TaskAssignee{UserID: userID}, nil) + return c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/assignees", taskID), nil, &TaskAssignee{UserID: userID}, nil) } diff --git a/veans/internal/client/buckets.go b/veans/internal/client/buckets.go index ba4c324e5..43aa8c398 100644 --- a/veans/internal/client/buckets.go +++ b/veans/internal/client/buckets.go @@ -21,25 +21,28 @@ import ( "fmt" ) -// ListBuckets returns the buckets configured on a Kanban view. +// ListBuckets returns the buckets configured on a Kanban view. Bucket.ReadAll +// ignores page/per_page and returns every bucket in a single page (the envelope +// total reflects the full set), so one GET gets them all — paging would +// re-fetch the same buckets and duplicate them. Unwrap .items. func (c *Client) ListBuckets(ctx context.Context, projectID, viewID int64) ([]*Bucket, error) { - var out []*Bucket path := fmt.Sprintf("/projects/%d/views/%d/buckets", projectID, viewID) - if err := c.Do(ctx, "GET", path, nil, nil, &out); err != nil { + items, _, err := doList[*Bucket](ctx, c, path, nil) + if err != nil { return nil, err } - return out, nil + return items, nil } -// CreateBucket inserts a new bucket into a Kanban view. +// CreateBucket inserts a new bucket into a Kanban view. The project and view +// come from the URL; the v2 handler ignores project_view_id in the body. func (c *Client) CreateBucket(ctx context.Context, projectID, viewID int64, b *Bucket) (*Bucket, error) { var out Bucket path := fmt.Sprintf("/projects/%d/views/%d/buckets", projectID, viewID) if b == nil { b = &Bucket{} } - b.ProjectViewID = viewID - if err := c.Do(ctx, "PUT", path, nil, b, &out); err != nil { + if err := c.Do(ctx, "POST", path, nil, b, &out); err != nil { return nil, err } return &out, nil @@ -47,17 +50,13 @@ func (c *Client) CreateBucket(ctx context.Context, projectID, viewID int64, b *B // MoveTaskToBucket positions an existing task in `bucketID` on the // project's view. Vikunja stores task↔bucket relations in a separate -// table (`task_buckets`), so POST /tasks/{id} with bucket_id does not -// reliably move tasks — this dedicated endpoint is the one the Kanban -// UI's drag-and-drop uses. +// table (`task_buckets`); a task update with bucket_id does not reliably +// move tasks — this dedicated endpoint is the one the Kanban UI's +// drag-and-drop uses. On v2 it's a PUT, and project/view/bucket all come +// from the URL, so the body only carries the task id. func (c *Client) MoveTaskToBucket(ctx context.Context, projectID, viewID, bucketID, taskID int64) error { path := fmt.Sprintf("/projects/%d/views/%d/buckets/%d/tasks", projectID, viewID, bucketID) - body := map[string]int64{ - "task_id": taskID, - "project_view_id": viewID, - "bucket_id": bucketID, - "project_id": projectID, - } - return c.Do(ctx, "POST", path, nil, body, nil) + body := map[string]int64{"task_id": taskID} + return c.Do(ctx, "PUT", path, nil, body, nil) } diff --git a/veans/internal/client/client.go b/veans/internal/client/client.go index 19ad0827a..6e45bb53d 100644 --- a/veans/internal/client/client.go +++ b/veans/internal/client/client.go @@ -33,7 +33,7 @@ import ( // Client is a thin JSON wrapper around the Vikunja REST API. It holds the // server base URL and a bearer token (either a JWT from POST /login or an -// API token minted via PUT /tokens). Every method in this package is a thin +// API token minted via POST /tokens). Every method in this package is a thin // shim over Do. type Client struct { BaseURL string @@ -41,6 +41,19 @@ type Client struct { HTTPClient *http.Client } +// apiBasePath is the version prefix every request is mounted under. veans +// targets the Huma-backed /api/v2 exclusively — v1 is frozen and the bucket +// CRUD endpoints veans needs only exist on v2. +const apiBasePath = "/api/v2" + +// contentTypeJSON / contentTypeMergePatch are the request body content types +// Do and DoMerge send. Merge-patch (RFC 7396) is how v2 does partial updates: +// only the fields present in the body are written, the rest are left intact. +const ( + contentTypeJSON = "application/json" + contentTypeMergePatch = "application/merge-patch+json" +) + // UserAgent is the value sent in the User-Agent header on every request. // main sets this at startup with the linker-injected version + the // runtime os/arch (e.g. "veans/0.3.1 (linux/amd64)"). Tests get the @@ -61,17 +74,36 @@ func New(baseURL, token string) *Client { } } -// vikunjaError matches `web.HTTPError` on the wire. +// vikunjaError matches the RFC 9457 problem+json body /api/v2 returns +// (huma.ErrorModel augmented with Vikunja's numeric domain `code`). The +// human-readable message lives in `detail`; `title` is the status text +// fallback. `message` is v1's legacy field, kept only as a fallback so a +// stray legacy/proxy error body still yields a readable message instead of +// raw JSON. The HTTP status used for output.Code mapping comes from the +// response status line, not this body. type vikunjaError struct { - Code int `json:"code"` + Title string `json:"title"` + Detail string `json:"detail"` Message string `json:"message"` + Code int `json:"code"` } -// Do performs a single JSON request against /api/v1. body, if non-nil, +// Do performs a single JSON request against /api/v2. body, if non-nil, // is JSON-marshalled. out, if non-nil, is JSON-unmarshalled. query is appended // as URL-encoded params. func (c *Client) Do(ctx context.Context, method, path string, query url.Values, body, out any) error { - full := c.BaseURL + "/api/v1" + path + return c.do(ctx, method, path, query, body, out, contentTypeJSON) +} + +// DoMerge is like Do but sends the body as a JSON Merge Patch +// (application/merge-patch+json). Used for PATCH updates so only the fields +// present in `body` are written server-side — see UpdateTask. +func (c *Client) DoMerge(ctx context.Context, method, path string, query url.Values, body, out any) error { + return c.do(ctx, method, path, query, body, out, contentTypeMergePatch) +} + +func (c *Client) do(ctx context.Context, method, path string, query url.Values, body, out any, contentType string) error { + full := c.BaseURL + apiBasePath + path if len(query) > 0 { full += "?" + query.Encode() } @@ -91,7 +123,7 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, } req.Header.Set("Accept", "application/json") if body != nil { - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Content-Type", contentType) } if c.Token != "" { req.Header.Set("Authorization", "Bearer "+c.Token) @@ -122,51 +154,55 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, return nil } -// DoPaginated is like Do but also returns the total page count parsed from -// the `x-pagination-total-pages` response header (0 if the header is -// missing or unparseable). Used by the list endpoints so paging terminates -// against the authoritative server count, not a `len(batch) < per_page` -// heuristic that loops one extra time on exact-multiple totals. -func (c *Client) DoPaginated(ctx context.Context, method, path string, query url.Values, out any) (totalPages int, err error) { - full := c.BaseURL + "/api/v1" + path - if len(query) > 0 { - full += "?" + query.Encode() - } - req, err := http.NewRequestWithContext(ctx, method, full, nil) - if err != nil { - return 0, fmt.Errorf("build request: %w", err) - } - req.Header.Set("Accept", "application/json") - if c.Token != "" { - req.Header.Set("Authorization", "Bearer "+c.Token) - } - req.Header.Set("User-Agent", UserAgent) +// Paginated mirrors the standard /api/v2 list envelope. Every v2 list +// operation returns this shape (v1 returned a bare array plus an +// x-pagination-total-pages header, which is gone). Single-object responses +// stay unwrapped. +type Paginated[T any] struct { + Items []T `json:"items"` + Total int64 `json:"total"` + Page int `json:"page"` + PerPage int `json:"per_page"` + TotalPages int `json:"total_pages"` +} - resp, err := c.HTTPClient.Do(req) - if err != nil { - return 0, output.Wrap(output.CodeUnknown, err, "%s %s: %v", method, path, err) +// doList GETs `path` and decodes the standard v2 list envelope, returning the +// items plus the server's total page count so a caller can page until +// page >= totalPages. Generic so each list endpoint reuses it without a +// per-type wrapper struct. +func doList[T any](ctx context.Context, c *Client, path string, query url.Values) (items []T, totalPages int, err error) { + var env Paginated[T] + if err := c.Do(ctx, "GET", path, query, nil, &env); err != nil { + return nil, 0, err } - defer resp.Body.Close() + return env.Items, env.TotalPages, nil +} - respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) - if err != nil { - return 0, fmt.Errorf("read response: %w", err) - } - if resp.StatusCode >= 400 { - return 0, mapHTTPError(method, path, resp.StatusCode, respBody, - parseRetryAfter(resp.Header.Get("Retry-After"))) - } - if out != nil && len(respBody) > 0 { - if err := json.Unmarshal(respBody, out); err != nil { - return 0, fmt.Errorf("decode %s %s: %w", method, path, err) +// doListAll pages through a v2 list endpoint, accumulating every item until +// page >= total_pages. +// +// Use it ONLY for endpoints whose model honours page/per_page — the +// server-paginated lists (tasks, projects, labels, comments, bots). For the +// endpoints whose ReadAll ignores pagination and returns every row in a single +// page (buckets, views), call doList instead: looping those re-fetches the full +// set on every page and duplicates it. +func doListAll[T any](ctx context.Context, c *Client, path string) ([]T, error) { + var all []T + page := 1 + for { + q := url.Values{} + q.Set("page", strconv.Itoa(page)) + q.Set("per_page", "50") + batch, totalPages, err := doList[T](ctx, c, path, q) + if err != nil { + return nil, err } - } - if v := resp.Header.Get("x-pagination-total-pages"); v != "" { - if n, perr := strconv.Atoi(v); perr == nil { - totalPages = n + all = append(all, batch...) + if page >= totalPages { + return all, nil } + page++ } - return totalPages, nil } // DoRaw is the escape hatch used by `veans api`. It returns the raw response @@ -176,7 +212,7 @@ func (c *Client) DoPaginated(ctx context.Context, method, path string, query url // "stdout is for the success payload; errors go through the envelope on // stderr"); see commands/api.go for the canonical handling. func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Values, body []byte) (status int, respBody []byte, retryAfter time.Duration, err error) { - full := c.BaseURL + "/api/v1" + path + full := c.BaseURL + apiBasePath + path if len(query) > 0 { full += "?" + query.Encode() } @@ -205,18 +241,6 @@ func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Value return resp.StatusCode, respBody, parseRetryAfter(resp.Header.Get("Retry-After")), err } -// paginationDone reports whether a paged GET has consumed every page, -// preferring the server's x-pagination-total-pages count when present and -// falling back to the len(batch) < per_page heuristic when the header is -// missing (older server / proxy stripped). Centralized so all list -// endpoints terminate identically. -func paginationDone(page, batchLen, perPage, totalPages int) bool { - if totalPages > 0 { - return page >= totalPages - } - return batchLen < perPage -} - // maxBodyBytes caps the size of any response body we'll read into memory. // Vikunja JSON payloads are far smaller; the cap exists so a misbehaving // proxy can't OOM the CLI by streaming an unbounded body. @@ -244,7 +268,16 @@ func parseRetryAfter(v string) time.Duration { func mapHTTPError(method, path string, status int, body []byte, retryAfter time.Duration) error { var ve vikunjaError _ = json.Unmarshal(body, &ve) - msg := strings.TrimSpace(ve.Message) + // v2's problem+json carries the human-readable text in `detail`; fall back + // to `title`, then v1's legacy `message`, then the raw body, then the + // status text. + msg := strings.TrimSpace(ve.Detail) + if msg == "" { + msg = strings.TrimSpace(ve.Title) + } + if msg == "" { + msg = strings.TrimSpace(ve.Message) + } if msg == "" { msg = strings.TrimSpace(string(body)) if msg == "" { diff --git a/veans/internal/client/client_test.go b/veans/internal/client/client_test.go index 5f7144fa2..19bb8ddd7 100644 --- a/veans/internal/client/client_test.go +++ b/veans/internal/client/client_test.go @@ -45,7 +45,7 @@ func TestMapHTTPError_StatusCodeMapping(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := mapHTTPError("GET", "/foo", tc.status, []byte(`{"message":"boom"}`), 0) + err := mapHTTPError("GET", "/foo", tc.status, []byte(`{"detail":"boom"}`), 0) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) @@ -59,7 +59,7 @@ func TestMapHTTPError_StatusCodeMapping(t *testing.T) { func TestMapHTTPError_RetryAfterAppendedToMessage(t *testing.T) { retry := 7 * time.Second - err := mapHTTPError("GET", "/foo", http.StatusTooManyRequests, []byte(`{"message":"slow down"}`), retry) + err := mapHTTPError("GET", "/foo", http.StatusTooManyRequests, []byte(`{"detail":"slow down"}`), retry) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) @@ -89,20 +89,53 @@ func TestMapHTTPError_BodyTruncation(t *testing.T) { } } -func TestMapHTTPError_VikunjaJSONTakesPrecedenceOverRawBody(t *testing.T) { - body := []byte(`{"code":404,"message":"x"}`) +func TestMapHTTPError_VikunjaProblemJSONTakesPrecedenceOverRawBody(t *testing.T) { + // v2 returns RFC 9457 problem+json: the message is in `detail`, and `code` + // carries Vikunja's numeric domain error code (not the HTTP status). + body := []byte(`{"status":404,"title":"Not Found","detail":"x","code":3001}`) err := mapHTTPError("GET", "/foo", http.StatusNotFound, body, 0) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) } // The formatted message is "METHOD PATH: STATUS MSG"; assert it carries - // the decoded message and not the raw JSON envelope. + // the decoded `detail` and not the raw JSON envelope. if !strings.HasSuffix(oe.Message, ": 404 x") { t.Errorf("expected formatted message to end with %q, got %q", ": 404 x", oe.Message) } - if strings.Contains(oe.Message, `"code":404`) { - t.Errorf("expected raw JSON body to be replaced by decoded message, got %q", oe.Message) + if strings.Contains(oe.Message, `"code"`) { + t.Errorf("expected raw JSON body to be replaced by decoded detail, got %q", oe.Message) + } +} + +func TestMapHTTPError_FallsBackToTitleWhenNoDetail(t *testing.T) { + // A problem+json body with no `detail` (e.g. Huma's own schema-validation + // 422 sometimes only sets title) falls back to `title`. + body := []byte(`{"status":422,"title":"Unprocessable Entity"}`) + err := mapHTTPError("PATCH", "/tasks/1", http.StatusUnprocessableEntity, body, 0) + var oe *output.Error + if !errors.As(err, &oe) { + t.Fatalf("expected *output.Error, got %T", err) + } + if !strings.HasSuffix(oe.Message, ": 422 Unprocessable Entity") { + t.Errorf("expected title fallback, got %q", oe.Message) + } +} + +func TestMapHTTPError_FallsBackToLegacyMessage(t *testing.T) { + // Defensive: a stray legacy/proxy body with only v1's `message` field + // still yields the message text rather than the raw JSON. + body := []byte(`{"code":403,"message":"forbidden"}`) + err := mapHTTPError("GET", "/foo", http.StatusForbidden, body, 0) + var oe *output.Error + if !errors.As(err, &oe) { + t.Fatalf("expected *output.Error, got %T", err) + } + if !strings.HasSuffix(oe.Message, ": 403 forbidden") { + t.Errorf("expected legacy message fallback, got %q", oe.Message) + } + if strings.Contains(oe.Message, `"message"`) { + t.Errorf("expected raw JSON to be replaced by the message text, got %q", oe.Message) } } @@ -146,36 +179,9 @@ func TestParseRetryAfter(t *testing.T) { } } -func TestPaginationDone(t *testing.T) { - cases := []struct { - name string - page int - batchLen int - perPage int - totalPages int - want bool - }{ - {"server says single page complete", 1, 50, 50, 1, true}, - {"server says more pages remain", 1, 50, 50, 2, false}, - {"server says we're on the last page", 2, 10, 50, 2, true}, - {"no header, full page -> not done", 1, 50, 50, 0, false}, - {"no header, short page -> done", 1, 10, 50, 0, true}, - {"no header, empty page -> done", 1, 0, 50, 0, true}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := paginationDone(tc.page, tc.batchLen, tc.perPage, tc.totalPages) - if got != tc.want { - t.Errorf("paginationDone(page=%d, batch=%d, per=%d, total=%d) = %v, want %v", - tc.page, tc.batchLen, tc.perPage, tc.totalPages, got, tc.want) - } - }) - } -} - func TestCreateBotUser_404TranslatesToBotUsersUnavailable(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPut || r.URL.Path != "/api/v1/user/bots" { + if r.Method != http.MethodPost || r.URL.Path != "/api/v2/user/bots" { http.Error(w, "unexpected route", http.StatusInternalServerError) return } @@ -196,3 +202,129 @@ func TestCreateBotUser_404TranslatesToBotUsersUnavailable(t *testing.T) { t.Errorf("got code %q, want %q", oe.Code, output.CodeBotUsersUnavailable) } } + +// TestListProjects_PaginatesEnvelope verifies the v2 list shape: each page is +// the {items,total,page,per_page,total_pages} envelope, and ListProjects keeps +// requesting until page >= total_pages, accumulating every item. +func TestListProjects_PaginatesEnvelope(t *testing.T) { + var gotPages []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/projects" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + page := r.URL.Query().Get("page") + gotPages = append(gotPages, page) + w.Header().Set("Content-Type", "application/json") + switch page { + case "1": + _, _ = w.Write([]byte(`{"items":[{"id":1,"title":"a"},{"id":2,"title":"b"}],"total":3,"page":1,"per_page":2,"total_pages":2}`)) + case "2": + _, _ = w.Write([]byte(`{"items":[{"id":3,"title":"c"}],"total":3,"page":2,"per_page":2,"total_pages":2}`)) + default: + t.Errorf("unexpected page %q (would loop past the end)", page) + http.Error(w, "no such page", http.StatusBadRequest) + } + })) + defer srv.Close() + + projects, err := New(srv.URL, "tk").ListProjects(context.Background()) + if err != nil { + t.Fatalf("ListProjects: %v", err) + } + if len(projects) != 3 { + t.Fatalf("expected 3 projects accumulated across 2 pages, got %d", len(projects)) + } + if len(gotPages) != 2 || gotPages[0] != "1" || gotPages[1] != "2" { + t.Fatalf("expected exactly pages [1 2], got %v", gotPages) + } +} + +// TestListTaskComments_PaginatesEnvelope guards the truncation bug: the v2 +// comments endpoint is server-paginated, so a task with >50 comments spans +// multiple pages and ListTaskComments must accumulate them all, not stop at +// page 1. +func TestListTaskComments_PaginatesEnvelope(t *testing.T) { + var pages []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/tasks/9/comments" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + page := r.URL.Query().Get("page") + pages = append(pages, page) + w.Header().Set("Content-Type", "application/json") + switch page { + case "1": + _, _ = w.Write([]byte(`{"items":[{"id":1,"comment":"a"},{"id":2,"comment":"b"}],"total":3,"page":1,"per_page":2,"total_pages":2}`)) + case "2": + _, _ = w.Write([]byte(`{"items":[{"id":3,"comment":"c"}],"total":3,"page":2,"per_page":2,"total_pages":2}`)) + default: + t.Errorf("unexpected page %q", page) + http.Error(w, "no such page", http.StatusBadRequest) + } + })) + defer srv.Close() + + comments, err := New(srv.URL, "tk").ListTaskComments(context.Background(), 9) + if err != nil { + t.Fatalf("ListTaskComments: %v", err) + } + if len(comments) != 3 { + t.Fatalf("expected 3 comments across 2 pages, got %d (truncation regression?)", len(comments)) + } + if len(pages) != 2 { + t.Fatalf("expected to fetch 2 pages, got %v", pages) + } +} + +// TestListBuckets_SingleFetchDoesNotPage pins the opposite invariant: the +// buckets model returns every row in one page, so ListBuckets must issue a +// single request even when the envelope's total_pages is >1 — paging would +// re-fetch and duplicate the buckets. +func TestListBuckets_SingleFetchDoesNotPage(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 1 { + t.Errorf("ListBuckets paged a single-page endpoint (request %d) — would duplicate", requests) + } + w.Header().Set("Content-Type", "application/json") + // total_pages deliberately > 1 to prove ListBuckets ignores it. + _, _ = w.Write([]byte(`{"items":[{"id":1,"title":"Todo"},{"id":2,"title":"Doing"}],"total":2,"page":1,"per_page":1,"total_pages":2}`)) + })) + defer srv.Close() + + buckets, err := New(srv.URL, "tk").ListBuckets(context.Background(), 7, 3) + if err != nil { + t.Fatalf("ListBuckets: %v", err) + } + if requests != 1 { + t.Fatalf("expected exactly 1 request, got %d", requests) + } + if len(buckets) != 2 { + t.Fatalf("expected the 2 buckets from the single page, got %d", len(buckets)) + } +} + +// TestListProjectViews_UnwrapsEnvelope pins that a previously-single-GET list +// (project views) now unwraps .items from the standard list envelope. +func TestListProjectViews_UnwrapsEnvelope(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/projects/7/views" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"items":[{"id":10,"title":"Kanban","view_kind":"kanban"}],"total":1,"page":1,"per_page":50,"total_pages":1}`)) + })) + defer srv.Close() + + views, err := New(srv.URL, "tk").ListProjectViews(context.Background(), 7) + if err != nil { + t.Fatalf("ListProjectViews: %v", err) + } + if len(views) != 1 || views[0].ViewKind != ViewKindKanban { + t.Fatalf("expected one kanban view unwrapped from .items, got %+v", views) + } +} diff --git a/veans/internal/client/comments.go b/veans/internal/client/comments.go index 8f4cc493b..331b24591 100644 --- a/veans/internal/client/comments.go +++ b/veans/internal/client/comments.go @@ -24,17 +24,15 @@ import ( // AddTaskComment posts a new comment on a task. func (c *Client) AddTaskComment(ctx context.Context, taskID int64, body string) (*TaskComment, error) { var out TaskComment - if err := c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/comments", taskID), nil, &TaskComment{Comment: body}, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/comments", taskID), nil, &TaskComment{Comment: body}, &out); err != nil { return nil, err } return &out, nil } -// ListTaskComments returns all comments on a task. +// ListTaskComments returns all comments on a task. The v2 comments endpoint is +// server-paginated (TaskComment.ReadAll applies a 50-item page limit), so page +// through to the end instead of returning only the first page. func (c *Client) ListTaskComments(ctx context.Context, taskID int64) ([]*TaskComment, error) { - var out []*TaskComment - if err := c.Do(ctx, "GET", fmt.Sprintf("/tasks/%d/comments", taskID), nil, nil, &out); err != nil { - return nil, err - } - return out, nil + return doListAll[*TaskComment](ctx, c, fmt.Sprintf("/tasks/%d/comments", taskID)) } diff --git a/veans/internal/client/discover.go b/veans/internal/client/discover.go index 8cad249e7..1e71e404c 100644 --- a/veans/internal/client/discover.go +++ b/veans/internal/client/discover.go @@ -31,11 +31,15 @@ import ( const defaultAPIPort = "3456" // DiscoverServer normalizes `input` and probes a small set of plausible -// URLs for /api/v1/info, returning the canonical base URL (without the -// /api/v1 suffix — that's what client.New expects) and the parsed Info. +// URLs for /api/v2/info, returning the canonical base URL (without the +// /api/v2 suffix — that's what client.New expects) and the parsed Info. +// +// Probing /api/v2/info doubles as the "is this server new enough" check: a +// Vikunja without /api/v2 fails discovery cleanly rather than limping along +// against endpoints veans needs. // // Mirrors the discovery the Vikunja web frontend does in -// helpers/checkAndSetApiUrl.ts: try the URL as-given, with /api/v1 +// helpers/checkAndSetApiUrl.ts: try the URL as-given, with the API path // appended, and with the default :3456 port — across http / https. The // first response that parses as Info wins. func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { @@ -53,7 +57,7 @@ func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { var attempts []string var lastErr error for _, base := range candidates { - attempts = append(attempts, base+"/api/v1/info") + attempts = append(attempts, base+"/api/v2/info") info, err := New(base, "").Info(ctx) if err == nil && info != nil { return base, info, nil @@ -67,15 +71,16 @@ func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { } // serverCandidates expands `input` into the ordered list of base URLs -// to probe for /api/v1/info. A "base URL" here is what client.New wants: -// the origin + the path that should sit BEFORE /api/v1 (typically empty -// or a reverse-proxy prefix). The probe itself adds /api/v1/info. +// to probe for /api/v2/info. A "base URL" here is what client.New wants: +// the origin + the path that should sit BEFORE /api/v2 (typically empty +// or a reverse-proxy prefix). The probe itself adds /api/v2/info. func serverCandidates(input string) ([]string, error) { - // Strip a trailing /api/v1[/] the user might have copied from a - // curl example. We add it back in the probe, and otherwise we'd - // end up calling /api/v1/api/v1/info. + // Strip a trailing /api/v1 or /api/v2[/] the user might have copied + // from a curl example. We add the API path back in the probe, and + // otherwise we'd end up calling /api/v2/api/v2/info. trimmed := strings.TrimRight(input, "/") trimmed = strings.TrimSuffix(trimmed, "/api/v1") + trimmed = strings.TrimSuffix(trimmed, "/api/v2") trimmed = strings.TrimRight(trimmed, "/") withScheme := trimmed diff --git a/veans/internal/client/labels.go b/veans/internal/client/labels.go index 1681b49a1..24ce13c28 100644 --- a/veans/internal/client/labels.go +++ b/veans/internal/client/labels.go @@ -33,15 +33,15 @@ func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") if search != "" { - q.Set("s", search) + // v2's list search param is `q` (v1 used `s`). + q.Set("q", search) } - var batch []*Label - total, err := c.DoPaginated(ctx, "GET", "/labels", q, &batch) + batch, totalPages, err := doList[*Label](ctx, c, "/labels", q) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), 50, total) { + if page >= totalPages { return all, nil } page++ @@ -51,7 +51,7 @@ func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error // CreateLabel creates a new label owned by the authenticated user. func (c *Client) CreateLabel(ctx context.Context, l *Label) (*Label, error) { var out Label - if err := c.Do(ctx, "PUT", "/labels", nil, l, &out); err != nil { + if err := c.Do(ctx, "POST", "/labels", nil, l, &out); err != nil { return nil, err } return &out, nil @@ -59,7 +59,7 @@ func (c *Client) CreateLabel(ctx context.Context, l *Label) (*Label, error) { // AddLabelToTask attaches an existing label to a task. func (c *Client) AddLabelToTask(ctx context.Context, taskID, labelID int64) error { - return c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/labels", taskID), nil, &LabelTask{LabelID: labelID}, nil) + return c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/labels", taskID), nil, &LabelTask{LabelID: labelID}, nil) } // RemoveLabelFromTask detaches a label. diff --git a/veans/internal/client/projects.go b/veans/internal/client/projects.go index e0f23eccb..eb2357430 100644 --- a/veans/internal/client/projects.go +++ b/veans/internal/client/projects.go @@ -23,8 +23,8 @@ import ( "strconv" ) -// ListProjects pages through GET /projects, accumulating until the server's -// x-pagination-total-pages header says we're done. +// ListProjects pages through GET /projects, accumulating until the list +// envelope's total_pages says we're done. func (c *Client) ListProjects(ctx context.Context) ([]*Project, error) { var all []*Project page := 1 @@ -32,13 +32,12 @@ func (c *Client) ListProjects(ctx context.Context) ([]*Project, error) { q := url.Values{} q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") - var batch []*Project - total, err := c.DoPaginated(ctx, "GET", "/projects", q, &batch) + batch, totalPages, err := doList[*Project](ctx, c, "/projects", q) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), 50, total) { + if page >= totalPages { return all, nil } page++ @@ -58,7 +57,7 @@ func (c *Client) GetProject(ctx context.Context, id int64) (*Project, error) { // auto-creates the default views (List, Gantt, Table, Kanban) on insert. func (c *Client) CreateProject(ctx context.Context, p *Project) (*Project, error) { var out Project - if err := c.Do(ctx, "PUT", "/projects", nil, p, &out); err != nil { + if err := c.Do(ctx, "POST", "/projects", nil, p, &out); err != nil { return nil, err } return &out, nil @@ -67,17 +66,20 @@ func (c *Client) CreateProject(ctx context.Context, p *Project) (*Project, error // ShareProjectWithUser grants `username` `permission` on project `id`. func (c *Client) ShareProjectWithUser(ctx context.Context, projectID int64, share *ProjectUser) (*ProjectUser, error) { var out ProjectUser - if err := c.Do(ctx, "PUT", fmt.Sprintf("/projects/%d/users", projectID), nil, share, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/projects/%d/users", projectID), nil, share, &out); err != nil { return nil, err } return &out, nil } // ListProjectViews returns saved views (Kanban, List, …) on a project. +// ProjectView.ReadAll ignores page/per_page and returns every view in a single +// page, so one GET gets them all — paging would re-fetch the same views and +// duplicate them. Unwrap .items. func (c *Client) ListProjectViews(ctx context.Context, projectID int64) ([]*ProjectView, error) { - var out []*ProjectView - if err := c.Do(ctx, "GET", fmt.Sprintf("/projects/%d/views", projectID), nil, nil, &out); err != nil { + items, _, err := doList[*ProjectView](ctx, c, fmt.Sprintf("/projects/%d/views", projectID), nil) + if err != nil { return nil, err } - return out, nil + return items, nil } diff --git a/veans/internal/client/relations.go b/veans/internal/client/relations.go index 136e57c58..8de7e3920 100644 --- a/veans/internal/client/relations.go +++ b/veans/internal/client/relations.go @@ -26,7 +26,7 @@ import ( func (c *Client) CreateRelation(ctx context.Context, taskID int64, otherTaskID int64, relationKind string) (*TaskRelation, error) { var out TaskRelation body := &TaskRelation{OtherTaskID: otherTaskID, RelationKind: relationKind} - if err := c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/relations", taskID), nil, body, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/relations", taskID), nil, body, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/routes.go b/veans/internal/client/routes.go index 179773f20..84cab0b41 100644 --- a/veans/internal/client/routes.go +++ b/veans/internal/client/routes.go @@ -40,7 +40,7 @@ func (c *Client) Routes(ctx context.Context) (map[string]RouteGroup, error) { // PermissionsForBot picks a curated subset of route groups the veans bot // needs and projects the available actions of each. Groups not present on // the server are silently dropped, so the resulting permission map is -// always valid for PUT /tokens regardless of Vikunja version. +// always valid for POST /tokens regardless of Vikunja version. // // The action names reflect Vikunja's actual route map (see GET /routes): // bucket CRUD and the bucket-task move endpoint live under the `projects` @@ -56,10 +56,16 @@ func PermissionsForBot(routes map[string]RouteGroup) map[string][]string { }, // Project access: read project metadata, manage buckets & move // tasks between them. tasks_by-index resolves #NN / PROJ-NN. + // The bucket-task MOVE (PUT .../buckets/:bucket/tasks) and the + // buckets-with-tasks LIST (GET .../buckets/tasks) collide on subkey + // `views_buckets_tasks`; which one gets the bare key vs the + // `_put`-suffixed key depends on unspecified route-init order, so we + // request BOTH and let the runtime intersection drop whichever the + // server didn't register. "projects": { "read_one", "read_all", "tasks_by-index", "views_buckets", "views_buckets_put", "views_buckets_post", - "views_buckets_delete", "views_buckets_tasks", + "views_buckets_delete", "views_buckets_tasks", "views_buckets_tasks_put", }, "projects_views": {"read_one", "read_all"}, "labels": {"read_one", "read_all", "create", "update", "delete"}, diff --git a/veans/internal/client/routes_test.go b/veans/internal/client/routes_test.go index 8fc2ab369..fc9687518 100644 --- a/veans/internal/client/routes_test.go +++ b/veans/internal/client/routes_test.go @@ -16,7 +16,10 @@ package client -import "testing" +import ( + "slices" + "testing" +) func TestPermissionsForBot_DropsUnknownGroups(t *testing.T) { // Server only exposes a subset of what we ask for. @@ -61,3 +64,42 @@ func TestPermissionsForBot_EmptyWhenServerIsEmpty(t *testing.T) { t.Fatalf("expected empty map, got %v", got) } } + +// TestPermissionsForBot_ProjectsBucketScopes pins the project-group scopes the +// bot needs for the v2 kanban-bucket calls: list/create/update/delete buckets +// plus the bucket-task MOVE. The MOVE and the buckets-with-tasks LIST collide +// on the `views_buckets_tasks` subkey and the bare-vs-_put assignment depends +// on unspecified route-init order, so the bot must request BOTH keys; the +// runtime intersection keeps whichever the server actually exposes. +func TestPermissionsForBot_ProjectsBucketScopes(t *testing.T) { + // A server that registered the move under the bare key and the list under + // the _put key (one of the two possible orderings). + server := map[string]RouteGroup{ + "projects": { + "read_one": {}, + "read_all": {}, + "tasks_by-index": {}, + "views_buckets": {}, // list buckets + "views_buckets_post": {}, // create bucket + "views_buckets_put": {}, // update bucket + "views_buckets_delete": {}, // delete bucket + "views_buckets_tasks": {}, // bucket-task move OR buckets-with-tasks list + "views_buckets_tasks_put": {}, // the other of the colliding pair + }, + } + got := PermissionsForBot(server) + projects, ok := got["projects"] + if !ok { + t.Fatalf("expected projects group in result") + } + want := []string{ + "read_one", "read_all", "tasks_by-index", + "views_buckets", "views_buckets_post", "views_buckets_put", + "views_buckets_delete", "views_buckets_tasks", "views_buckets_tasks_put", + } + for _, w := range want { + if !slices.Contains(projects, w) { + t.Errorf("projects scope %q missing from bot grant; got %v", w, projects) + } + } +} diff --git a/veans/internal/client/tasks.go b/veans/internal/client/tasks.go index c53c219c5..dbbdbbc2b 100644 --- a/veans/internal/client/tasks.go +++ b/veans/internal/client/tasks.go @@ -52,7 +52,7 @@ func (o *TaskListOptions) values() url.Values { } // ListProjectTasks paginates `GET /projects/{id}/tasks` exhaustively, -// terminating against the server's x-pagination-total-pages header. +// terminating against the list envelope's total_pages. func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *TaskListOptions) ([]*Task, error) { if opts == nil { opts = &TaskListOptions{} @@ -61,19 +61,19 @@ func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *Ta if per <= 0 { per = 50 } + path := fmt.Sprintf("/projects/%d/tasks", projectID) var all []*Task page := 1 for { o := *opts o.Page = page o.PerPage = per - var batch []*Task - total, err := c.DoPaginated(ctx, "GET", fmt.Sprintf("/projects/%d/tasks", projectID), o.values(), &batch) + batch, totalPages, err := doList[*Task](ctx, c, path, o.values()) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), per, total) { + if page >= totalPages { return all, nil } page++ @@ -114,24 +114,26 @@ func (t *Task) CurrentBucketID(viewID int64) int64 { return 0 } -// CreateTask inserts a task into a project (PUT /projects/{id}/tasks). +// CreateTask inserts a task into a project (POST /projects/{id}/tasks). func (c *Client) CreateTask(ctx context.Context, projectID int64, t *Task) (*Task, error) { var out Task - if err := c.Do(ctx, "PUT", fmt.Sprintf("/projects/%d/tasks", projectID), nil, t, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/projects/%d/tasks", projectID), nil, t, &out); err != nil { return nil, err } return &out, nil } -// UpdateTask updates a task (POST /tasks/{id}). This endpoint does NOT -// move tasks between buckets — the task↔bucket relation is row-shaped in -// task_buckets, and bucket_id on the request body is ignored. Use -// MoveTaskToBucket() for that. The server does auto-flip the bucket -// when `done` toggles, but only between the canonical "todo" and "done" -// buckets the project view is configured with. -func (c *Client) UpdateTask(ctx context.Context, id int64, t *Task) (*Task, error) { +// UpdateTask partially updates a task via PATCH /tasks/{id} with a JSON Merge +// Patch body: only the fields set on `patch` are written, the rest are left +// intact (the fix for issue #2962, where a status-only update used to zero +// description and priority). This endpoint does NOT move tasks between +// buckets — the task↔bucket relation is row-shaped in task_buckets, and +// bucket_id on the request body is ignored. Use MoveTaskToBucket() for that. +// The server still auto-flips the bucket when `done` toggles, between the +// canonical "todo" and "done" buckets the project view is configured with. +func (c *Client) UpdateTask(ctx context.Context, id int64, patch *TaskPatch) (*Task, error) { var out Task - if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d", id), nil, t, &out); err != nil { + if err := c.DoMerge(ctx, "PATCH", fmt.Sprintf("/tasks/%d", id), nil, patch, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/tokens.go b/veans/internal/client/tokens.go index 58a99b682..a8c0bc054 100644 --- a/veans/internal/client/tokens.go +++ b/veans/internal/client/tokens.go @@ -23,7 +23,7 @@ import "context" // the bot in step 8 of init). func (c *Client) CreateToken(ctx context.Context, t *APIToken) (*APIToken, error) { var out APIToken - if err := c.Do(ctx, "PUT", "/tokens", nil, t, &out); err != nil { + if err := c.Do(ctx, "POST", "/tokens", nil, t, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/types.go b/veans/internal/client/types.go index edcabc766..930004c6c 100644 --- a/veans/internal/client/types.go +++ b/veans/internal/client/types.go @@ -29,7 +29,7 @@ type User struct { Email string `json:"email,omitempty"` } -// BotUser is what `PUT /bots` returns. +// BotUser is what `POST /user/bots` returns. type BotUser struct { ID int64 `json:"id"` Username string `json:"username"` @@ -38,7 +38,7 @@ type BotUser struct { Created time.Time `json:"created,omitempty"` } -// BotUserCreate is the request body for PUT /bots. +// BotUserCreate is the request body for POST /user/bots. type BotUserCreate struct { Username string `json:"username"` Name string `json:"name,omitempty"` @@ -119,6 +119,20 @@ type Task struct { PercentDone float64 `json:"percent_done,omitempty"` } +// TaskPatch is the JSON Merge Patch body for UpdateTask (PATCH /tasks/{id}). +// Every field is a pointer with omitempty so only the fields the caller sets +// are serialized; absent fields are left untouched server-side. This is the +// fix for issue #2962 — a status-only update no longer zeroes description or +// priority the way the old whole-object write did. A non-nil pointer to a zero +// value (e.g. *Priority = 0, *Done = false) still serializes, which is how an +// explicit "clear priority" or "reopen" reaches the server. +type TaskPatch struct { + Title *string `json:"title,omitempty"` + Description *string `json:"description,omitempty"` + Done *bool `json:"done,omitempty"` + Priority *int64 `json:"priority,omitempty"` +} + // TaskComment matches pkg/models/task_comments.TaskComment. type TaskComment struct { ID int64 `json:"id"` @@ -138,12 +152,12 @@ type Label struct { Updated time.Time `json:"updated,omitempty"` } -// LabelTask is the body for `PUT /tasks/{id}/labels`. +// LabelTask is the body for `POST /tasks/{id}/labels`. type LabelTask struct { LabelID int64 `json:"label_id"` } -// TaskRelation is the body for `PUT /tasks/{id}/relations` and the row +// TaskRelation is the body for `POST /tasks/{id}/relations` and the row // returned. RelationKind is one of: subtask, parenttask, related, duplicates, // duplicateof, blocking, blocked, precedes, follows, copiedfrom, copiedto. type TaskRelation struct { @@ -152,12 +166,12 @@ type TaskRelation struct { RelationKind string `json:"relation_kind"` } -// TaskAssignee is the body for `PUT /tasks/{id}/assignees`. +// TaskAssignee is the body for `POST /tasks/{id}/assignees`. type TaskAssignee struct { UserID int64 `json:"user_id"` } -// ProjectUser is the body and response for `PUT /projects/{id}/users`. +// ProjectUser is the body and response for `POST /projects/{id}/users`. type ProjectUser struct { ID int64 `json:"id,omitempty"` Username string `json:"username"` @@ -171,7 +185,7 @@ const ( PermissionAdmin = 2 ) -// APIToken is the request and response shape for `PUT /tokens`. The plaintext +// APIToken is the request and response shape for `POST /tokens`. The plaintext // `Token` field is only populated on creation. Vikunja requires ExpiresAt; // callers that want a long-lived token use FarFuture (year 9999). type APIToken struct { @@ -223,8 +237,9 @@ type LoginResponse struct { Token string `json:"token"` } -// OAuthTokenRequest is the JSON body for POST /api/v1/oauth/token. Vikunja's -// OAuth server explicitly rejects form-encoded requests; everything is JSON. +// OAuthTokenRequest is the JSON body for POST /api/v2/oauth/token. The v2 +// endpoint accepts both JSON and form-encoded bodies; veans sends JSON, which +// Huma decodes off the Content-Type header regardless of the declared form. type OAuthTokenRequest struct { GrantType string `json:"grant_type"` Code string `json:"code,omitempty"` diff --git a/veans/internal/client/users.go b/veans/internal/client/users.go index eb2335610..9308fd9ae 100644 --- a/veans/internal/client/users.go +++ b/veans/internal/client/users.go @@ -23,17 +23,17 @@ import ( "code.vikunja.io/veans/internal/output" ) -// CreateBotUser provisions a bot user via PUT /user/bots. The username must +// CreateBotUser provisions a bot user via POST /user/bots. The username must // be prefixed `bot-` (Vikunja enforces this). The caller becomes the bot's // owner, which is what allows them to mint API tokens for the bot via -// PUT /tokens with owner_id. +// POST /tokens with owner_id. // // On Vikunja versions that predate the /user/bots endpoint, the server // returns 404, which we surface as BOT_USERS_UNAVAILABLE so init can fail // fast with a clear message. func (c *Client) CreateBotUser(ctx context.Context, username, name string) (*BotUser, error) { var out BotUser - err := c.Do(ctx, "PUT", "/user/bots", nil, &BotUserCreate{Username: username, Name: name}, &out) + err := c.Do(ctx, "POST", "/user/bots", nil, &BotUserCreate{Username: username, Name: name}, &out) if err != nil { var oe *output.Error if errors.As(err, &oe) && oe.Code == output.CodeNotFound { @@ -45,13 +45,11 @@ func (c *Client) CreateBotUser(ctx context.Context, username, name string) (*Bot return &out, nil } -// ListBotUsers returns all bot users owned by the authenticated user. +// ListBotUsers returns all bot users owned by the authenticated user. The v2 +// endpoint is server-paginated (BotUser.ReadAll applies a 50-item page limit), +// so page through to the end instead of returning only the first page. func (c *Client) ListBotUsers(ctx context.Context) ([]*BotUser, error) { - var out []*BotUser - if err := c.Do(ctx, "GET", "/user/bots", nil, nil, &out); err != nil { - return nil, err - } - return out, nil + return doListAll[*BotUser](ctx, c, "/user/bots") } // FindMyBotByUsername scans the caller's owned bots for one with the given diff --git a/veans/internal/commands/api.go b/veans/internal/commands/api.go index d1e9669c9..b36964ba2 100644 --- a/veans/internal/commands/api.go +++ b/veans/internal/commands/api.go @@ -37,14 +37,14 @@ func newAPICmd() *cobra.Command { cmd := &cobra.Command{ Use: "api ", Short: "Raw REST passthrough — escape hatch for endpoints veans doesn't wrap", - Long: `Sends a request to /api/v1 as the bot. Use this when curated + Long: `Sends a request to /api/v2 as the bot. Use this when curated commands don't shape the data the way you need. The response body is written to stdout verbatim. Examples: veans api GET /projects veans api GET /tasks/123 - veans api POST /tasks/123 --data '{"description":"updated"}' + veans api POST /tasks/123/comments --data '{"comment":"

note

"}' veans api GET /tasks --query expand=reactions --query per_page=100`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/veans/internal/commands/login.go b/veans/internal/commands/login.go index dbc2c9cf4..6d5b9411e 100644 --- a/veans/internal/commands/login.go +++ b/veans/internal/commands/login.go @@ -99,7 +99,7 @@ you want to rotate.`, return err } if minted.Token == "" { - return output.New(output.CodeUnknown, "PUT /tokens did not return token plaintext") + return output.New(output.CodeUnknown, "POST /tokens did not return token plaintext") } if err := credentials.Default().Set(cfg.Server, cfg.Bot.Username, minted.Token); err != nil { diff --git a/veans/internal/commands/update.go b/veans/internal/commands/update.go index a0d272864..63540ee99 100644 --- a/veans/internal/commands/update.go +++ b/veans/internal/commands/update.go @@ -88,7 +88,7 @@ func newUpdateCmd() *cobra.Command { } // runUpdate is intentionally a single linear flow — the steps it performs -// (concurrency check → status → field changes → comments → field POST → +// (concurrency check → status → field changes → comments → field PATCH → // bucket move → label add/remove → refetch) all share the same task, // flag set, and error-handling shape. Splitting them produces five tiny // functions that each take the same five arguments. @@ -126,17 +126,18 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli } } - // Build the update payload incrementally so we don't clobber unmentioned - // fields. The base must include the ID; bucket/done are conditional. - body := &client.Task{ID: id} + // Build the merge-patch payload from only the changed fields. PATCH leaves + // absent fields untouched, so omitting a field preserves it — the id rides + // in the URL, not the body. + body := &client.TaskPatch{} dirty := false if f.title != "" { - body.Title = f.title + body.Title = &f.title dirty = true } if f.priorityIsSet { - body.Priority = f.priority + body.Priority = &f.priority dirty = true } @@ -147,7 +148,7 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli return nil, err } if descChanged { - body.Description = newDesc + body.Description = &newDesc dirty = true } @@ -162,7 +163,8 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli return nil, err } bucketTransitionTarget = bid - body.Done = newStatus.Done() + done := newStatus.Done() + body.Done = &done dirty = true } diff --git a/veans/internal/commands/update_test.go b/veans/internal/commands/update_test.go index 716ab9e8c..6a66fb6b8 100644 --- a/veans/internal/commands/update_test.go +++ b/veans/internal/commands/update_test.go @@ -155,28 +155,29 @@ func startRecordingServer(t *testing.T) (*httptest.Server, *[]recordedCall) { w.Header().Set("Content-Type", "application/json") switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/tasks/42": + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/tasks/42": // Initial fetch + the final refetch both land here. Return a // fixed task with an empty label set — labels.go's // findLabelOnTask only iterates t.Labels. _ = json.NewEncoder(w).Encode(map[string]any{ "id": 42, "title": "t", "updated": "2026-01-01T00:00:00Z", }) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/tasks/42/comments": + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/tasks/42/comments": _ = json.NewEncoder(w).Encode(map[string]any{"id": 1, "comment": ""}) - case r.Method == http.MethodPost && r.URL.Path == "/api/v1/tasks/42": - // UpdateTask. Echo back the id so the encoder downstream is - // happy with a non-nil Task. + case r.Method == http.MethodPatch && r.URL.Path == "/api/v2/tasks/42": + // UpdateTask (merge-patch). Echo back the id so the encoder + // downstream is happy with a non-nil Task. _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) - case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/projects/") && strings.HasSuffix(r.URL.Path, "/tasks"): + case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, "/api/v2/projects/") && strings.HasSuffix(r.URL.Path, "/tasks"): + // Bucket-task move (PUT .../buckets/{b}/tasks). _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/labels": - // getOrCreateLabelByTitle's lookup. Empty array → falls through + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/labels": + // getOrCreateLabelByTitle's lookup. Empty envelope → falls through // to label creation. - _ = json.NewEncoder(w).Encode([]any{}) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/labels": + _ = json.NewEncoder(w).Encode(map[string]any{"items": []any{}, "total_pages": 1}) + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/labels": _ = json.NewEncoder(w).Encode(map[string]any{"id": 99, "title": "veans:bug"}) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/tasks/42/labels": + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/tasks/42/labels": _ = json.NewEncoder(w).Encode(map[string]any{"id": 99}) default: t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) @@ -222,11 +223,11 @@ func TestRunUpdate_ScrappedOrdersCommentUpdateMove(t *testing.T) { } want := []recordedCall{ - {http.MethodGet, "/api/v1/tasks/42"}, // current task fetch - {http.MethodPut, "/api/v1/tasks/42/comments"}, // "Scrapped: obsolete" - {http.MethodPost, "/api/v1/tasks/42"}, // field update (done=true) - {http.MethodPost, "/api/v1/projects/7/views/1/buckets/14/tasks"}, // bucket move to Scrapped - {http.MethodGet, "/api/v1/tasks/42"}, // refetch with new bucket + {http.MethodGet, "/api/v2/tasks/42"}, // current task fetch + {http.MethodPost, "/api/v2/tasks/42/comments"}, // "Scrapped: obsolete" + {http.MethodPatch, "/api/v2/tasks/42"}, // field update (done=true) + {http.MethodPut, "/api/v2/projects/7/views/1/buckets/14/tasks"}, // bucket move to Scrapped + {http.MethodGet, "/api/v2/tasks/42"}, // refetch with new bucket } if !reflect.DeepEqual(*calls, want) { t.Fatalf("call order mismatch:\nwant: %#v\ngot: %#v", want, *calls) @@ -253,13 +254,13 @@ func TestRunUpdate_BucketMoveBeforeLabelAdd(t *testing.T) { } want := []recordedCall{ - {http.MethodGet, "/api/v1/tasks/42"}, // current task fetch - {http.MethodPost, "/api/v1/tasks/42"}, // field update (done=false) - {http.MethodPost, "/api/v1/projects/7/views/1/buckets/11/tasks"}, // bucket move to In Progress - {http.MethodGet, "/api/v1/labels"}, // getOrCreateLabelByTitle lookup - {http.MethodPut, "/api/v1/labels"}, // create veans:bug - {http.MethodPut, "/api/v1/tasks/42/labels"}, // attach label - {http.MethodGet, "/api/v1/tasks/42"}, // refetch + {http.MethodGet, "/api/v2/tasks/42"}, // current task fetch + {http.MethodPatch, "/api/v2/tasks/42"}, // field update (done=false) + {http.MethodPut, "/api/v2/projects/7/views/1/buckets/11/tasks"}, // bucket move to In Progress + {http.MethodGet, "/api/v2/labels"}, // getOrCreateLabelByTitle lookup + {http.MethodPost, "/api/v2/labels"}, // create veans:bug + {http.MethodPost, "/api/v2/tasks/42/labels"}, // attach label + {http.MethodGet, "/api/v2/tasks/42"}, // refetch } if !reflect.DeepEqual(*calls, want) { t.Fatalf("call order mismatch:\nwant: %#v\ngot: %#v", want, *calls)