store, ipc: type the routine status, defend the framing with tests (V-410)
Two review threads from PR 4, and the answer to the third. The routine status was a bare string with its legal set in a comment. Nothing caught a typo at compile time, nothing enumerated the set for a test, and a bad value surfaced as a /routines row that neither accepts nor dismisses. It is a RoutineStatus now, with the three constants, a RoutineStatuses slice as the single source of truth, and Valid(). Listing by an unknown status is refused with ErrRoutineStatus instead of answering "no rows", which is what a correct query says about an empty table. A round-trip test moves a routine into each state and reads it back, so a constant that drifts from the inline SQL fails loudly. The hand-rolled framing stays, and frame.go now says why: ninety lines, readable with socat, and every standard replacement brings schema machinery this boundary does not want. What was wrong was inheriting it untested. frame_test.go covers the paths a real socket produces and the round-trip test never does — truncated header, truncated body, one byte per Read, two frames back to back, and a non-JSON body. Empty input is the only EOF. The unanswered question in the same file is answered in place: a routine object stays a local string, not a Nexus ref, because nothing acts on it. It is the word he used, replayed back to him, compared only against itself for the UNIQUE key. Canonical refs arrive if a routine ever drives a Hexis call, which is V-272. The mood enum has the same shape and is not done here: it is spelled in the GBNF grammar, three prompts and the parse, so it is its own change.
This commit is contained in:
@@ -36,6 +36,13 @@ const maxFrame = 4 << 20
|
||||
// (we read the length but not the body), so the caller must close it.
|
||||
var ErrFrameTooLarge = errors.New("ipc: frame too large")
|
||||
|
||||
// The framing is hand-rolled and it stays that way (Vikunja #410): ninety
|
||||
// lines, readable on the wire with socat, and every standard replacement
|
||||
// brings schema machinery this boundary does not want. The condition is that
|
||||
// it is defended by tests rather than inherited untested — truncated header,
|
||||
// truncated body, partial reads, frame boundaries and a non-JSON body all
|
||||
// live in frame_test.go.
|
||||
//
|
||||
// writeFrame encodes v as JSON and frames it as a 4-byte big-endian length
|
||||
// prefix + body. length-prefixed JSON (not a tighter binary schema) is the
|
||||
// deferred-but-picked wire format: debuggable with `socat`/`nc`, trivial to
|
||||
|
||||
@@ -0,0 +1,142 @@
|
||||
package ipc
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/binary"
|
||||
"errors"
|
||||
"io"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// The framing is hand-rolled, and it is the protocol all nine daemons depend
|
||||
// on, so a bug in it is a bug everywhere (Vikunja #410). The review asked
|
||||
// whether to replace it with something standard. It stays: length-prefixed
|
||||
// JSON over a unix socket is ninety lines, it is readable with socat, and the
|
||||
// alternatives (net/rpc, gRPC, a codec library) all buy schema machinery this
|
||||
// boundary does not want. What was wrong was inheriting it untested. These
|
||||
// are the paths a real socket produces that the round-trip test never does.
|
||||
|
||||
// A short header is not EOF. EOF means the peer closed cleanly between
|
||||
// frames, which the server treats as a normal disconnect; a header that stops
|
||||
// halfway is a truncated frame and must be reported as an error, or a peer
|
||||
// that dies mid-write looks like one that hung up politely.
|
||||
func TestReadFrameTruncatedHeader(t *testing.T) {
|
||||
var v any
|
||||
err := readFrame(bytes.NewReader([]byte{0, 0, 4}), &v)
|
||||
if err == nil {
|
||||
t.Fatal("a three-byte header must fail")
|
||||
}
|
||||
if errors.Is(err, io.EOF) {
|
||||
t.Fatalf("err = %v, want a truncation error, not EOF", err)
|
||||
}
|
||||
}
|
||||
|
||||
// A header promising more body than follows. Same reasoning: the frame never
|
||||
// arrived, so it must not decode into a zero value the caller then trusts.
|
||||
func TestReadFrameTruncatedBody(t *testing.T) {
|
||||
var buf bytes.Buffer
|
||||
var hdr [4]byte
|
||||
binary.BigEndian.PutUint32(hdr[:], 32)
|
||||
buf.Write(hdr[:])
|
||||
buf.WriteString(`{"m":"pi`)
|
||||
|
||||
var got map[string]string
|
||||
if err := readFrame(&buf, &got); err == nil {
|
||||
t.Fatal("a body shorter than its prefix must fail")
|
||||
}
|
||||
if len(got) != 0 {
|
||||
t.Errorf("decoded %v from a truncated frame", got)
|
||||
}
|
||||
}
|
||||
|
||||
// Nothing at all is EOF, and only this is.
|
||||
func TestReadFrameEmptyIsEOF(t *testing.T) {
|
||||
var v any
|
||||
if err := readFrame(bytes.NewReader(nil), &v); !errors.Is(err, io.EOF) {
|
||||
t.Fatalf("err = %v, want io.EOF", err)
|
||||
}
|
||||
}
|
||||
|
||||
// byteAtATime returns one byte per Read, which is what a socket is allowed to
|
||||
// do and what a bytes.Reader never does. readFrame uses io.ReadFull for both
|
||||
// the header and the body; this is the test that would fail if either turned
|
||||
// into a bare Read.
|
||||
type byteAtATime struct {
|
||||
b []byte
|
||||
i int
|
||||
}
|
||||
|
||||
func (r *byteAtATime) Read(p []byte) (int, error) {
|
||||
if r.i >= len(r.b) {
|
||||
return 0, io.EOF
|
||||
}
|
||||
if len(p) == 0 {
|
||||
return 0, nil
|
||||
}
|
||||
p[0] = r.b[r.i]
|
||||
r.i++
|
||||
return 1, nil
|
||||
}
|
||||
|
||||
func TestReadFrameReassemblesPartialReads(t *testing.T) {
|
||||
type payload struct {
|
||||
Msg string `json:"m"`
|
||||
N int `json:"n"`
|
||||
}
|
||||
want := payload{Msg: "привет", N: 7}
|
||||
var buf bytes.Buffer
|
||||
if err := writeFrame(&buf, want); err != nil {
|
||||
t.Fatalf("writeFrame: %v", err)
|
||||
}
|
||||
|
||||
var got payload
|
||||
if err := readFrame(&byteAtATime{b: buf.Bytes()}, &got); err != nil {
|
||||
t.Fatalf("readFrame: %v", err)
|
||||
}
|
||||
if got != want {
|
||||
t.Fatalf("got %+v, want %+v", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// Two frames written back to back must come back as two frames. A reader that
|
||||
// consumed more than one frame's body would desynchronize the connection, and
|
||||
// the symptom would be a reply attributed to the wrong request.
|
||||
func TestReadFrameStopsAtTheFrameBoundary(t *testing.T) {
|
||||
var buf bytes.Buffer
|
||||
for _, m := range []string{"first", "second"} {
|
||||
if err := writeFrame(&buf, map[string]string{"m": m}); err != nil {
|
||||
t.Fatalf("writeFrame: %v", err)
|
||||
}
|
||||
}
|
||||
r := &byteAtATime{b: buf.Bytes()}
|
||||
for _, want := range []string{"first", "second"} {
|
||||
var got map[string]string
|
||||
if err := readFrame(r, &got); err != nil {
|
||||
t.Fatalf("readFrame(%s): %v", want, err)
|
||||
}
|
||||
if got["m"] != want {
|
||||
t.Fatalf("got %q, want %q", got["m"], want)
|
||||
}
|
||||
}
|
||||
var extra map[string]string
|
||||
if err := readFrame(r, &extra); !errors.Is(err, io.EOF) {
|
||||
t.Fatalf("after two frames: err = %v, want io.EOF", err)
|
||||
}
|
||||
}
|
||||
|
||||
// A body that is not JSON is an error, not a zero value. The peer is either
|
||||
// broken or not speaking this protocol; either way the caller must not read
|
||||
// on as though it decoded.
|
||||
func TestReadFrameRejectsNonJSONBody(t *testing.T) {
|
||||
var buf bytes.Buffer
|
||||
var hdr [4]byte
|
||||
body := []byte("not json at all")
|
||||
binary.BigEndian.PutUint32(hdr[:], uint32(len(body)))
|
||||
buf.Write(hdr[:])
|
||||
buf.Write(body)
|
||||
|
||||
var got map[string]string
|
||||
if err := readFrame(&buf, &got); err == nil {
|
||||
t.Fatal("a non-JSON body must fail")
|
||||
}
|
||||
}
|
||||
@@ -356,7 +356,7 @@ func (a *storeAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine,
|
||||
Action: r.Action,
|
||||
Object: r.Object,
|
||||
IntervalDays: r.IntervalDays,
|
||||
Status: r.Status,
|
||||
Status: string(r.Status),
|
||||
CreatedTs: r.CreatedTs.UnixMilli(),
|
||||
}
|
||||
if r.ReminderID != nil {
|
||||
|
||||
@@ -8,14 +8,37 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// The three states a proposal can be in. A proposal starts 'proposed' and
|
||||
// moves once, either way, and never moves again.
|
||||
// RoutineStatus — the state a proposal is in. A defined type, not a bare
|
||||
// string, because the legal set used to live in a comment: nothing caught a
|
||||
// typo at compile time, nothing enumerated the set for a test, and a bad value
|
||||
// surfaced as a /routines row that neither accepts nor dismisses (Vikunja #46,
|
||||
// #410).
|
||||
type RoutineStatus string
|
||||
|
||||
// The three states a proposal can be in. A proposal starts proposed and moves
|
||||
// once, either way, and never moves again.
|
||||
const (
|
||||
RoutineProposed = "proposed"
|
||||
RoutineAccepted = "accepted"
|
||||
RoutineDismissed = "dismissed"
|
||||
RoutineProposed RoutineStatus = "proposed"
|
||||
RoutineAccepted RoutineStatus = "accepted"
|
||||
RoutineDismissed RoutineStatus = "dismissed"
|
||||
)
|
||||
|
||||
// RoutineStatuses — the legal set, and the single source of truth a test can
|
||||
// range over. Adding a state means adding it here.
|
||||
var RoutineStatuses = []RoutineStatus{RoutineProposed, RoutineAccepted, RoutineDismissed}
|
||||
|
||||
// Valid reports whether s is one of RoutineStatuses.
|
||||
func (s RoutineStatus) Valid() bool {
|
||||
for _, v := range RoutineStatuses {
|
||||
if s == v {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func (s RoutineStatus) String() string { return string(s) }
|
||||
|
||||
// ProposedRoutine — a detected pattern the system wants to nudge about on a
|
||||
// repeating interval. Status 'proposed' means awaiting human confirmation;
|
||||
// 'accepted' means the human confirmed and the tick loop now owns the schedule;
|
||||
@@ -30,7 +53,7 @@ type ProposedRoutine struct {
|
||||
Action string
|
||||
Object string
|
||||
IntervalDays float64
|
||||
Status string // proposed | accepted | dismissed
|
||||
Status RoutineStatus
|
||||
CreatedTs time.Time
|
||||
ReminderID *int64
|
||||
AcceptedTs *time.Time
|
||||
@@ -40,6 +63,7 @@ type ProposedRoutine struct {
|
||||
var (
|
||||
ErrProposedRoutineNotFound = errors.New("store: proposed routine not found")
|
||||
ErrProposedRoutineExists = errors.New("store: proposed routine already exists for this action+object")
|
||||
ErrRoutineStatus = errors.New("store: unknown routine status")
|
||||
)
|
||||
|
||||
// CreateProposedRoutine inserts a new proposed routine. Returns
|
||||
@@ -51,6 +75,16 @@ var (
|
||||
// keep finding the pattern, and every re-propose is refused here. Maven is not
|
||||
// a nag.
|
||||
//
|
||||
// The object stays a local string. It is not resolved against Nexus and it
|
||||
// carries no canonical entity ref (asked on the PR 4 review, decided here,
|
||||
// Vikunja #410). Nexus owns identity for things the ecosystem acts on, and
|
||||
// nothing acts on a routine object: it is the word he used, replayed back to
|
||||
// him in a nudge, and compared only against itself for the UNIQUE key. Two
|
||||
// spellings of the same watering can are two routines, and that is the right
|
||||
// answer when the point is to say the sentence he would say. Canonical refs
|
||||
// arrive here only if a routine ever drives a Hexis call, which is Vikunja
|
||||
// #272, not this.
|
||||
//
|
||||
// Vikunja #43: this is called both from the voice fact-write path (for the
|
||||
// immediate spoken confirmation) and from the digestion tick's proactive
|
||||
// scan (cmd/mavend/tick.go's detectPatterns, via patterns.go's
|
||||
@@ -102,8 +136,13 @@ func (s *Store) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, er
|
||||
}
|
||||
|
||||
// ListProposedRoutinesByStatus returns routines in one status, newest first.
|
||||
// An empty status returns every row.
|
||||
func (s *Store) ListProposedRoutinesByStatus(ctx context.Context, status string) ([]ProposedRoutine, error) {
|
||||
// An empty status returns every row; an unknown one is refused rather than
|
||||
// silently answering with nothing, since a typo and a genuinely empty state
|
||||
// read the same otherwise.
|
||||
func (s *Store) ListProposedRoutinesByStatus(ctx context.Context, status RoutineStatus) ([]ProposedRoutine, error) {
|
||||
if status != "" && !status.Valid() {
|
||||
return nil, fmt.Errorf("%w: %q", ErrRoutineStatus, status)
|
||||
}
|
||||
q := `SELECT id, action, object, interval_days, status, created_ts, reminder_id, accepted_ts, last_fired_ts
|
||||
FROM proposed_routines`
|
||||
var args []any
|
||||
|
||||
@@ -235,7 +235,7 @@ func TestListProposedRoutinesByStatus(t *testing.T) {
|
||||
}
|
||||
|
||||
cases := []struct {
|
||||
status string
|
||||
status RoutineStatus
|
||||
want int
|
||||
}{
|
||||
{RoutineProposed, 0},
|
||||
@@ -266,3 +266,59 @@ func TestLookupMissingProposedRoutine(t *testing.T) {
|
||||
t.Fatal("want nil for missing routine")
|
||||
}
|
||||
}
|
||||
|
||||
// Every legal status must survive the database. The status column is written
|
||||
// by three different UPDATE statements with the value spelled inline, so a
|
||||
// constant that drifts from its SQL is exactly the failure this catches: the
|
||||
// row would come back in a state no Go code compares equal to.
|
||||
func TestRoutineStatusRoundTrip(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
s := newTestStore(t)
|
||||
now := time.Now().UTC()
|
||||
|
||||
write := map[RoutineStatus]func(id int64) error{
|
||||
RoutineProposed: func(int64) error { return nil },
|
||||
RoutineAccepted: func(id int64) error { return s.AcceptProposedRoutine(ctx, id, now) },
|
||||
RoutineDismissed: func(id int64) error { return s.DismissProposedRoutine(ctx, id) },
|
||||
}
|
||||
|
||||
for _, want := range RoutineStatuses {
|
||||
if !want.Valid() {
|
||||
t.Fatalf("%q is in RoutineStatuses but not Valid()", want)
|
||||
}
|
||||
object := "object-" + want.String()
|
||||
id, err := s.CreateProposedRoutine(ctx, "полить", object, 3, now)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateProposedRoutine(%s): %v", want, err)
|
||||
}
|
||||
if err := write[want](id); err != nil {
|
||||
t.Fatalf("move to %s: %v", want, err)
|
||||
}
|
||||
got, err := s.LookupProposedRoutine(ctx, "полить", object)
|
||||
if err != nil || got == nil {
|
||||
t.Fatalf("LookupProposedRoutine(%s): %v", want, err)
|
||||
}
|
||||
if got.Status != want {
|
||||
t.Errorf("status = %q, want %q", got.Status, want)
|
||||
}
|
||||
list, err := s.ListProposedRoutinesByStatus(ctx, want)
|
||||
if err != nil {
|
||||
t.Fatalf("ListProposedRoutinesByStatus(%s): %v", want, err)
|
||||
}
|
||||
if len(list) != 1 {
|
||||
t.Errorf("status %s: listed %d rows, want 1", want, len(list))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A typo used to read as "nothing is in that state", which is the same answer
|
||||
// a correct query gives on an empty table.
|
||||
func TestListByStatusRefusesAnUnknownStatus(t *testing.T) {
|
||||
s := newTestStore(t)
|
||||
if _, err := s.ListProposedRoutinesByStatus(context.Background(), "accpeted"); !errors.Is(err, ErrRoutineStatus) {
|
||||
t.Fatalf("err = %v, want ErrRoutineStatus", err)
|
||||
}
|
||||
if RoutineStatus("accpeted").Valid() {
|
||||
t.Error("a typo must not be Valid()")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user