mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
fix: three codex round-3 findings — oracle, bulk defaults, required (TASK-2878)
Round 3 confirmed round 2's fixes and found three more. Codex still could not execute anything, so all three were static reads I verified myself. ## `wrong_collection` DISCLOSED EXISTENCE The store emits `wrong_collection` when a value names a LIVE item outside the field's declared target, and the server's visibility layer skipped any key that already carried an issue. So the message — "is not an item in collection X" — told a caller the value EXISTS, distinguishably from the `not_found` a nonexistent value gets. An existence oracle for anyone who cannot see that item, and the exact shape the `not_found` collapse exists to prevent. Collapsed to `not_found` when the requester cannot see the target, and KEPT otherwise: "you linked a task where a person belongs" is the useful half of this reason, and blanket-collapsing would have passed the security leg while destroying it. Both legs are in the test for that reason. Needed one new store export, `ResolveRelationTarget`: deciding what to disclose about a live item requires the item, and the resolver is the only thing that maps a value to one under the no-slug rule. ## BULK STATUS / PRIORITY BYPASSED RELATION DEFAULTS `bulkFieldUpdate` resolves only the keys `changes` names — correctly, since re-litigating stored values would freeze legacy items — but `ValidateFields` runs BEFORE it and INJECTS schema defaults. A defaulted relation was persisted raw: never canonicalised, never checked against its collection. The same late-arrival the migrate doors hit, reached by a different route, and the narrow pass built for them covers it unchanged. `dropped_fields` now rides every bulk op's activity row, not just the move branch: a status or priority change can discard a relation default too, and a drop nobody records is the defect BUG-2674 closed. ## A REQUIRED RELATION COULD END UP ABSENT AND REPORTED VALID The late pass deletes a key AFTER validation passed, so nothing re-checked required-ness: a required relation whose default did not resolve left the item with the field absent, and the preflight reported `valid: true`. Re-running validation is not the fix — it would re-inject the same broken default. There is no valid value for that field, so the doors refuse: `missing_required_fields` on move and bulk move, a FieldValidationError on the copy, and on the preflight a `needs_value` row with `valid: false`. That split is the one this pair has everywhere else — the preview says what is wrong, the copy refuses. ## Counterfactuals Remove the oracle collapse -> the visibility leg DETECTED. Remove the bulk-update late pass -> DETECTED. Remove the required-relation refusal at the copy door -> DETECTED. All build-checked first. Gates: internal/server ok 352.1s · internal/store ok 358.7s · internal/items ok · internal/mcp ok 21.0s · go vet clean · gofmt clean · make lint 0 issues. Postgres green on the parent commit (store 596.7s, server 294.7s, private container port 5481); re-running on this tree.
This commit is contained in:
@@ -2413,6 +2413,17 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) {
|
||||
writeInternalError(w, lateErr)
|
||||
return
|
||||
}
|
||||
// A REQUIRED relation whose default did not resolve cannot be left as a
|
||||
// drop: the key is deleted AFTER validation passed, so nothing re-checks
|
||||
// it and the item would land with a required field absent, reported valid
|
||||
// (codex round 3). Re-running validation is not the answer — it would
|
||||
// re-inject the same broken default. There is no valid value, so this
|
||||
// refuses.
|
||||
if req := store.RequiredRelationIssues(items.SchemaForMigratedFields(targetSchema), lateDropped); len(req) > 0 {
|
||||
writeError(w, http.StatusBadRequest, "missing_required_fields",
|
||||
"Required fields missing: "+relationIssuesMessage(req))
|
||||
return
|
||||
}
|
||||
for _, ri := range lateDropped {
|
||||
result.Dropped = append(result.Dropped, ri.Key)
|
||||
}
|
||||
|
||||
@@ -306,12 +306,15 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) {
|
||||
action = "moved"
|
||||
meta["from_collection"] = item.CollectionSlug
|
||||
meta["to_collection"] = req.Collection
|
||||
// Same key, same joined-string shape and the same reason as
|
||||
// handleMoveItem's: this map is map[string]string, and a raw array
|
||||
// renders as a Go map literal in the timeline (BUG-2628).
|
||||
if len(droppedFields) > 0 {
|
||||
meta["dropped_fields"] = strings.Join(droppedFields, ", ")
|
||||
}
|
||||
}
|
||||
// Same key, same joined-string shape and the same reason as
|
||||
// handleMoveItem's: this map is map[string]string, and a raw array
|
||||
// renders as a Go map literal in the timeline (BUG-2628). Outside the
|
||||
// move branch because a bulk STATUS or PRIORITY change can discard a
|
||||
// relation default too, and a drop nobody records is the defect
|
||||
// BUG-2674 closed.
|
||||
if len(droppedFields) > 0 {
|
||||
meta["dropped_fields"] = strings.Join(droppedFields, ", ")
|
||||
}
|
||||
s.logActivityWithMeta(workspaceID, item.ID, action, r, auditMeta(meta))
|
||||
|
||||
@@ -457,10 +460,10 @@ func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.I
|
||||
return s.bulkMoveCollection(r, workspaceID, item, req, visibleIDs, resolvedTarget, batchID, droppedFields)
|
||||
}
|
||||
// Status-only move = a field update on the same collection.
|
||||
return s.bulkFieldUpdate(r, workspaceID, item, map[string]any{"status": req.Status}, req.Force, visibleIDs, actor, source, batchID)
|
||||
return s.bulkFieldUpdate(r, workspaceID, item, map[string]any{"status": req.Status}, req.Force, visibleIDs, actor, source, batchID, droppedFields)
|
||||
|
||||
case "set-priority":
|
||||
return s.bulkFieldUpdate(r, workspaceID, item, map[string]any{"priority": req.Priority}, req.Force, visibleIDs, actor, source, batchID)
|
||||
return s.bulkFieldUpdate(r, workspaceID, item, map[string]any{"priority": req.Priority}, req.Force, visibleIDs, actor, source, batchID, droppedFields)
|
||||
|
||||
case "tag":
|
||||
return s.bulkTagUpdate(item, req.Tags, true, actor, source, batchID)
|
||||
@@ -491,7 +494,7 @@ func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.I
|
||||
// validates against the collection schema, runs the open-children guard
|
||||
// (unless force), and writes via UpdateItemWithPreCheck — the same path
|
||||
// the single PATCH handler uses. Used by status moves and set-priority.
|
||||
func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *models.Item, changes map[string]any, force bool, visibleIDs []string, actor, source, batchID string) (*models.Item, *bulkOpError) {
|
||||
func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *models.Item, changes map[string]any, force bool, visibleIDs []string, actor, source, batchID string, droppedFields *[]string) (*models.Item, *bulkOpError) {
|
||||
coll, err := s.store.GetCollection(item.CollectionID)
|
||||
if err != nil || coll == nil {
|
||||
return nil, &bulkOpError{message: "failed to load collection"}
|
||||
@@ -511,6 +514,12 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode
|
||||
|
||||
// Coerce strings to their declared types before validating (BUG-2850).
|
||||
fieldMap = items.CoerceFields(fieldMap, schema)
|
||||
// Snapshot before validation, which INJECTS schema defaults: this door's
|
||||
// relation pass looks only at the keys `changes` names, so a relation
|
||||
// default validation fills in was persisted raw — never canonicalised and
|
||||
// never checked against its target collection (codex round 3). The same
|
||||
// late-arrival the migrate doors hit, reached by a different route.
|
||||
relBefore := store.RelationKeysPresent(schema, fieldMap)
|
||||
if err := items.ValidateFields(fieldMap, schema); err != nil {
|
||||
return nil, &bulkOpError{message: err.Error(), code: "validation_error"}
|
||||
}
|
||||
@@ -546,6 +555,21 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode
|
||||
for k, v := range suppliedRelations {
|
||||
fieldMap[k] = v
|
||||
}
|
||||
lateDropped, lateErr := s.store.ResolveLateRelationDefaults(workspaceID, schema, fieldMap, relBefore)
|
||||
if lateErr != nil {
|
||||
return nil, &bulkOpError{message: lateErr.Error(), code: "internal_error"}
|
||||
}
|
||||
if req := store.RequiredRelationIssues(schema, lateDropped); len(req) > 0 {
|
||||
return nil, &bulkOpError{
|
||||
message: "required fields missing: " + relationIssuesMessage(req),
|
||||
code: "missing_required_fields",
|
||||
}
|
||||
}
|
||||
if droppedFields != nil && len(lateDropped) > 0 {
|
||||
for _, ri := range lateDropped {
|
||||
*droppedFields = append(*droppedFields, ri.Key)
|
||||
}
|
||||
}
|
||||
if err := s.checkUniqueFields(workspaceID, item.CollectionID, item.ID, schema, fieldMap); err != nil {
|
||||
return nil, &bulkOpError{message: err.Error(), code: "conflict"}
|
||||
}
|
||||
@@ -763,6 +787,18 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m
|
||||
if lateErr != nil {
|
||||
return nil, &bulkOpError{message: lateErr.Error(), code: "internal_error"}
|
||||
}
|
||||
// A REQUIRED relation whose default did not resolve cannot be left as a
|
||||
// drop: the key is deleted AFTER validation passed, so nothing re-checks
|
||||
// it and the item would land with a required field absent, reported valid
|
||||
// (codex round 3). Re-running validation is not the answer — it would
|
||||
// re-inject the same broken default. There is no valid value, so this
|
||||
// refuses.
|
||||
if req := store.RequiredRelationIssues(items.SchemaForMigratedFields(targetSchema), lateDropped); len(req) > 0 {
|
||||
return nil, &bulkOpError{
|
||||
message: "required fields missing: " + relationIssuesMessage(req),
|
||||
code: "missing_required_fields",
|
||||
}
|
||||
}
|
||||
for _, ri := range lateDropped {
|
||||
result.Dropped = append(result.Dropped, ri.Key)
|
||||
}
|
||||
|
||||
@@ -775,6 +775,18 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request)
|
||||
writeInternalError(w, fmt.Errorf("copy preflight: resolve relation defaults: %w", lateErr))
|
||||
return
|
||||
}
|
||||
// A REQUIRED relation whose default did not resolve becomes a needs_value
|
||||
// row rather than a drop, so the preview says what the copy will do:
|
||||
// the copy REFUSES this request, and `valid` must be false (codex round
|
||||
// 3). Reported through the same issues slice the required check feeds, so
|
||||
// it lands in the same bucket a genuinely missing required field does.
|
||||
for _, ri := range store.RequiredRelationIssues(items.SchemaForMigratedFields(targetSchema), lateDropped) {
|
||||
issues = append(issues, items.FieldIssue{
|
||||
Key: ri.Key,
|
||||
Kind: items.IssueRequired,
|
||||
Message: ri.Message(),
|
||||
})
|
||||
}
|
||||
for _, ri := range lateDropped {
|
||||
migrated.Dropped = append(migrated.Dropped, ri.Key)
|
||||
relationDropReason[ri.Key] = string(ri.Reason)
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
@@ -292,7 +293,7 @@ const (
|
||||
// for, so a value can be present in the migrated map having been chosen by the
|
||||
// DESTINATION rather than carried from the source — a third origin, and the
|
||||
// one that is invisible until a schema declares a default.
|
||||
func newCopyRelationFixtureWith(t *testing.T, destDefault destDefaultKind, sourceOwnerRef *string) *relationFixture {
|
||||
func newCopyRelationFixtureWith(t *testing.T, destDefault destDefaultKind, sourceOwnerRef *string, requiredRelation bool) *relationFixture {
|
||||
t.Helper()
|
||||
srv := testServer(t)
|
||||
bus := events.New()
|
||||
@@ -317,18 +318,21 @@ func newCopyRelationFixtureWith(t *testing.T, destDefault destDefaultKind, sourc
|
||||
t.Fatalf("CreateItem(targetB): %v", err)
|
||||
}
|
||||
|
||||
relSchema := func(targetSlug, def string) string {
|
||||
defaultClause := ""
|
||||
relSchema := func(targetSlug, def string, required bool) string {
|
||||
clauses := ""
|
||||
if def != "" {
|
||||
defaultClause = fmt.Sprintf(`,"default":%q`, def)
|
||||
clauses += fmt.Sprintf(`,"default":%q`, def)
|
||||
}
|
||||
if required {
|
||||
clauses += `,"required":true`
|
||||
}
|
||||
return fmt.Sprintf(`{"fields":[
|
||||
{"key":"status","label":"Status","type":"select","options":["open","done"],"required":true},
|
||||
{"key":"owner_ref","label":"Owner","type":"relation","collection":%q%s}
|
||||
]}`, targetSlug, defaultClause)
|
||||
]}`, targetSlug, clauses)
|
||||
}
|
||||
|
||||
collA := mustSchemaCollection(t, srv, wsA.ID, "Rel2 Tasks A", relSchema(targetsA.Slug, ""))
|
||||
collA := mustSchemaCollection(t, srv, wsA.ID, "Rel2 Tasks A", relSchema(targetsA.Slug, "", false))
|
||||
// A REF, not the UUID, and that is the discriminating choice: a UUID is
|
||||
// already its canonical form, so a default that was RESOLVED and one that
|
||||
// was dropped-then-re-injected-raw by ValidateFields produce IDENTICAL
|
||||
@@ -340,7 +344,7 @@ func newCopyRelationFixtureWith(t *testing.T, destDefault destDefaultKind, sourc
|
||||
case unresolvableDestDefault:
|
||||
destDef = badRef
|
||||
}
|
||||
collB := mustSchemaCollection(t, srv, wsB.ID, "Rel2 Tasks B", relSchema(targetsB.Slug, destDef))
|
||||
collB := mustSchemaCollection(t, srv, wsB.ID, "Rel2 Tasks B", relSchema(targetsB.Slug, destDef, requiredRelation))
|
||||
|
||||
sourceFields := `{"status":"open"}`
|
||||
if sourceOwnerRef != nil {
|
||||
@@ -377,7 +381,7 @@ func newCopyRelationFixtureWith(t *testing.T, destDefault destDefaultKind, sourc
|
||||
// and the default never enters the migrated map, which is the arrangement that
|
||||
// hid this.
|
||||
func TestCopyEndpoint_DestinationDefaultRelationIsNotDroppedAsNotPortable(t *testing.T) {
|
||||
f := newCopyRelationFixtureWith(t, resolvableDestDefault, nil)
|
||||
f := newCopyRelationFixtureWith(t, resolvableDestDefault, nil, false)
|
||||
body := f.baseBody()
|
||||
|
||||
pre := f.ok(body)
|
||||
@@ -409,7 +413,7 @@ func TestCopyEndpoint_DestinationDefaultRelationIsNotDroppedAsNotPortable(t *tes
|
||||
// P1, second half).
|
||||
func TestCopyEndpoint_EmptyCarriedRelationIsNotReportedDropped(t *testing.T) {
|
||||
empty := ""
|
||||
f := newCopyRelationFixtureWith(t, noDestDefault, &empty)
|
||||
f := newCopyRelationFixtureWith(t, noDestDefault, &empty, false)
|
||||
|
||||
pre := f.ok(f.baseBody())
|
||||
if reason, dropped := droppedReason(pre, "owner_ref"); dropped {
|
||||
@@ -492,7 +496,7 @@ func TestCopyEndpoint_InvisibleRelationOverrideIsRefused(t *testing.T) {
|
||||
// with StillDropped then suppressing the warning about it.
|
||||
func TestCopyEndpoint_LateInjectedRelationDefaultIsResolved(t *testing.T) {
|
||||
t.Run("a null override lets the default in, and it is resolved", func(t *testing.T) {
|
||||
f := newCopyRelationFixtureWith(t, resolvableDestDefault, nil)
|
||||
f := newCopyRelationFixtureWith(t, resolvableDestDefault, nil, false)
|
||||
body := f.baseBody()
|
||||
// Explicit null: DELETE the key, which is what makes the default the
|
||||
// only thing that can fill it — and it arrives after the resolver.
|
||||
@@ -508,7 +512,7 @@ func TestCopyEndpoint_LateInjectedRelationDefaultIsResolved(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("an unresolvable default is dropped and reported, not silently stored", func(t *testing.T) {
|
||||
f := newCopyRelationFixtureWith(t, unresolvableDestDefault, nil)
|
||||
f := newCopyRelationFixtureWith(t, unresolvableDestDefault, nil, false)
|
||||
body := f.baseBody()
|
||||
|
||||
pre := f.ok(body)
|
||||
@@ -529,3 +533,51 @@ func TestCopyEndpoint_LateInjectedRelationDefaultIsResolved(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// A REQUIRED relation whose default does not resolve must REFUSE, not land the
|
||||
// item with the field absent (codex round 3).
|
||||
//
|
||||
// The late pass deletes the key after validation has already passed, so
|
||||
// nothing re-checks required-ness. Re-running validation is not the fix — it
|
||||
// would re-inject the same broken default — so the doors refuse outright,
|
||||
// which is honest: there is no valid value for that field.
|
||||
//
|
||||
// The preflight reports it as needs_value rather than a hard error, which is
|
||||
// the split this pair has everywhere else: the preview says what is wrong, the
|
||||
// copy refuses. `valid` must be false either way.
|
||||
func TestCopyEndpoint_RequiredRelationWithBrokenDefaultIsRefused(t *testing.T) {
|
||||
f := newCopyRelationFixtureWith(t, unresolvableDestDefault, nil, true)
|
||||
body := f.baseBody()
|
||||
|
||||
preRR := f.call(f.owner, reqOpts{}, body)
|
||||
if preRR.Code != http.StatusOK {
|
||||
t.Fatalf("preflight: expected 200, got %d: %s", preRR.Code, preRR.Body.String())
|
||||
}
|
||||
var pre ItemCopyPreflight
|
||||
if err := json.Unmarshal(preRR.Body.Bytes(), &pre); err != nil {
|
||||
t.Fatalf("parse preflight: %v", err)
|
||||
}
|
||||
if pre.Valid {
|
||||
t.Fatalf("the preflight reports valid=true for a copy that cannot satisfy a required "+
|
||||
"relation: %+v", pre.Fields)
|
||||
}
|
||||
var flagged bool
|
||||
for _, nv := range pre.Fields.NeedsValue {
|
||||
if nv.Key == "owner_ref" {
|
||||
flagged = true
|
||||
}
|
||||
}
|
||||
if !flagged {
|
||||
t.Fatalf("owner_ref is not in needs_value, so the dialog cannot tell the user what to "+
|
||||
"supply: %+v", pre.Fields)
|
||||
}
|
||||
|
||||
before := f.snapshot()
|
||||
copyRR := f.callCopy(f.owner, reqOpts{}, body)
|
||||
if copyRR.Code != http.StatusBadRequest {
|
||||
t.Fatalf("copy: expected 400, got %d: %s", copyRR.Code, copyRR.Body.String())
|
||||
}
|
||||
if after := f.snapshot(); after != before {
|
||||
t.Fatalf("a refused copy mutated state:\n before: %+v\n after: %+v", before, after)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -101,8 +101,35 @@ func (s *Server) resolveRelationReferentsAs(
|
||||
if !isStr || id == "" {
|
||||
continue
|
||||
}
|
||||
if issuesContainKey(issues, def.Key) {
|
||||
continue // already unresolvable; no second complaint about it
|
||||
if ri, already := issueForKey(issues, def.Key); already {
|
||||
// `wrong_collection` is the one issue that names a LIVE item, so
|
||||
// its message ("is not an item in collection X") tells the caller
|
||||
// the value EXISTS — distinguishable from the `not_found` a
|
||||
// nonexistent value gets, and therefore an existence oracle for
|
||||
// anyone who cannot see that item (codex round 3). Collapse it to
|
||||
// `not_found` when the requester may not see the target; keep the
|
||||
// specific message when they may, because "you linked a task
|
||||
// where a person belongs" is the useful half of this reason.
|
||||
//
|
||||
// Any other issue is already `not_found`-shaped and needs nothing.
|
||||
if ri.Reason != store.RelationTargetWrongCollection {
|
||||
continue
|
||||
}
|
||||
target, terr := s.store.ResolveRelationTarget(workspaceID, ri.Value)
|
||||
if terr != nil {
|
||||
return nil, terr
|
||||
}
|
||||
if target == nil {
|
||||
continue // vanished since; already the safe answer
|
||||
}
|
||||
seen, verr := s.checkItemVisible(workspaceID, target, currentUser(r), role, isBearerAuth(r))
|
||||
if verr != nil {
|
||||
return nil, verr
|
||||
}
|
||||
if !seen {
|
||||
collapseIssue(issues, def.Key, store.RelationTargetNotFound)
|
||||
}
|
||||
continue
|
||||
}
|
||||
item, err := s.store.GetItem(id)
|
||||
if err != nil {
|
||||
@@ -136,13 +163,25 @@ func (s *Server) resolveRelationReferentsAs(
|
||||
return issues, nil
|
||||
}
|
||||
|
||||
func issuesContainKey(issues []store.RelationIssue, key string) bool {
|
||||
// issueForKey returns the issue already raised for key, if any.
|
||||
func issueForKey(issues []store.RelationIssue, key string) (store.RelationIssue, bool) {
|
||||
for _, ri := range issues {
|
||||
if ri.Key == key {
|
||||
return true
|
||||
return ri, true
|
||||
}
|
||||
}
|
||||
return store.RelationIssue{}, false
|
||||
}
|
||||
|
||||
// collapseIssue rewrites the reason of the issue already raised for key. In
|
||||
// place, because the slice is what the caller renders.
|
||||
func collapseIssue(issues []store.RelationIssue, key string, reason store.RelationIssueReason) {
|
||||
for i := range issues {
|
||||
if issues[i].Key == key {
|
||||
issues[i].Reason = reason
|
||||
return
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// refuseRelationIssues writes the 400 a write door owes and reports whether it
|
||||
|
||||
@@ -320,7 +320,7 @@ func TestRelationDoors_BulkUpdateSuppliedBranch(t *testing.T) {
|
||||
item := f.seed(`{"status":"open"}`)
|
||||
|
||||
_, opErr := f.srv.bulkFieldUpdate(f.requestFor(), f.ws.ID, item,
|
||||
map[string]any{"owner_ref": badRef}, true, nil, f.owner.ID, "test", "batch")
|
||||
map[string]any{"owner_ref": badRef}, true, nil, f.owner.ID, "test", "batch", nil)
|
||||
if opErr == nil {
|
||||
t.Fatalf("bulkFieldUpdate accepted an unresolvable supplied referent")
|
||||
}
|
||||
@@ -337,7 +337,7 @@ func TestRelationDoors_BulkUpdateSuppliedBranch(t *testing.T) {
|
||||
item := f.seed(`{"status":"open"}`)
|
||||
|
||||
if _, opErr := f.srv.bulkFieldUpdate(f.requestFor(), f.ws.ID, item,
|
||||
map[string]any{"owner_ref": f.target.Ref}, true, nil, f.owner.ID, "test", "batch"); opErr != nil {
|
||||
map[string]any{"owner_ref": f.target.Ref}, true, nil, f.owner.ID, "test", "batch", nil); opErr != nil {
|
||||
t.Fatalf("bulkFieldUpdate refused a resolvable referent: %s", opErr.message)
|
||||
}
|
||||
// Supplied as a ref; the ID is what must be stored. Without the
|
||||
@@ -627,3 +627,97 @@ func (f *doorFixture) callAs(user *models.User, role string, h http.HandlerFunc,
|
||||
h(rr, r.WithContext(ctx))
|
||||
return rr
|
||||
}
|
||||
|
||||
// `wrong_collection` names a LIVE item, so its message tells the caller the
|
||||
// value exists — distinguishable from the `not_found` a nonexistent value
|
||||
// gets, and therefore an existence oracle for anyone who cannot see that item
|
||||
// (codex round 3).
|
||||
//
|
||||
// Both legs are required. Collapsing every wrong_collection to not_found would
|
||||
// pass the first and destroy the second, and "you linked a task where a person
|
||||
// belongs" is the useful half of this reason.
|
||||
func TestRelationDoors_WrongCollectionDoesNotDiscloseExistence(t *testing.T) {
|
||||
f := newDoorFixture(t)
|
||||
// A live item in a collection that is NOT the relation's declared target.
|
||||
other := mustSchemaCollection(t, f.srv, f.ws.ID, "Vaults", `{"fields":[]}`)
|
||||
secret, err := f.srv.store.CreateItem(f.ws.ID, other.ID, models.ItemCreate{
|
||||
Title: "Secret", CreatedBy: f.owner.ID,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateItem(secret): %v", err)
|
||||
}
|
||||
|
||||
body := map[string]any{"title": "New", "fields": map[string]any{"owner_ref": secret.Ref}}
|
||||
path := "/api/v1/workspaces/" + f.ws.Slug + "/collections/" + f.tasks.Slug + "/items"
|
||||
params := map[string]string{"collSlug": f.tasks.Slug}
|
||||
|
||||
// The owner can see Vaults, so they get the specific, useful reason.
|
||||
seeing := f.call(f.srv.handleCreateItem, "POST", path, params, body)
|
||||
if seeing.Code != http.StatusBadRequest {
|
||||
t.Fatalf("expected 400, got %d: %s", seeing.Code, seeing.Body.String())
|
||||
}
|
||||
if !strings.Contains(seeing.Body.String(), "is not an item in collection") {
|
||||
t.Fatalf("a caller who CAN see the target lost the wrong_collection reason: %s",
|
||||
seeing.Body.String())
|
||||
}
|
||||
|
||||
// An editor with no access to Vaults must not be able to tell that
|
||||
// `secret.Ref` names anything at all.
|
||||
blind := mustUser(t, f.srv, "blind-oracle@example.com", "blindoracle", "")
|
||||
if err := f.srv.store.AddWorkspaceMember(f.ws.ID, blind.ID, "editor"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
if err := f.srv.store.SetMemberCollectionAccess(f.ws.ID, blind.ID, "specific",
|
||||
[]string{f.tasks.ID, f.people.ID}); err != nil {
|
||||
t.Fatalf("SetMemberCollectionAccess: %v", err)
|
||||
}
|
||||
|
||||
hidden := f.callAs(blind, "editor", f.srv.handleCreateItem, "POST", path, params, body)
|
||||
if hidden.Code != http.StatusBadRequest {
|
||||
t.Fatalf("expected 400, got %d: %s", hidden.Code, hidden.Body.String())
|
||||
}
|
||||
if strings.Contains(hidden.Body.String(), "is not an item in collection") {
|
||||
t.Fatalf("the refusal tells a caller who cannot see the target that it EXISTS: %s",
|
||||
hidden.Body.String())
|
||||
}
|
||||
if !strings.Contains(hidden.Body.String(), "does not name an item") {
|
||||
t.Fatalf("expected the not_found phrasing, got: %s", hidden.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// A bulk status or priority change must resolve a relation default validation
|
||||
// injects (codex round 3).
|
||||
//
|
||||
// `bulkFieldUpdate` looks only at the keys `changes` names — correctly, since
|
||||
// re-litigating stored values would freeze legacy items — but `ValidateFields`
|
||||
// runs first and INJECTS schema defaults, so a defaulted relation was
|
||||
// persisted raw: never canonicalised, never checked against its collection.
|
||||
func TestRelationDoors_BulkUpdateResolvesInjectedRelationDefault(t *testing.T) {
|
||||
f := newDoorFixture(t)
|
||||
defaulted := mustSchemaCollection(t, f.srv, f.ws.ID, "Defaulted", fmt.Sprintf(`{"fields":[
|
||||
{"key":"status","label":"Status","type":"select","options":["open","done"]},
|
||||
{"key":"priority","label":"Priority","type":"select","options":["low","high"]},
|
||||
{"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"default":%q}
|
||||
]}`, f.people.Slug, f.target.Ref))
|
||||
|
||||
item, err := f.srv.store.CreateItem(f.ws.ID, defaulted.ID, models.ItemCreate{
|
||||
Title: "Needs a default", Fields: `{"status":"open"}`, CreatedBy: f.owner.ID,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateItem: %v", err)
|
||||
}
|
||||
|
||||
rr := f.call(f.srv.handleBulkItems, "POST",
|
||||
"/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil,
|
||||
map[string]any{"op": "set-priority", "ids": []string{item.ID}, "priority": "high"})
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("bulk set-priority: expected 200, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
// The default is declared as a REF; only a resolved one lands as the id.
|
||||
if v, ok := f.storedRelation(item.ID); !ok || v != f.target.ID {
|
||||
t.Fatalf("stored owner_ref = %#v (present=%v), want the default RESOLVED to %q; the raw "+
|
||||
"ref %q means validation injected it after the relation pass had finished",
|
||||
v, ok, f.target.ID, f.target.Ref)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1155,6 +1155,15 @@ func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON,
|
||||
if lateErr != nil {
|
||||
return nil, nil, fmt.Errorf("copy item across workspaces: resolve relation defaults: %w", lateErr)
|
||||
}
|
||||
// A REQUIRED relation whose default did not resolve cannot be left as a
|
||||
// drop: the key is deleted AFTER validation passed, so nothing re-checks
|
||||
// it and the item would land with a required field absent, reported valid
|
||||
// (codex round 3). Re-running validation is not the answer — it would
|
||||
// re-inject the same broken default. There is no valid value, so this
|
||||
// refuses.
|
||||
if req := RequiredRelationIssues(items.SchemaForMigratedFields(targetSchema), lateDropped); len(req) > 0 {
|
||||
return nil, nil, &FieldValidationError{Err: errors.New(RelationIssuesMessage(req))}
|
||||
}
|
||||
for _, ri := range lateDropped {
|
||||
migrated.Dropped = append(migrated.Dropped, ri.Key)
|
||||
}
|
||||
|
||||
@@ -224,6 +224,41 @@ func (s *Store) ResolveRelationReferentsQ(
|
||||
return issues, nil
|
||||
}
|
||||
|
||||
// ResolveRelationTarget resolves ONE relation value to its item, or (nil, nil)
|
||||
// when nothing in the workspace answers to it.
|
||||
//
|
||||
// Exported for the server's visibility layer. `wrong_collection` names a LIVE
|
||||
// item, so the message distinguishes "exists, elsewhere" from "does not
|
||||
// exist" — an existence oracle unless the server can first check whether the
|
||||
// requester may see that item, which needs the item. Same UUID-or-ref rule as
|
||||
// everything else here: no slug fallback.
|
||||
func (s *Store) ResolveRelationTarget(workspaceID, value string) (*models.Item, error) {
|
||||
return s.resolveRelationTargetQ(s.Q(), workspaceID, value)
|
||||
}
|
||||
|
||||
// RequiredRelationIssues returns the subset of issues whose field the schema
|
||||
// declares REQUIRED.
|
||||
//
|
||||
// A dropped value in a required relation field cannot be left as a drop: the
|
||||
// key is deleted after validation has already passed, so nothing re-checks it
|
||||
// and the item lands with a required field absent. Callers turn these into
|
||||
// their own door's missing-required refusal.
|
||||
func RequiredRelationIssues(schema models.CollectionSchema, issues []RelationIssue) []RelationIssue {
|
||||
required := map[string]bool{}
|
||||
for _, def := range schema.Fields {
|
||||
if def.Type == "relation" && def.Required {
|
||||
required[def.Key] = true
|
||||
}
|
||||
}
|
||||
var out []RelationIssue
|
||||
for _, ri := range issues {
|
||||
if required[ri.Key] {
|
||||
out = append(out, ri)
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// resolveRelationTarget looks a value up by UUID or by issue ref/slug, scoped
|
||||
// to the workspace. Returns (nil, nil) when nothing answers to it.
|
||||
//
|
||||
|
||||
Reference in New Issue
Block a user