diff --git a/docs/plan.md b/docs/plan.md index 25ed321..33a3bef 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -173,6 +173,7 @@ The v1.2 design in §4 to §6 stays as reference. A piece of it is built only wh | `time-attention` | The time service answers the attention contract, so the Home's "Needs you" lists the hours to log on Jira: a row for each weekday of the last 7 days with nothing logged, today's included until anything is logged, each due at 17:00 on its day, so the days before today show as late. The twelfth of `time-tracking`'s fourteen steps (decision 188) | Done (#157) | | `jira-add-worklog` | The shared Jira client adds a worklog to one of my tickets, for the `time` service to take one: the work's start, with its own offset, and the time spent in whole seconds, keeping Jira's defaults; a key that isn't a Jira key fails before anything is sent, and decision 125's key rule moves from todos to the client. The thirteenth of `time-tracking`'s sixteen steps (decision 189) | Done (#158) | | `poll-now` | The shared poller polls on request, between its timed polls: `PollNow` polls at once, or once the poll that runs has ended, and returns when its own poll is over, so `time` can read a new worklog before it answers. The fourteenth of `time-tracking`'s sixteen steps (decision 189) | Done (#159) | +| `shared-json-body` | The reader of a request's JSON body, a copy each in todos and notify, moves to a package of its own, `internal/jsonbody`, for `time` to read worklogs with; nothing changes in behaviour. The fifteenth of `time-tracking`'s eighteen steps (decision 190) | Done (#160) | | `nudge-merge` | Nudge, the user's phone-questions tool for agents (`codedogapp/nudge`), moves into Bakhyard: one app, not two (decision 157). The nudge session does the work, one small PR per step, from its plan, [`bakhyard-merge.md`](https://github.com/codedogapp/nudge/blob/main/docs/bakhyard-merge.md); this session doesn't stop its own steps for it | In review | After these: the product phases (§8.2), each split into steps. @@ -1204,7 +1205,7 @@ Every item is pending unless it says done. The service an item lands in is in br - `pulls-reminders`: at 10:00 and 15:00 on weekdays, pulls pushes one reminder that lists my PRs under their required approvals (decision 177). Done (#144). - `homelab-pulls-notify`: done (codedogapp/homelab#40). In the homelab repo, pulls' `BAKHYARD_PEERS` names notify, `TZ` gives it the user's zone, and a NetworkPolicy lets pulls reach notify. - Later, with a database for pulls: snooze (2 h / tomorrow / until updated) and mark as nudged, from the PRs page. -- `time-tracking` [time]. Needs `todos-jira`. Reads and writes standard Jira worklogs (§2, decision 46), with no timers: Clockwork already has them inside Jira. Sixteen steps (decisions 180, 181, 183, 186, 187, 188 and 189): +- `time-tracking` [time]. Needs `todos-jira`. Reads and writes standard Jira worklogs (§2, decision 46), with no timers: Clockwork already has them inside Jira. Eighteen steps (decisions 180, 181, 183, 186, 187, 188, 189 and 190): - `shared-jira`: todos' Jira client moves to the root `internal/jira`, with the code that reads its settings and token, for the `time` service; todos' poller of my tickets moves to a package of its own, `tickets`. Nothing changes in behaviour. Done (#147). - `jira-worklogs`: the client reads my worklogs in a span of days: my account from `/myself`, the tickets I logged on from a `worklogAuthor` search, and each ticket's worklogs. Done (#148). - `time-report`: a package of the `time` service, `report`, works out the time report from my worklogs: the hours I logged on each of the last 7 days, with every weekday before today that has none marked as missing, plus this week's total against the target. Done (#149). @@ -1219,6 +1220,8 @@ Every item is pending unless it says done. The service an item lands in is in br - `time-attention`: `time` answers the attention contract with an item for each weekday with nothing logged, today's included until anything is logged, due at 17:00 on its day (decision 188). Done (#157). - `jira-add-worklog`: the shared Jira client adds a worklog to one of my tickets (decision 189). Done (#158). - `poll-now`: the shared poller polls on request, between its timed polls, and says when that poll is over (decision 189). Done (#159). + - `shared-json-body`: todos' and notify's reader of a request's JSON body moves to the root `internal/jsonbody`, for `time` to share; nothing changes in behaviour (decision 190). Done (#160). + - `jira-own-writes`: the shared Jira client reads its own worklogs at once, though Jira's search finds them only seconds or minutes later: it remembers the ticket of each worklog it adds and reads that ticket's worklogs until a search finds it (decision 190). - `worklog-add`: `time` takes a worklog at `POST /time/worklogs`, adds it to Jira, and polls again before it answers (decision 189). - `fill-day`: the Time page fills a missing day: pick one of my open tickets (assigned to me, not done) and the hours; Bakhyard shows the worklog it will add, starting at 09:00 local time, and writes it only after I confirm. @@ -1616,6 +1619,11 @@ Made on 2026-10-23: - **Polling on request** (`poll-now`): `poll.Poller.PollNow(ctx)` polls with the caller's context, in the caller's goroutine, and returns once that poll has ended, whether the look-up succeeded or failed; a failure goes to the snapshot and the log, as a timed poll's does. Polls take turns through a channel that holds one token, Run's polls too, so an older look-up can't replace what a newer one found: an ask that comes during a poll waits for it, then polls anew, since the running poll may have begun before the write. Each call gets a poll of its own. If the context ends first, while the call waits or polls, its poll counts for nothing, and `PollNow` returns the context's error. `PollNow` doesn't need Run, so a request that's still being served as the service stops still gets its poll, and the timed polls keep their schedule. Rejected: Run's loop taking the asks over a channel, which needs a way out for a request once Run has stopped, since the server's shutdown doesn't cancel requests; one poll shared by callers who ask at once, which nothing needs; and restarting the interval after an asked poll. - **Rejected**: `timeSpent` as text ("1h 30m"), which Jira reads by the site's time-tracking settings; reading the new worklog from the answer, which nothing needs; Jira's error messages in a refusal (a §0 idea); one step for all three packages. - **The token** may write worklogs: it has `write:jira-work` (decision 103), as the user confirmed on 2026-10-29. +190. **Two more steps before `worklog-add`** (`shared-json-body`, `jira-own-writes`): the lead's choices (2026-10-29), for the user to review. `time-tracking` has eighteen steps. + - **The shared body reader** (`shared-json-body`): todos and notify each had a copy of the same function that reads a request's JSON body, and `worklog-add` would have needed a third, so it moves to the root `internal/`, as AGENTS.md asks of code that two services need. `jsonbody.Read(w, r, v, refusal)` decodes the body, a JSON object without members that v lacks, into v through json/v2; a body over 64 KiB gets a plain 413, and anything else it can't read a plain 400 with the refusal. nudge keeps its two readers: their 400s say what went wrong (`read the question: …`, `read the answer: …`) rather than a fixed refusal, and sharing would change what nudge answers. + - **Jira's search lags behind its writes**: Atlassian's [Search After Write with Search And Reconcile](https://developer.atlassian.com/cloud/jira/platform/search-and-reconcile/) says the JQL search (`/rest/api/3/search/jql`) doesn't read its own writes by default: most changes show "within seconds", others take minutes. The worklog read of decision 180 finds my tickets with that search (`worklogAuthor`, `worklogDate`), so the poll that decision 189 asks for right after a write can miss the new worklog, and the Time page would still show its day as missing and invite the same worklog twice. Developers on Atlassian's community report duplicates from this lag ([topic 93517](https://community.developer.atlassian.com/t/93517)). + - **Reading its own writes** (`jira-own-writes`): the shared Jira client remembers the ticket of each worklog it adds, by the `issueId` in Jira's 201 answer, with the worklog's start, and each worklog read also reads the worklogs of the remembered tickets that the search didn't return. It forgets a ticket once a search returns it, or once its remembered start falls before the span read. A ticket's worklogs come from `/rest/api/3/issue/{id}/worklog`, which isn't a search: Atlassian names only the search endpoints as eventually consistent, so the plan takes that read to be current, an assumption not yet checked against a real site. An answer the client can't read doesn't fail the add, since the worklog is written and a retry would add it twice. + - **Rejected**: the search's own fix, `reconcileIssues`, which makes a search consistent for up to 50 issue IDs. Anonymous probes of a public Jira Cloud site, `ecosystem.atlassian.net`, on 2026-10-29 didn't show it working: the GET took neither comma-separated IDs (400) nor repeated keys (500), and the POST answered no issues at all once it named any, the issues' own IDs included, while the same search without them listed issues. Whether it covers the worklog clauses isn't documented, an Atlassian engineer says some clauses need related issues reconciled too ([topic 90411](https://community.developer.atlassian.com/t/90411)), and it has had bugs: a deleted issue's ID failed the whole search until March 2025 ([JRACLOUD-93328](https://jira.atlassian.com/browse/JRACLOUD-93328)). Also rejected: reconciling a made-up ID to force consistency, a community trick that Atlassian doesn't document; remembering tickets by key, since the search answers IDs; waiting a few seconds before the poll, which only narrows the window; and `/rest/api/3/worklog/updated`, which leaves out the last minute and lists every worklog on the site. ## 10. Risks diff --git a/internal/jsonbody/jsonbody.go b/internal/jsonbody/jsonbody.go new file mode 100644 index 0000000..7c4d0db --- /dev/null +++ b/internal/jsonbody/jsonbody.go @@ -0,0 +1,34 @@ +// Package jsonbody reads the JSON body of a request to a service's API, up to 64 KiB (decision +// 190). +package jsonbody + +import ( + "encoding/json/v2" + "errors" + "net/http" +) + +// maxBody is the largest body that Read reads: many times what any of the bodies needs. +const maxBody = 64 << 10 + +// Read decodes r's body, a JSON object without members that v lacks, into v. When it can't, the +// client gets a 413 for a body over 64 KiB or a 400 with refusal otherwise, and Read returns +// false. +func Read(w http.ResponseWriter, r *http.Request, v any, refusal string) bool { + err := json.UnmarshalRead(http.MaxBytesReader(w, r.Body, maxBody), v, + json.RejectUnknownMembers(true)) + if _, tooLarge := errors.AsType[*http.MaxBytesError](err); tooLarge { + http.Error(w, http.StatusText(http.StatusRequestEntityTooLarge), + http.StatusRequestEntityTooLarge) + + return false + } + + if err != nil { + http.Error(w, refusal, http.StatusBadRequest) + + return false + } + + return true +} diff --git a/internal/jsonbody/jsonbody_test.go b/internal/jsonbody/jsonbody_test.go new file mode 100644 index 0000000..135e461 --- /dev/null +++ b/internal/jsonbody/jsonbody_test.go @@ -0,0 +1,99 @@ +package jsonbody_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/codedogapp/bakhyard/internal/jsonbody" +) + +const refusal = "the body isn't a JSON object with just a title and a count" + +type payload struct { + Title string `json:"title"` + Count int `json:"count"` +} + +// read hands body to jsonbody.Read and returns the response, what Read returned and what it +// decoded. +func read(t *testing.T, body string) (*httptest.ResponseRecorder, bool, payload) { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", + strings.NewReader(body)) + rec := httptest.NewRecorder() + + var decoded payload + + ok := jsonbody.Read(rec, req, &decoded, refusal) + + return rec, ok, decoded +} + +func TestReadDecodesTheBody(t *testing.T) { + for _, test := range []struct { + name, body string + want payload + }{ + {"every member", `{"title":"Buy milk","count":2}`, payload{Title: "Buy milk", Count: 2}}, + {"a member missing", `{"count":2}`, payload{Count: 2}}, + {"no member", ` {} `, payload{}}, + } { + t.Run(test.name, func(t *testing.T) { + rec, ok, got := read(t, test.body) + if !ok || got != test.want { + t.Errorf("got %t %+v, want true and %+v", ok, got, test.want) + } + + if rec.Body.Len() != 0 || len(rec.Header()) != 0 { + t.Errorf("got %d %q %v, want the response left alone", + rec.Code, rec.Body, rec.Header()) + } + }) + } +} + +func TestReadRefusesWhatItCantRead(t *testing.T) { + for _, test := range []struct{ name, body string }{ + {"an unknown member", `{"title":"Buy milk","x":1}`}, + {"a member's name in another case", `{"Title":"Buy milk"}`}, + {"not JSON", `nope`}, + {"a wrong type", `{"title":1}`}, + {"an array", `[]`}, + {"an empty body", ``}, + {"a cut-off object", `{"title":"Buy milk"`}, + {"two objects", `{"title":"a"}{"title":"b"}`}, + {"invalid UTF-8", "{\"title\":\"\xff\"}"}, + {"a repeated member", `{"title":"a","title":"b"}`}, + } { + t.Run(test.name, func(t *testing.T) { + rec, ok, _ := read(t, test.body) + if ok || rec.Code != http.StatusBadRequest || rec.Body.String() != refusal+"\n" || + rec.Header().Get("Content-Type") != "text/plain; charset=utf-8" { + t.Errorf("got %t %d %q (%s), want false and a plain 400 with the refusal", + ok, rec.Code, rec.Body, rec.Header().Get("Content-Type")) + } + }) + } +} + +func TestReadTakesUpTo64KiB(t *testing.T) { + const body = `{"title":"Buy milk"}` + + padded := strings.Repeat(" ", 64<<10-len(body)) + body + if rec, ok, got := read(t, padded); !ok || got.Title != "Buy milk" { + t.Errorf("64 KiB: got %t %d %q, want the body read", ok, rec.Code, rec.Body) + } + + for _, large := range []string{" " + padded, `{"title":"` + strings.Repeat("a", 64<<10) + `"}`} { + rec, ok, _ := read(t, large) + if ok || rec.Code != http.StatusRequestEntityTooLarge || + rec.Body.String() != "Request Entity Too Large\n" || + rec.Header().Get("Content-Type") != "text/plain; charset=utf-8" { + t.Errorf("%d bytes: got %t %d %q (%s), want false and a plain 413", + len(large), ok, rec.Code, rec.Body, rec.Header().Get("Content-Type")) + } + } +} diff --git a/services/notify/internal/api/api.go b/services/notify/internal/api/api.go index 5c6035f..83a3b2f 100644 --- a/services/notify/internal/api/api.go +++ b/services/notify/internal/api/api.go @@ -11,13 +11,11 @@ import ( "github.com/go-chi/chi/v5" + "github.com/codedogapp/bakhyard/internal/jsonbody" "github.com/codedogapp/bakhyard/services/notify/internal/push" "github.com/codedogapp/bakhyard/services/notify/internal/subscription" ) -// maxBody is the largest body notify reads: 64 KiB, many times a subscription or a message. -const maxBody = 64 << 10 - // Adder keeps a device's subscription. type Adder interface { Add(ctx context.Context, sub subscription.Subscription) error @@ -73,7 +71,7 @@ type route[T any] struct { func (rt route[T]) handler(logger *slog.Logger) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { var body T - if !readBody(w, r, &body, rt.refusal) { + if !jsonbody.Read(w, r, &body, rt.refusal) { return } @@ -95,25 +93,3 @@ func (rt route[T]) handler(logger *slog.Logger) http.HandlerFunc { w.WriteHeader(http.StatusNoContent) } } - -// readBody decodes r's body, a JSON object without members that v lacks, into v. When it can't, -// the client gets a 413 for a body over maxBody or a 400 with refusal otherwise, and readBody -// returns false. -func readBody(w http.ResponseWriter, r *http.Request, v any, refusal string) bool { - err := json.UnmarshalRead(http.MaxBytesReader(w, r.Body, maxBody), v, - json.RejectUnknownMembers(true)) - if _, tooLarge := errors.AsType[*http.MaxBytesError](err); tooLarge { - http.Error(w, http.StatusText(http.StatusRequestEntityTooLarge), - http.StatusRequestEntityTooLarge) - - return false - } - - if err != nil { - http.Error(w, refusal, http.StatusBadRequest) - - return false - } - - return true -} diff --git a/services/todos/internal/api/add.go b/services/todos/internal/api/add.go index 0ceb948..e85b006 100644 --- a/services/todos/internal/api/add.go +++ b/services/todos/internal/api/add.go @@ -6,17 +6,18 @@ import ( "log/slog" "net/http" + "github.com/codedogapp/bakhyard/internal/jsonbody" "github.com/codedogapp/bakhyard/services/todos/internal/todo" ) // addTodo returns the handler of POST /todos, which adds a todo and answers 201 with it. A body // that isn't a JSON object with just a title and a ticket, or a title or a ticket that breaks its -// rule, gets a 400 with the reason, the title's first; a body over maxBody gets a 413, and a +// rule, gets a 400 with the reason, the title's first; a body over 64 KiB gets a 413, and a // database error a 500, whose cause logger gets. An empty, null or missing ticket names none. func addTodo(store *todo.Store, logger *slog.Logger) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { var addition todo.Addition - if !readBody(w, r, &addition, + if !jsonbody.Read(w, r, &addition, "the body isn't a JSON object with just a title and a ticket") { return } diff --git a/services/todos/internal/api/body.go b/services/todos/internal/api/body.go deleted file mode 100644 index b75fe07..0000000 --- a/services/todos/internal/api/body.go +++ /dev/null @@ -1,32 +0,0 @@ -package api - -import ( - "encoding/json/v2" - "errors" - "net/http" -) - -// maxBody is the largest body the todos' API reads: 64 KiB, many times a 500-character title's. -const maxBody = 64 << 10 - -// readBody decodes r's body, a JSON object without members that v lacks, into v. When it can't, -// the client gets a 413 for a body over maxBody or a 400 with refusal otherwise, and readBody -// returns false. -func readBody(w http.ResponseWriter, r *http.Request, v any, refusal string) bool { - err := json.UnmarshalRead(http.MaxBytesReader(w, r.Body, maxBody), v, - json.RejectUnknownMembers(true)) - if _, tooLarge := errors.AsType[*http.MaxBytesError](err); tooLarge { - http.Error(w, http.StatusText(http.StatusRequestEntityTooLarge), - http.StatusRequestEntityTooLarge) - - return false - } - - if err != nil { - http.Error(w, refusal, http.StatusBadRequest) - - return false - } - - return true -} diff --git a/services/todos/internal/api/edit.go b/services/todos/internal/api/edit.go index e96f118..62c3c04 100644 --- a/services/todos/internal/api/edit.go +++ b/services/todos/internal/api/edit.go @@ -7,6 +7,7 @@ import ( "net/http" "strconv" + "github.com/codedogapp/bakhyard/internal/jsonbody" "github.com/codedogapp/bakhyard/services/todos/internal/todo" ) @@ -16,7 +17,7 @@ import ( // (decision 125), and before or after names the todo to go right before or right after (decision // 118). A body that isn't a JSON object with just those members, a title, a priority or a ticket // that breaks its rule, both before and after, or a todo to go next to that doesn't exist gets a -// 400 with the reason, a body over maxBody a 413, a todo that doesn't exist a 404, and a database +// 400 with the reason, a body over 64 KiB a 413, a todo that doesn't exist a 404, and a database // error a 500, whose cause logger gets. func changeTodo(store *todo.Store, logger *slog.Logger) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -26,8 +27,8 @@ func changeTodo(store *todo.Store, logger *slog.Logger) http.HandlerFunc { } var change todo.Change - if !readBody(w, r, &change, "the body isn't a JSON object with just a title, a done mark, "+ - "a priority, a ticket and the ID of a todo to go before or after") { + if !jsonbody.Read(w, r, &change, "the body isn't a JSON object with just a title, a "+ + "done mark, a priority, a ticket and the ID of a todo to go before or after") { return }