diff --git a/internal/server/handlers_items_copy_relation_test.go b/internal/server/handlers_items_copy_relation_test.go index cd750e82..0ff60c77 100644 --- a/internal/server/handlers_items_copy_relation_test.go +++ b/internal/server/handlers_items_copy_relation_test.go @@ -672,3 +672,54 @@ func TestCopyEndpoint_NullSourceWithoutDefaultIsNotReportedAsDefault(t *testing. // claiming a default that does not exist. Reaching here means the key was // absent from `carried`, so there is nothing mislabelled. } + +// A relation default that is not a reference at all must be reported, not +// silently stored (codex round 6). +// +// THE ROUTE MATTERS, and the review's account of it was not quite right. +// `MigrateFields` injects destination defaults itself, so in the ordinary case +// the key is present when `ValidateFieldsDetailed` runs and its type IS +// checked — a numeric default lands in needs_value with "must be a string", +// which is correct behaviour and not a defect. +// +// The unchecked route is narrower: a NULL OVERRIDE deletes the key after +// MigrateFields has filled it, so validation injects the default itself — and +// its own injection branch `continue`s PAST the type check. That is the one +// way a non-string reaches a relation field unchallenged, and the +// late-default pass owns values that arrive from defaults. +func TestCopyEndpoint_NonStringRelationDefaultIsReported(t *testing.T) { + f := newCopyRelationFixtureNonStringDefault(t) + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": nil} + + pre := f.ok(body) + reason, dropped := droppedReason(pre, "owner_ref") + if !dropped { + t.Fatalf("a non-reference default is neither carried nor dropped: %+v", pre.Fields) + } + if reason != "invalid_shape" { + t.Fatalf("dropped for reason %q, want invalid_shape — the value is not a reference, so "+ + "every other reason describes a lookup that never happened", reason) + } + + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "non-string relation default", body) + if v, present := f.persistedFields(res.Item.ID)["owner_ref"]; present { + t.Fatalf("the copy stored a non-reference in a relation field: %#v", v) + } +} + +// newCopyRelationFixtureNonStringDefault gives the DESTINATION's relation +// field a numeric default, which no valid schema would declare and which +// nothing in the pipeline type-checks. +func newCopyRelationFixtureNonStringDefault(t *testing.T) *relationFixture { + t.Helper() + f := newCopyRelationFixtureWith(t, noDestDefault, nil, false) + schema := fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"],"required":true}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q,"default":42} + ]}`, f.targetsB.Slug) + if _, err := f.srv.store.UpdateCollection(f.collB.ID, models.CollectionUpdate{Schema: &schema}); err != nil { + t.Fatalf("UpdateCollection(non-string default): %v", err) + } + return f +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index 4949e3b4..cd135cb6 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -2,6 +2,7 @@ package server import ( "net/http" + "strings" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" @@ -98,7 +99,12 @@ func (s *Server) resolveRelationReferentsAs( continue } id, isStr := raw.(string) - if !isStr || id == "" { + // TRIMMED, matching the store resolver: it ignores a whitespace-only + // value as "no reference", so an untrimmed check here refuses a value + // the store never objected to — and since the vanished-target arm + // below turns a missing lookup into a refusal, `" "` became a + // not_found instead of an empty field (codex round 6). + if !isStr || strings.TrimSpace(id) == "" { continue } if ri, already := issueForKey(issues, def.Key); already { diff --git a/internal/server/relation_referents_doors_test.go b/internal/server/relation_referents_doors_test.go index 4398b86b..c69eed3b 100644 --- a/internal/server/relation_referents_doors_test.go +++ b/internal/server/relation_referents_doors_test.go @@ -721,3 +721,24 @@ func TestRelationDoors_BulkUpdateResolvesInjectedRelationDefault(t *testing.T) { v, ok, f.target.ID, f.target.Ref) } } + +// A whitespace-only relation value is "no reference", not a bad one (codex +// round 6). +// +// The store resolver trims and ignores it. The server wrapper checked the +// UNTRIMMED string, so it fell through to the visibility loop — and since the +// vanished-target arm turns a missing lookup into a refusal, `" "` came back +// as not_found instead of an empty field. A defect my own round-1 fix +// introduced: before it, the same path did `continue`. +func TestRelationDoors_WhitespaceOnlyRelationIsNotRefused(t *testing.T) { + f := newDoorFixture(t) + + rr := f.call(f.srv.handleCreateItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/collections/"+f.tasks.Slug+"/items", + map[string]string{"collSlug": f.tasks.Slug}, + map[string]any{"title": "Blank relation", "fields": map[string]any{"owner_ref": " "}}) + if rr.Code != http.StatusCreated { + t.Fatalf("a whitespace-only relation was refused %d; the store treats it as no value "+ + "at all: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index cc34f5cb..f5607d94 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -48,6 +48,14 @@ const ( // so nothing can be checked against it. A schema problem, surfaced rather // than treated as permission to store anything. RelationTargetMissing RelationIssueReason = "target_missing" + // RelationTargetInvalidShape — the value is not a string at all, so it + // cannot name anything. Normally `ValidateFields` catches this and the + // resolver deliberately stays out of it (one error per defect), but an + // injected schema DEFAULT is never type-checked: ValidateFields assigns + // it and `continue`s past its own validation. That is the one route by + // which a non-string reaches a relation field unchallenged (codex round + // 6), so the late-default pass reports it rather than skipping it. + RelationTargetInvalidShape RelationIssueReason = "invalid_shape" ) // RelationIssue is one unresolvable relation value, carrying everything a @@ -68,6 +76,8 @@ func (ri RelationIssue) Message() string { return fmt.Sprintf("field %q: %q is not an item in collection %q", ri.Key, ri.Value, ri.Target) case RelationTargetMissing: return fmt.Sprintf("field %q declares no target collection, so %q cannot be resolved", ri.Key, ri.Value) + case RelationTargetInvalidShape: + return fmt.Sprintf("field %q has a default that is not a reference", ri.Key) default: return fmt.Sprintf("field %q: %q does not name an item in collection %q", ri.Key, ri.Value, ri.Target) } @@ -87,6 +97,7 @@ func RelationIssueReasons() []RelationIssueReason { RelationTargetWrongCollection, RelationTargetMissing, RelationTargetNotPortable, + RelationTargetInvalidShape, } } @@ -534,7 +545,16 @@ func (s *Store) MigrateRelationReferentsQ( if !exists { continue } - value, _ := raw.(string) + value, isStr := raw.(string) + if !isStr { + // Not a reference at all, so "cannot cross a workspace + // boundary" is a false account of why it is going. Left in + // place for ValidateFields to reject on shape — which is what + // the SAME-workspace branch already does with it, and the two + // modes disagreeing about one malformed value was the defect + // (codex round 6). + continue + } dropped = append(dropped, RelationIssue{ Key: def.Key, Value: value, Target: def.Collection, Reason: RelationTargetNotPortable, }) @@ -628,13 +648,27 @@ func (s *Store) ResolveLateRelationDefaultsQ( if !exists || raw == nil { continue } - if str, isStr := raw.(string); isStr && strings.TrimSpace(str) == "" { + str, isStr := raw.(string) + if !isStr { + // A default validation injected without type-checking it. The + // resolver cannot use it and nothing else will complain, so it is + // dropped and reported here. + dropped = append(dropped, RelationIssue{ + Key: def.Key, Target: def.Collection, Reason: RelationTargetInvalidShape, + }) + delete(fieldMap, def.Key) + continue + } + if strings.TrimSpace(str) == "" { continue } late[def.Key] = raw } if len(late) == 0 { - return nil, nil + // `dropped` may already carry non-string defaults rejected above, so + // this returns it rather than nil — an early `return nil, nil` here + // silently discarded them. + return dropped, nil } issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, late) if resolveErr != nil { diff --git a/web/src/lib/components/items/CopyItemDialog.svelte b/web/src/lib/components/items/CopyItemDialog.svelte index cf802b03..29283ca0 100644 --- a/web/src/lib/components/items/CopyItemDialog.svelte +++ b/web/src/lib/components/items/CopyItemDialog.svelte @@ -959,6 +959,8 @@ user hunting for an item that provably does not exist. return 'it refers to an item outside the field’s collection'; case 'target_missing': return 'the field declares no collection to link to'; + case 'invalid_shape': + return 'the destination field’s default is not a valid reference'; default: return reason; } diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index eab349bf..502cd181 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -799,7 +799,13 @@ export interface ItemCopyPreflightDropped { */ | 'not_found' | 'wrong_collection' - | 'target_missing'; + | 'target_missing' + /** + * The destination schema's default for this field is not a reference at + * all. Injected defaults are never type-checked, so this is the one + * route by which a non-string reaches a relation field. + */ + | 'invalid_shape'; } export interface ItemCopyPreflightNeedsValue {