diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index ca7c863b..85d99b9d 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -592,13 +592,20 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) // existing intra-workspace move path (Codex round 5). Migration // matches on key and type only — plus the option list for a select. // It does not consider a relation field's `collection`, `computed`, - // `terminal_options` or `unique_scope`. So a same-named `relation` - // field carries a SOURCE-workspace item id into the destination and - // is reported as a clean carry, when across workspaces it is a - // dangling reference. Reporting that here would require the preflight - // to model semantics MigrateFields does not, and the copy would then - // disagree with its own preview — the divergence DR-6 exists to - // prevent. It belongs in MigrateFields, for both callers at once. + // `terminal_options` or `unique_scope`. + // + // The RELATION half of that is closed as of TASK-2878, and not where + // this comment predicted. A same-named `relation` field used to carry a + // SOURCE-workspace item id into the destination and be reported as a + // clean carry, when across workspaces it is a dangling reference. The fix + // did NOT go into MigrateFields: that function is in `internal/items`, + // which is DB-free by construction, and deciding whether a string names a + // live item in a particular collection is a database question. It went + // into `store.MigrateRelationReferents`, called below by this endpoint and + // by `migrateCopyFields` — one function, so the preview and the copy still + // cannot disagree, which is what the original objection was actually + // about. `computed`, `terminal_options` and `unique_scope` remain + // unmodelled here. // // MigrateFields computes result.Errors before any override exists, so // those errors are stale the instant an override merges in. They are @@ -649,6 +656,55 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) // preflight exists to PREDICT what the copy does, so a coercion on one // side only would make it report a field as failing that the copy accepts. final = items.CoerceFields(final, items.SchemaForMigratedFields(targetSchema)) + // Relation referents (TASK-2878), through the SAME store function the + // mutating copy calls — which is the whole reason that function is in + // `store` rather than beside either caller. This endpoint and + // `migrateCopyFields` sit in different PACKAGES, and the note above the + // CoerceFields call says in as many words that this is how the two drift + // unnoticed; a preview that says "carried" while the copy drops is one + // request answered two ways, the exact DR-6 divergence this pair exists to + // prevent. + // + // POOL executor (`s.store.MigrateRelationReferents`, not the ...Q form): + // the preflight is a read-only dry run holding no transaction. The copy + // passes its tx for the reason recorded at that call site. + // + // Scope computed the same way MigrateFields was given it, a few lines + // above — not re-derived, so the two cannot disagree about whether this + // request crosses a boundary. + relMode := store.RelationCarryWithinWorkspace + if items.ScopeFor(item.WorkspaceID, dst.WorkspaceID()) == items.CrossWorkspace { + relMode = store.RelationCarryCrossWorkspace + } + relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( + dst.WorkspaceID(), items.SchemaForMigratedFields(targetSchema), final, + input.FieldOverrides, relMode) + if relErr != nil { + writeInternalError(w, fmt.Errorf("copy preflight: resolve relation referents: %w", relErr)) + return + } + // An override the caller typed that names nothing is REFUSED here, not + // bucketed into needs_value — DR-12's disposition for an override with an + // invalid value, which is the branch immediately below. Same 400 + // validation_error and the same sentence the copy returns, because both + // render through store.RelationIssuesMessage. + if refuseRelationIssues(w, relRefusals) { + return + } + // Carried values that could not survive join `migrated.Dropped` rather + // than a bucket of their own, so `StillDropped` filters them against the + // final map exactly as it filters a type-mismatch drop — including the + // case where the destination schema's own default re-populates the key, + // which makes it a carry again and must not also be reported dropped. + // `origin` loses its entry for the same reason: if a default does + // re-populate the key, its origin is the destination's default, not the + // source value that was just discarded. + relationDropReason := make(map[string]string, len(relDropped)) + for _, ri := range relDropped { + migrated.Dropped = append(migrated.Dropped, ri.Key) + relationDropReason[ri.Key] = string(ri.Reason) + delete(origin, ri.Key) + } issues := items.ValidateFieldsDetailed(final, items.SchemaForMigratedFields(targetSchema)) // DR-12's other half: an override whose VALUE is invalid is rejected, @@ -785,6 +841,21 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) for _, key := range sortedDroppedKeys(items.StillDropped(migrated.Dropped, final), sourceSchema.Fields) { reason := "no_target_field" label := key + // A relation value whose referent did not survive (TASK-2878). The + // reason is the resolver's own — `referent_not_portable` for a carried + // value on a cross-workspace copy, or the specific lookup failure + // within a workspace — because the generic no_target_field is simply + // false here: the destination DOES declare the key, and reporting a + // missing field would send the reader to fix a schema that is fine. + if relReason, isRelation := relationDropReason[key]; isRelation { + if def, exists := targetDefs[key]; exists && def.Label != "" { + label = def.Label + } + resp.Fields.Dropped = append(resp.Fields.Dropped, ItemCopyPreflightDropped{ + Key: key, Label: label, Kind: "field", Reason: relReason, + }) + continue + } // A reserved key in Dropped can only have got there one way: it is // referential and this is a cross-workspace copy. The generic // no_target_field would be actively misleading — no schema declares diff --git a/internal/server/handlers_items_copy_relation_test.go b/internal/server/handlers_items_copy_relation_test.go new file mode 100644 index 00000000..7918aaf7 --- /dev/null +++ b/internal/server/handlers_items_copy_relation_test.go @@ -0,0 +1,272 @@ +package server + +import ( + "fmt" + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/events" + "github.com/PerpetualSoftware/pad/internal/models" +) + +// Relation referents across the two CROSS-WORKSPACE doors (PLAN-2857 U1 / +// TASK-2878). +// +// THIS IS THE PIN THE COPY PAIR HAD TO LAND WITH, and it is deliberately not a +// per-door table. `handleCopyItemPreflight` lives in `internal/server` and +// `migrateCopyFields` lives in `internal/store`; the code at both sites carries +// a note saying that is exactly how the two drift unnoticed. A table with a +// row per door can be fully green while the two doors disagree about the same +// request — which is the defect, not a gap in coverage of it. So every case +// here sends ONE body to BOTH endpoints and asserts the two answers are the +// same answer. +// +// The three legs are chosen to discriminate rather than to enumerate: +// +// - carried — the defect being closed. A relation value naming a live item +// in the SOURCE workspace used to cross the boundary and be reported as a +// clean carry. It must now drop on both doors, and the preflight must say +// WHY in a way that is true (referent_not_portable, not no_target_field). +// - supplied+unresolvable — the refusal half. Both doors must refuse, with +// the same status and code, and nothing may be written. +// - supplied+resolvable — the positive control. Without it the first two +// legs are equally consistent with "relations always fail", which would +// pass a build that simply refused every relation value. + +// relationFixture builds the copy fixture with relation-bearing schemas on +// both sides. It reuses copyPreflightFixture so every existing helper — +// call / callCopy / ok / copyOK / persistedFields / snapshot — drives these +// collections unchanged; only the schemas and the source item differ. +type relationFixture struct { + *copyPreflightFixture + targetsA, targetsB *models.Collection + // targetA is a live item in workspace A's target collection: the referent + // the source item's carried relation names, so the carried value is VALID + // where it starts. A value that was already broken in A would drop for a + // reason that has nothing to do with crossing the boundary. + targetA *models.Item + // targetB is its counterpart in B, the only thing an override can name. + targetB *models.Item +} + +func newCopyRelationFixture(t *testing.T) *relationFixture { + t.Helper() + srv := testServer(t) + bus := events.New() + srv.SetEventBus(bus) + t.Cleanup(bus.Close) + + owner := mustUser(t, srv, "rel-owner@example.com", "relowner", "") + wsA := mustWorkspace(t, srv, "Rel Source WS", owner.ID) + wsB := mustWorkspace(t, srv, "Rel Dest WS", owner.ID) + + // The target collections first: their SLUGS go into the relation + // FieldDefs, and a hardcoded guess at how a name slugifies would make + // this test's premise depend on CreateCollection's naming rather than on + // the behaviour under test. + targetsA := mustSchemaCollection(t, srv, wsA.ID, "People A", `{"fields":[]}`) + targetsB := mustSchemaCollection(t, srv, wsB.ID, "People B", `{"fields":[]}`) + + relSchema := func(targetSlug string) string { + return fmt.Sprintf(`{"fields":[ + {"key":"status","label":"Status","type":"select","options":["open","done"],"required":true}, + {"key":"owner_ref","label":"Owner","type":"relation","collection":%q} + ]}`, targetSlug) + } + + collA := mustSchemaCollection(t, srv, wsA.ID, "Rel Tasks A", relSchema(targetsA.Slug)) + collB := mustSchemaCollection(t, srv, wsB.ID, "Rel Tasks B", relSchema(targetsB.Slug)) + + targetA, err := srv.store.CreateItem(wsA.ID, targetsA.ID, models.ItemCreate{ + Title: "Ada in A", CreatedBy: owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(targetA): %v", err) + } + targetB, err := srv.store.CreateItem(wsB.ID, targetsB.ID, models.ItemCreate{ + Title: "Grace in B", CreatedBy: owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(targetB): %v", err) + } + + source, err := srv.store.CreateItem(wsA.ID, collA.ID, models.ItemCreate{ + Title: "The Related Source", + Content: "body", + Fields: fmt.Sprintf(`{"status":"open","owner_ref":%q}`, targetA.ID), + CreatedBy: owner.ID, + }) + if err != nil { + t.Fatalf("CreateItem(source): %v", err) + } + + return &relationFixture{ + copyPreflightFixture: ©PreflightFixture{ + t: t, srv: srv, bus: bus, owner: owner, + wsA: wsA, wsB: wsB, collA: collA, collB: collB, hiddenB: targetsB, + source: source, + }, + targetsA: targetsA, targetsB: targetsB, + targetA: targetA, targetB: targetB, + } +} + +// droppedReason returns the reason the preflight gave for key, and whether it +// reported the key dropped at all. +func droppedReason(pre ItemCopyPreflight, key string) (string, bool) { + for _, d := range pre.Fields.Dropped { + if d.Key == key { + return d.Reason, true + } + } + return "", false +} + +// carriedValue returns the value the preflight says will land under key. +func carriedValue(pre ItemCopyPreflight, key string) (any, bool) { + for _, c := range pre.Fields.Carried { + if c.Key == key { + return c.Value, true + } + } + return nil, false +} + +func TestCopyEndpoint_PreflightAndCopyAgreeOnRelationReferents(t *testing.T) { + t.Run("a carried relation drops identically on both doors", func(t *testing.T) { + f := newCopyRelationFixture(t) + body := f.baseBody() + + pre := f.ok(body) + + // The defect, stated positively: the value named a live item in A and + // used to arrive in B still naming it. It must not be reported as a + // carry. + if v, carried := carriedValue(pre, "owner_ref"); carried { + t.Fatalf("preflight reports owner_ref CARRYING %#v; a source-workspace "+ + "referent means nothing in the destination", v) + } + reason, dropped := droppedReason(pre, "owner_ref") + if !dropped { + t.Fatalf("preflight reports owner_ref in NEITHER bucket — a field that "+ + "silently vanishes is worse than one reported wrongly: %+v", pre.Fields) + } + // The reason is load-bearing. `no_target_field` is what the generic + // path would have said and it is false: the destination declares + // owner_ref, so that answer sends the reader to fix a schema that is + // fine. + if reason != "referent_not_portable" { + t.Fatalf("preflight dropped owner_ref for reason %q, want referent_not_portable", reason) + } + + // The copy's turn — same body, and the agreement assertion is the + // point of the pin. + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "carried relation", body) + + got := f.persistedFields(res.Item.ID) + if v, present := got["owner_ref"]; present { + t.Fatalf("the copy PERSISTED owner_ref = %#v, a workspace-A item id stored "+ + "on a workspace-B item", v) + } + // And the copy's own warnings report it, through the dropped_fields + // channel BUG-2674 established — a copy that drops a field silently is + // the defect that convention closed, in a new place. + if !hasDroppedField(res, "owner_ref") { + t.Fatalf("the copy dropped owner_ref without reporting it in warnings.dropped_fields: %+v", res) + } + }) + + t.Run("an unresolvable supplied override refuses identically on both doors", func(t *testing.T) { + f := newCopyRelationFixture(t) + body := f.baseBody() + // A ref shaped like a real one and naming nothing. Not a slug and not + // free text: those are refused by the resolver for a DIFFERENT reason + // (no slug fallback), and a fixture rejectable two ways discriminates + // nothing. + body["field_overrides"] = map[string]any{"owner_ref": "PEOP-9999"} + + before := f.snapshot() + + preRR := f.call(f.owner, reqOpts{}, body) + copyRR := f.callCopy(f.owner, reqOpts{}, body) + + if preRR.Code != http.StatusBadRequest { + t.Fatalf("preflight: expected 400, got %d: %s", preRR.Code, preRR.Body.String()) + } + if copyRR.Code != preRR.Code { + t.Fatalf("the copy answered %d where the preflight answered %d — the preview "+ + "lies about whether the request is acceptable:\n preflight: %s\n copy: %s", + copyRR.Code, preRR.Code, preRR.Body.String(), copyRR.Body.String()) + } + if pc, cc := errCode(t, preRR), errCode(t, copyRR); pc != cc { + t.Fatalf("error codes differ: preflight %q, copy %q", pc, cc) + } + if got := errCode(t, copyRR); got != "validation_error" { + t.Fatalf("copy error code = %q, want validation_error", got) + } + // Both refusals name the offending value, so a client can fix its + // request rather than guess which override was rejected. + for _, rr := range []struct { + label string + body string + }{{"preflight", preRR.Body.String()}, {"copy", copyRR.Body.String()}} { + if !containsAll(rr.body, "owner_ref", "PEOP-9999") { + t.Errorf("%s refusal does not name the field and value: %s", rr.label, rr.body) + } + } + + // Nothing was written. "Refused" and "created it anyway without the + // key" both produce a non-201 if only the status is checked. + if after := f.snapshot(); after != before { + t.Fatalf("a refused copy mutated state:\n before: %+v\n after: %+v", before, after) + } + }) + + t.Run("a resolvable supplied override carries, canonicalised", func(t *testing.T) { + f := newCopyRelationFixture(t) + body := f.baseBody() + // Supplied as a REF, not a UUID: resolving is then visible in the + // result, because what lands is the target's ID. A door that accepted + // the string without resolving would store "PEOP-1" and pass an + // equality check against itself. + body["field_overrides"] = map[string]any{"owner_ref": f.targetB.Ref} + + pre := f.ok(body) + v, carried := carriedValue(pre, "owner_ref") + if !carried { + t.Fatalf("preflight does not carry a resolvable override: %+v", pre.Fields) + } + if v != f.targetB.ID { + t.Fatalf("preflight carries owner_ref = %#v, want the resolved id %q — a ref "+ + "that survives unresolved is a value the destination cannot render", v, f.targetB.ID) + } + + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "resolvable relation override", body) + + got := f.persistedFields(res.Item.ID) + if got["owner_ref"] != f.targetB.ID { + t.Fatalf("the copy persisted owner_ref = %#v, want %q", got["owner_ref"], f.targetB.ID) + } + }) +} + +// hasDroppedField reports whether the copy's own response warned that key was +// dropped. +func hasDroppedField(res ItemCopyResult, key string) bool { + for _, k := range res.Warnings.DroppedFields { + if k == key { + return true + } + } + return false +} + +func containsAll(s string, subs ...string) bool { + for _, sub := range subs { + if !strings.Contains(s, sub) { + return false + } + } + return true +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index bef77a8f..9697a3fb 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -2,7 +2,6 @@ package server import ( "net/http" - "strings" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" @@ -124,10 +123,12 @@ func refuseRelationIssues(w http.ResponseWriter, issues []store.RelationIssue) b // and the two bulk operations (*bulkOpError). Split out rather than duplicated // so a caller cannot accidentally produce a different sentence for the same // refusal depending on which door it came through. +// +// DELEGATES to store.RelationIssuesMessage rather than joining here (TASK-2878). +// The eighth door refuses inside `internal/store` — the cross-workspace copy, +// through *FieldValidationError — so the sentence has to be reachable from +// there too. Two joins in two packages is how one refusal acquires two +// phrasings, which is the drift this unit exists to remove. func relationIssuesMessage(issues []store.RelationIssue) string { - parts := make([]string, 0, len(issues)) - for _, ri := range issues { - parts = append(parts, ri.Message()) - } - return strings.Join(parts, "; ") + return store.RelationIssuesMessage(issues) } diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 12a7ba94..9322fd94 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -621,7 +621,17 @@ func (s *Store) copyItemAcrossWorkspacesTx(req CrossWorkspaceCopyRequest, source // computes it the same way — a divergence here would have the preview // promising a carry the copy drops, which DR-6 exists to prevent. scope := items.ScopeFor(sourceWorkspaceID, req.TargetWorkspaceID) - finalFields, dropped, err := migrateCopyFields(source.Fields, sourceColl.Schema, targetColl.Schema, req.FieldOverrides, scope) + // The TRANSACTION is the executor, not the pool. This function now reads + // (relation referents, TASK-2878), and `copyItemAcrossWorkspacesTx` has + // held a transaction since its second statement — a pool read from inside + // it can wait for a free connection while every pooled connection is + // itself blocked on this transaction's locks, which is the starvation + // shape BUG-2409 fixed for the attachment planner and this repo keeps a + // deterministic test for. + // + // The DESTINATION workspace id, not the source's: a supplied override is + // a write into workspace B and has to name something that exists THERE. + finalFields, dropped, err := s.migrateCopyFields(tx, req.TargetWorkspaceID, source.Fields, sourceColl.Schema, targetColl.Schema, req.FieldOverrides, scope) if err != nil { return nil, err } @@ -1045,7 +1055,7 @@ func (s *Store) getCollectionInWorkspaceTx(tx *sql.Tx, collectionID, workspaceID // // Returns the final field map (the planner's input, pre-rewrite) and the keys // migration dropped. -func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON string, overrides map[string]any, scope items.MigrateScope) (map[string]any, []string, error) { +func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON string, overrides map[string]any, scope items.MigrateScope) (map[string]any, []string, error) { var sourceSchema, targetSchema models.CollectionSchema if err := json.Unmarshal([]byte(sourceSchemaJSON), &sourceSchema); err != nil { return nil, nil, fmt.Errorf("copy item across workspaces: parse source schema: %w", err) @@ -1089,6 +1099,46 @@ func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON stri // note there; these two live in different PACKAGES, which is exactly how // they would drift unnoticed. migrated.Fields = items.CoerceFields(migrated.Fields, items.SchemaForMigratedFields(targetSchema)) + // Relation referents (TASK-2878) — the eighth and last coercion door, and + // the only one that refuses from inside `store`. + // + // A migrate door, so PROVENANCE decides rather than the door: a SUPPLIED + // override is an ordinary write and an unresolvable one is refused; a + // CARRIED value was asserted by nobody, so it is dropped and reported + // through the same `dropped_fields` channel BUG-2674 established. The + // alternative — refusing carried values — would make every legacy item + // uncopyable, and `internal/items` has accepted any string for a relation + // all along, so "legacy" is most of them. + // + // MODE COMES FROM `scope`, the same value MigrateFields was given, rather + // than a second flag derived here. Two independent answers to "is this + // crossing a workspace boundary" is how one request gets migrated one way + // and validated the other; this path also serves a copy whose target IS + // the source workspace, where relations resolve and survive exactly as + // they do on a move. + mode := RelationCarryWithinWorkspace + if scope == items.CrossWorkspace { + mode = RelationCarryCrossWorkspace + } + relRefusals, relDropped, relErr := s.MigrateRelationReferentsQ(q, destWorkspaceID, + items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, mode) + if relErr != nil { + return nil, nil, fmt.Errorf("copy item across workspaces: resolve relation referents: %w", relErr) + } + if len(relRefusals) > 0 { + // The same 400 validation_error the preflight's refuseRelationIssues + // emits, through the channel this function already uses for a failed + // destination validation — so the copy and its preview refuse one + // request with one code and one sentence. + return nil, nil, &FieldValidationError{Err: errors.New(RelationIssuesMessage(relRefusals))} + } + // Appended to Dropped rather than reported separately: StillDropped below + // filters this list against the FINAL map, and MigrateRelationReferentsQ + // has already deleted these keys from it, so they survive that filter and + // reach `warnings.dropped_fields` exactly as a type-mismatch drop does. + for _, ri := range relDropped { + migrated.Dropped = append(migrated.Dropped, ri.Key) + } if err := items.ValidateFields(migrated.Fields, items.SchemaForMigratedFields(targetSchema)); err != nil { return nil, nil, &FieldValidationError{Err: err} } diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index 26ed32fc..1fb534cc 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -73,6 +73,21 @@ func (ri RelationIssue) Message() string { } } +// RelationIssuesMessage renders a set of issues as the ONE sentence every +// refusing door reports. It lives here rather than in `internal/server` +// because the store's own copy door refuses too (through +// `FieldValidationError`), and a second join in a second package is how the +// same refusal comes to be phrased two ways — the drift this whole unit +// exists to remove. `internal/server`'s `relationIssuesMessage` delegates to +// this. +func RelationIssuesMessage(issues []RelationIssue) string { + parts := make([]string, 0, len(issues)) + for _, ri := range issues { + parts = append(parts, ri.Message()) + } + return strings.Join(parts, "; ") +} + // ResolveRelationReferents canonicalises every `relation` value in fieldMap to // the target item's ID and reports the ones that cannot be resolved. //