diff --git a/internal/models/awaits_human.go b/internal/models/awaits_human.go index 155fffe1b..7c532e7b2 100644 --- a/internal/models/awaits_human.go +++ b/internal/models/awaits_human.go @@ -3,6 +3,7 @@ package models import ( "encoding/json" "errors" + "fmt" "strings" ) @@ -106,6 +107,12 @@ func boolSettingValid(settingsJSON, key string) bool { return false } +// ErrSettingsNotObject is wrapped by SetTracksWork and SetAwaitsHuman when the +// settings they merge into do not parse as a JSON object, so a handler can +// answer the caller 400 instead of 500 (BUG-3976). A full settings write +// replaces such a row; a typed member alone does not rewrite the rest. +var ErrSettingsNotObject = errors.New("the collection's settings are not a JSON object") + // setBoolSetting returns settingsJSON with key set to v and every other key // unchanged. func setBoolSetting(settingsJSON, key string, v bool) (string, error) { @@ -115,7 +122,7 @@ func setBoolSetting(settingsJSON, key string, v bool) (string, error) { } var m map[string]json.RawMessage if err := json.Unmarshal([]byte(raw), &m); err != nil || m == nil { - return "", errors.New("the collection's settings are not a JSON object; " + key + " cannot be set") + return "", fmt.Errorf("%w; %s cannot be set", ErrSettingsNotObject, key) } if v { m[key] = json.RawMessage("true") diff --git a/internal/server/bug3976_settings_not_object_test.go b/internal/server/bug3976_settings_not_object_test.go new file mode 100644 index 000000000..7faef1af1 --- /dev/null +++ b/internal/server/bug3976_settings_not_object_test.go @@ -0,0 +1,177 @@ +package server + +import ( + "encoding/json" + "errors" + "net/http" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-3976: a typed tracks_work / awaits_human member merges into the stored +// settings, and stored settings that are not a JSON object (import, a hand +// edit, an old row) made that write answer 500. It now answers a typed 400, +// changes nothing, and a full settings write still repairs the row. + +// nonObjectSettings are the probe table's shapes. Postgres stores settings as +// JSONB, so text that is not JSON cannot arise there and runs on SQLite only. +var nonObjectSettings = []struct { + raw string + sqliteOnly bool +}{ + {`[]`, false}, + {`"x"`, false}, + {`7`, false}, + {`not json`, true}, + {`null`, false}, +} + +var typedSettingMembers = []string{"tracks_work", "awaits_human"} + +type bug3976Fixture struct { + *accessFixture + base string +} + +func newBUG3976Fixture(t *testing.T, d store.DriverType) *bug3976Fixture { + f := newAccessFixture(t, d) + return &bug3976Fixture{accessFixture: f, base: "/api/v1/workspaces/" + f.wsSlug + "/collections"} +} + +// corruptCollection creates a collection and overwrites its stored settings +// with raw, as an import or a hand edit would. +func (f *bug3976Fixture) corruptCollection(name, raw string) models.Collection { + f.t.Helper() + rr := f.do("POST", f.base, f.ownerTok, map[string]any{"name": name}) + f.must(rr, http.StatusCreated, "create "+name) + var c models.Collection + parseJSON(f.t, rr, &c) + if _, err := f.srv.store.DB().Exec(f.srv.store.D().Rebind(`UPDATE collections SET settings = ? WHERE id = ?`), raw, c.ID); err != nil { + f.t.Fatalf("store settings %s: %v", raw, err) + } + return c +} + +func (f *bug3976Fixture) storedSettings(id string) string { + f.t.Helper() + var s string + if err := f.srv.store.DB().QueryRow(f.srv.store.D().Rebind(`SELECT settings FROM collections WHERE id = ?`), id).Scan(&s); err != nil { + f.t.Fatalf("read settings: %v", err) + } + return s +} + +func errorEnvelope(t *testing.T, body []byte) (code, message string) { + t.Helper() + var env struct { + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(body, &env); err != nil { + t.Fatalf("error envelope: %v (%s)", err, body) + } + return env.Error.Code, env.Error.Message +} + +// Door 1, the defect: a flags-only PATCH over corrupt stored settings. +func TestBUG3976_TypedMemberOverNonObjectStoredSettings(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newBUG3976Fixture(t, d) + for i, shape := range nonObjectSettings { + if shape.sqliteOnly && d != store.DriverSQLite { + continue + } + for _, member := range typedSettingMembers { + c := f.corruptCollection("Door1 "+member+" "+string(rune('A'+i)), shape.raw) + before := f.storedSettings(c.ID) + rr := f.do("PATCH", f.base+"/"+c.Slug, f.ownerTok, map[string]any{member: true}) + if rr.Code != http.StatusBadRequest { + t.Fatalf("stored %s, %s: got %d %s, want 400", shape.raw, member, rr.Code, rr.Body.String()) + } + code, msg := errorEnvelope(t, rr.Body.Bytes()) + want := "the collection's settings are not a JSON object; " + member + " cannot be set" + if code != "validation_error" || msg != want { + t.Fatalf("stored %s, %s: error %q %q, want validation_error %q", shape.raw, member, code, msg, want) + } + if after := f.storedSettings(c.ID); after != before { + t.Fatalf("stored %s, %s: a refused write changed settings %q -> %q", shape.raw, member, before, after) + } + } + } + }) +} + +// Doors 2 and 3: incoming settings that are not an object never reach the +// merge. The request decoder refuses them (IDEA-1488); these rows pin the +// measured answer (BUG-3976 checkpoint 1) so neither door regresses to a 500. +func TestBUG3976_IncomingNonObjectSettingsWithTypedMember(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newBUG3976Fixture(t, d) + for i, shape := range nonObjectSettings { + for _, member := range typedSettingMembers { + name := "Door3 " + member + " " + string(rune('A'+i)) + rr := f.do("POST", f.base, f.ownerTok, map[string]any{"name": name, "settings": shape.raw, member: true}) + if code, msg := errorEnvelope(t, rr.Body.Bytes()); rr.Code != http.StatusBadRequest || code != "bad_request" || msg != models.ErrInvalidSettingsType.Error() { + t.Fatalf("create, settings %s, %s: got %d %s", shape.raw, member, rr.Code, rr.Body.String()) + } + rr = f.do("POST", f.base, f.ownerTok, map[string]any{"name": "Door2 " + member + " " + string(rune('A'+i))}) + f.must(rr, http.StatusCreated, "create door 2 target") + var c models.Collection + parseJSON(t, rr, &c) + rr = f.do("PATCH", f.base+"/"+c.Slug, f.ownerTok, map[string]any{"settings": shape.raw, member: true}) + if code, msg := errorEnvelope(t, rr.Body.Bytes()); rr.Code != http.StatusBadRequest || code != "bad_request" || msg != models.ErrInvalidSettingsType.Error() { + t.Fatalf("full update, settings %s, %s: got %d %s", shape.raw, member, rr.Code, rr.Body.String()) + } + } + } + }) +} + +// Control: a full settings write repairs a corrupt row, and the typed member +// then works. The refusal leaves a way out. +func TestBUG3976_FullSettingsWriteRepairsTheRow(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newBUG3976Fixture(t, d) + for _, member := range typedSettingMembers { + c := f.corruptCollection("Repair "+member, `[]`) + f.must(f.do("PATCH", f.base+"/"+c.Slug, f.ownerTok, map[string]any{"settings": map[string]any{}}), http.StatusOK, "full settings write") + f.must(f.do("PATCH", f.base+"/"+c.Slug, f.ownerTok, map[string]any{member: false}), http.StatusOK, member+" after repair") + var got map[string]any + if err := json.Unmarshal([]byte(f.storedSettings(c.ID)), &got); err != nil { + t.Fatalf("%s: repaired settings are not an object: %v", member, err) + } + if got[member] != false { + t.Fatalf("%s: repaired settings %v, want %s=false", member, got, member) + } + } + }) +} + +// The sentinel at the store, for both members and both store doors (create +// and update), so the handlers' errors.Is has something to match. +func TestBUG3976_StoreWrapsErrSettingsNotObject(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newBUG3976Fixture(t, d) + yes := true + for _, member := range typedSettingMembers { + c := f.corruptCollection("Store "+member, `[]`) + upd := models.CollectionUpdate{} + create := models.CollectionCreate{Name: "Store create " + member, Settings: `[]`} + if member == "tracks_work" { + upd.TracksWork, create.TracksWork = &yes, &yes + } else { + upd.AwaitsHuman, create.AwaitsHuman = &yes, &yes + } + if _, err := f.srv.store.UpdateCollection(c.ID, upd); !errors.Is(err, models.ErrSettingsNotObject) { + t.Fatalf("UpdateCollection %s: err %v, want ErrSettingsNotObject", member, err) + } + if _, err := f.srv.store.CreateCollection(f.wsID, create); !errors.Is(err, models.ErrSettingsNotObject) { + t.Fatalf("CreateCollection %s: err %v, want ErrSettingsNotObject", member, err) + } + } + }) +} diff --git a/internal/server/handlers_collections.go b/internal/server/handlers_collections.go index e5f2ebedc..ae145d0d1 100644 --- a/internal/server/handlers_collections.go +++ b/internal/server/handlers_collections.go @@ -338,6 +338,14 @@ func (s *Server) handleCreateCollection(w http.ResponseWriter, r *http.Request) writeError(w, http.StatusBadRequest, "bad_request", v.Reason) return } + // BUG-3976, defensive: unreachable over HTTP today, because + // CollectionCreate.UnmarshalJSON already refuses settings that are not + // an object (IDEA-1488). Kept so the create door answers like update + // if that ever changes. + if errors.Is(err, models.ErrSettingsNotObject) { + writeError(w, http.StatusBadRequest, "validation_error", err.Error()) + return + } writeInternalError(w, err) return } @@ -546,6 +554,14 @@ func (s *Server) handleUpdateCollection(w http.ResponseWriter, r *http.Request) writeError(w, http.StatusBadRequest, "bad_request", v.Reason) return } + // BUG-3976: a typed tracks_work / awaits_human member over STORED + // settings that are not an object (import, a hand edit, an old row). + // The caller's input is fine but the write cannot be done; a full + // settings write repairs the row. + if errors.Is(err, models.ErrSettingsNotObject) { + writeError(w, http.StatusBadRequest, "validation_error", err.Error()) + return + } writeInternalError(w, err) return }