diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index 9d3d0895..23e01991 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -2354,9 +2354,21 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { // move. Refusing carried values here would make every legacy item — and // `internal/items` has accepted any string for a relation all along — // permanently unmovable. + // The supplied half is a write, so it owes the same visibility check the + // four write doors run. The store resolver cannot do it — see + // refuseInvisibleRelationOverrides. Same workspace here, so the role + // stashed for this request is the right one. + if invisible, err := s.refuseInvisibleRelationOverrides( + r, workspaceID, workspaceRole(r), items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides); err != nil { + writeInternalError(w, err) + return + } else if refuseRelationIssues(w, invisible) { + return + } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, - input.FieldOverrides, store.RelationCarryWithinWorkspace) + input.FieldOverrides, currentFields, store.RelationCarryWithinWorkspace) if relErr != nil { writeInternalError(w, relErr) return diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index f6598ead..67040eba 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -269,7 +269,8 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) { continue } - updated, opErr := s.applyBulkOp(r, workspaceID, item, &req, actor, source, visibleIDs, resolvedTarget, batchID) + var droppedFields []string + updated, opErr := s.applyBulkOp(r, workspaceID, item, &req, actor, source, visibleIDs, resolvedTarget, batchID, &droppedFields) if opErr != nil { resp.Failed = append(resp.Failed, bulkItemFailure{ Ref: itemRefOrSlug(*item), @@ -305,6 +306,12 @@ 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, ", ") + } } s.logActivityWithMeta(workspaceID, item.ID, action, r, auditMeta(meta)) @@ -406,7 +413,14 @@ func bulkStoreError(err error) *bulkOpError { return &bulkOpError{message: err.Error()} } -func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, actor, source string, visibleIDs []string, resolvedTarget *models.Collection, batchID string) (*models.Item, *bulkOpError) { +// droppedFields is an OUT-PARAMETER, and it is one deliberately. Only the +// collection-move branch produces dropped field keys, the caller needs them to +// build ONE activity row per item (a second row would misreport a single +// move as two events), and threading a third return value through fourteen +// unrelated returns would put `nil` in every branch that has nothing to say. +// Reset by the caller each iteration; nil is accepted and means "not +// interested". +func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, actor, source string, visibleIDs []string, resolvedTarget *models.Collection, batchID string, droppedFields *[]string) (*models.Item, *bulkOpError) { switch req.Op { case "archive": if err := s.store.DeleteItem(item.ID, store.WithEventBatch(batchID)); err != nil { @@ -440,7 +454,7 @@ func (s *Server) applyBulkOp(r *http.Request, workspaceID string, item *models.I case "move": if req.Collection != "" { - return s.bulkMoveCollection(r, workspaceID, item, req, visibleIDs, resolvedTarget, batchID) + 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) @@ -661,7 +675,7 @@ func (s *Server) bulkTagUpdate(item *models.Item, tags []string, add bool, actor // resolves to nothing — deliberately, so that case and an existing-but-hidden // target fail identically here rather than at different HTTP statuses (the // existence oracle from codex round 4). Hence the nil check below. -func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, visibleIDs []string, targetColl *models.Collection, batchID string) (*models.Item, *bulkOpError) { +func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *models.Item, req *bulkItemsRequest, visibleIDs []string, targetColl *models.Collection, batchID string, droppedFields *[]string) (*models.Item, *bulkOpError) { if targetColl == nil { return nil, &bulkOpError{message: "target collection not found", code: "invalid_collection"} } @@ -721,7 +735,7 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m // for `supplied` says that rather than leaving it implied. relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( workspaceID, items.SchemaForMigratedFields(targetSchema), result.Fields, - nil, store.RelationCarryWithinWorkspace) + nil, currentFields, store.RelationCarryWithinWorkspace) if relErr != nil { return nil, &bulkOpError{message: relErr.Error(), code: "internal_error"} } @@ -736,6 +750,17 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m if err := items.ValidateFields(result.Fields, items.SchemaForMigratedFields(targetSchema)); err != nil { return nil, &bulkOpError{message: err.Error(), code: "validation_error"} } + // Hand the discarded keys back so the caller's activity row can name them + // (BUG-2674, which fixed only the SINGLE-item move). `result.Dropped` has + // been populated on this path since MigrateFields existed and NOTHING read + // it — a bulk move discarded field values with no record anywhere, which is + // the same silence that convention closed one door over. Filtered against + // the FINAL map for the reason handleMoveItem filters: a key MigrateFields + // listed may have been re-supplied or re-defaulted since, and reporting + // that would be a confident falsehood about data sitting on the item. + if droppedFields != nil { + *droppedFields = items.StillDropped(result.Dropped, result.Fields) + } fieldsJSON, err := json.Marshal(result.Fields) if err != nil { diff --git a/internal/server/handlers_items_copy.go b/internal/server/handlers_items_copy.go index 4384ad2f..df69961e 100644 --- a/internal/server/handlers_items_copy.go +++ b/internal/server/handlers_items_copy.go @@ -2,12 +2,14 @@ package server import ( "database/sql" + "encoding/json" "errors" "fmt" "log/slog" "net/http" "runtime/debug" + "github.com/PerpetualSoftware/pad/internal/items" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" ) @@ -320,6 +322,35 @@ func (s *Server) handleCopyItem(w http.ResponseWriter, r *http.Request) { // has no counterpart, because it writes nothing to attribute. actor, actorSource := actorFromRequest(r) + // Visibility on the SUPPLIED relation overrides, before the store call + // (TASK-2878). The store's migrate function resolves them against the + // database and cannot answer "may this requester see that item" — see + // refuseInvisibleRelationOverrides. Against the DESTINATION, with the + // caller's role THERE: dst.Role is derived fresh from membership and + // grants, and workspaceRole(r) here is the SOURCE's. + // + // Before the store call rather than inside it, and that is deliberate: + // this is a pre-write refusal, so it must not open a transaction and roll + // it back, and the preflight runs the identical check — DR-6's "the + // preview IS the copy" only holds if both doors refuse the same request. + if len(input.FieldOverrides) > 0 { + var targetSchema models.CollectionSchema + if err := json.Unmarshal([]byte(targetColl.Schema), &targetSchema); err != nil { + writeInternalError(w, fmt.Errorf("copy item: parse destination schema: %w", err)) + return + } + invisible, err := s.refuseInvisibleRelationOverrides( + r, dst.WorkspaceID(), dst.Role, items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides) + if err != nil { + writeInternalError(w, err) + return + } + if refuseRelationIssues(w, invisible) { + return + } + } + // ---- The copy. ONE call, no retry (DR-13). --------------------------- res, err := s.copyItemAcrossWorkspaces(store.CrossWorkspaceCopyRequest{ SourceItemID: item.ID, diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index 13a84d3c..07fe7ab5 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -718,9 +718,20 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) if items.ScopeFor(item.WorkspaceID, dst.WorkspaceID()) == items.CrossWorkspace { relMode = store.RelationCarryCrossWorkspace } + // Visibility on the supplied half, against the DESTINATION and the + // caller's role THERE — dst.Role, never workspaceRole(r), which is the + // source's. See refuseInvisibleRelationOverrides. + if invisible, err := s.refuseInvisibleRelationOverrides( + r, dst.WorkspaceID(), dst.Role, items.SchemaForMigratedFields(targetSchema), + input.FieldOverrides); err != nil { + writeInternalError(w, err) + return + } else if refuseRelationIssues(w, invisible) { + return + } relRefusals, relDropped, relErr := s.store.MigrateRelationReferents( dst.WorkspaceID(), items.SchemaForMigratedFields(targetSchema), final, - input.FieldOverrides, relMode) + input.FieldOverrides, currentFields, relMode) if relErr != nil { writeInternalError(w, fmt.Errorf("copy preflight: resolve relation referents: %w", relErr)) return diff --git a/internal/server/handlers_items_copy_relation_test.go b/internal/server/handlers_items_copy_relation_test.go index 7918aaf7..b10d22f7 100644 --- a/internal/server/handlers_items_copy_relation_test.go +++ b/internal/server/handlers_items_copy_relation_test.go @@ -3,6 +3,7 @@ package server import ( "fmt" "net/http" + "net/http/httptest" "strings" "testing" @@ -270,3 +271,184 @@ func containsAll(s string, subs ...string) bool { } return true } + +// newCopyRelationFixtureWith builds the relation fixture with two knobs the +// default constructor does not need: a `default` on the DESTINATION's relation +// field, and the source item's own stored value for that field. +// +// Both exist to reach origins the plain fixture cannot. `MigrateFields` +// injects a destination default for any target field the source has nothing +// 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 bool, sourceOwnerRef *string) *relationFixture { + t.Helper() + srv := testServer(t) + bus := events.New() + srv.SetEventBus(bus) + t.Cleanup(bus.Close) + + owner := mustUser(t, srv, "rel2-owner@example.com", "rel2owner", "") + wsA := mustWorkspace(t, srv, "Rel2 Source WS", owner.ID) + wsB := mustWorkspace(t, srv, "Rel2 Dest WS", owner.ID) + + targetsA := mustSchemaCollection(t, srv, wsA.ID, "People A", `{"fields":[]}`) + targetsB := mustSchemaCollection(t, srv, wsB.ID, "People B", `{"fields":[]}`) + + 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) + } + // Created BEFORE collB, because its id is what the destination default + // names — a default pointing at nothing would fail for the wrong reason. + 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) + } + + relSchema := func(targetSlug, def string) string { + defaultClause := "" + if def != "" { + defaultClause = fmt.Sprintf(`,"default":%q`, def) + } + 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) + } + + collA := mustSchemaCollection(t, srv, wsA.ID, "Rel2 Tasks A", relSchema(targetsA.Slug, "")) + // A REF, not the UUID. This 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 + // bytes, and the test proves nothing. Supplied as `PEOP-1`, only a + // resolved default lands as targetB.ID. + destDef := "" + if destDefault { + destDef = targetB.Ref + } + collB := mustSchemaCollection(t, srv, wsB.ID, "Rel2 Tasks B", relSchema(targetsB.Slug, destDef)) + + sourceFields := `{"status":"open"}` + if sourceOwnerRef != nil { + sourceFields = fmt.Sprintf(`{"status":"open","owner_ref":%q}`, *sourceOwnerRef) + } + source, err := srv.store.CreateItem(wsA.ID, collA.ID, models.ItemCreate{ + Title: "Rel2 Source", Content: "body", Fields: sourceFields, 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, + } +} + +// A DESTINATION DEFAULT is not a carried referent, and must not be dropped as +// one (codex round 1, P1). +// +// `MigrateFields` fills in the destination schema's defaults for keys the +// source item has nothing for. A classifier that splits only on "did the +// caller supply it" then files that value as CARRIED, and a cross-workspace +// copy drops every carried relation without a lookup — so the destination's +// own default was discarded and reported `referent_not_portable`, which is +// flatly false about a value the destination chose. +// +// The source item deliberately has NO owner_ref: with one, the key is carried +// and the default never enters the migrated map, which is the arrangement that +// hid this. +func TestCopyEndpoint_DestinationDefaultRelationIsNotDroppedAsNotPortable(t *testing.T) { + f := newCopyRelationFixtureWith(t, true, nil) + body := f.baseBody() + + pre := f.ok(body) + if reason, dropped := droppedReason(pre, "owner_ref"); dropped { + t.Fatalf("the preflight drops the DESTINATION's own default for reason %q; it never "+ + "pointed at the source workspace", reason) + } + // RESOLVED, not merely present. Misclassifying the default as carried + // deletes it and ValidateFields then re-injects the raw default, so the + // key comes back either way — the value is the only thing that tells the + // two apart. + if v, carried := carriedValue(pre, "owner_ref"); !carried || v != f.targetB.ID { + t.Fatalf("preflight carries owner_ref = %#v (carried=%v), want the destination default "+ + "RESOLVED to %q; the raw ref %q means it was dropped and re-injected unresolved", + v, carried, f.targetB.ID, f.targetB.Ref) + } + + res := assertPreflightMatchesCopy(t, f.copyPreflightFixture, "destination default relation", body) + if got := f.persistedFields(res.Item.ID)["owner_ref"]; got != f.targetB.ID { + t.Fatalf("the copy persisted owner_ref = %#v, want the destination default %q", got, f.targetB.ID) + } + if hasDroppedField(res, "owner_ref") { + t.Fatalf("the copy reported the destination's own default as dropped: %+v", res.Warnings) + } +} + +// An EMPTY relation value is a cleared field, not a referent, and reporting it +// as dropped tells a user they lost something they never had (codex round 1, +// P1, second half). +func TestCopyEndpoint_EmptyCarriedRelationIsNotReportedDropped(t *testing.T) { + empty := "" + f := newCopyRelationFixtureWith(t, false, &empty) + + pre := f.ok(f.baseBody()) + if reason, dropped := droppedReason(pre, "owner_ref"); dropped { + t.Fatalf("the preflight reports an EMPTY relation as dropped (%q) — there was no "+ + "referent to lose: %+v", reason, pre.Fields.Dropped) + } + + res := f.copyOK(f.baseBody()) + if hasDroppedField(res, "owner_ref") { + t.Fatalf("the copy reports an empty relation as dropped: %+v", res.Warnings) + } +} + +// A supplied relation override must name an item the REQUESTER CAN SEE, on the +// copy and its preflight alike (codex round 1, P1). +// +// The four write doors have always checked this; the migrate doors called the +// store resolver directly, and the store cannot answer a request-scoped +// question. The role that matters here is the caller's in the DESTINATION — +// `workspaceRole(r)` is the source's. +// +// The unrestricted owner leg is the control: without it this passes against a +// build that refuses every override. +func TestCopyEndpoint_InvisibleRelationOverrideIsRefused(t *testing.T) { + f := newCopyRelationFixture(t) + body := f.baseBody() + body["field_overrides"] = map[string]any{"owner_ref": f.targetB.Ref} + + // Editor in both, but with no access to People B — so the override names a + // live item in the right collection that this caller cannot see. + blind := f.restrictedEditor("blind-rel@example.com", "blindrel", + []string{f.collA.ID}, []string{f.collB.ID}) + + for _, d := range []struct { + name string + rr *httptest.ResponseRecorder + }{ + {"preflight", f.call(blind, reqOpts{wsRoleCtx: "editor"}, body)}, + {"copy", f.callCopy(blind, reqOpts{wsRoleCtx: "editor"}, body)}, + } { + if d.rr.Code != http.StatusBadRequest { + t.Fatalf("%s: expected 400 for an override naming an item the caller cannot see, "+ + "got %d: %s", d.name, d.rr.Code, d.rr.Body.String()) + } + if code := errCode(t, d.rr); code != "validation_error" { + t.Fatalf("%s: error code = %q, want validation_error", d.name, code) + } + } + + // Control: the same override from the owner, who can see People B. + if rr := f.call(f.owner, reqOpts{}, body); rr.Code != http.StatusOK { + t.Fatalf("the owner's identical override was refused %d — the check is refusing "+ + "visibility-independent of who asks: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/relation_referents.go b/internal/server/relation_referents.go index 9697a3fb..0355a026 100644 --- a/internal/server/relation_referents.go +++ b/internal/server/relation_referents.go @@ -42,6 +42,25 @@ func (s *Server) resolveRelationReferents( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, +) ([]store.RelationIssue, error) { + return s.resolveRelationReferentsAs(r, workspaceID, workspaceRole(r), schema, fieldMap) +} + +// resolveRelationReferentsAs is resolveRelationReferents with the requester's +// effective role passed EXPLICITLY rather than read from the request. +// +// The cross-workspace copy and its preflight need this. `workspaceRole(r)` is +// the role the middleware stashed for the workspace in the URL — the SOURCE — +// and a relation override on a copy names an item in the DESTINATION, where +// the caller's role can be different or absent. `CrossWorkspaceAccess.Role` +// is that role, derived fresh from membership and grants, and its own doc says +// in as many words never to substitute `workspaceRole(r)` for it. +func (s *Server) resolveRelationReferentsAs( + r *http.Request, + workspaceID string, + role string, + schema models.CollectionSchema, + fieldMap map[string]any, ) ([]store.RelationIssue, error) { issues, err := s.store.ResolveRelationReferents(workspaceID, schema, fieldMap) if err != nil { @@ -70,9 +89,19 @@ func (s *Server) resolveRelationReferents( return nil, err } if item == nil { - continue // resolved a moment ago; treat a race as someone else's 404 + // It resolved moments ago and is gone now — soft-deleted between + // the two reads. Treated as unresolvable rather than waved + // through: this whole unit exists to stop a dangling referent + // reaching the blob, and "the target vanished mid-request" is the + // one case where letting it through would be a deliberate one. + // Same `not_found` the resolver would have given a moment later, + // so a retry reports it identically. + issues = append(issues, store.RelationIssue{ + Key: def.Key, Value: id, Target: def.Collection, Reason: store.RelationTargetNotFound, + }) + continue } - visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), workspaceRole(r), isBearerAuth(r)) + visible, err := s.checkItemVisible(workspaceID, item, currentUser(r), role, isBearerAuth(r)) if err != nil { return nil, err } @@ -132,3 +161,46 @@ func refuseRelationIssues(w http.ResponseWriter, issues []store.RelationIssue) b func relationIssuesMessage(issues []store.RelationIssue) string { return store.RelationIssuesMessage(issues) } + +// refuseInvisibleRelationOverrides is the visibility check the MIGRATE doors +// owe their SUPPLIED half. +// +// The four write doors go through resolveRelationReferents, which adds +// checkItemVisible on top of the store resolver. The migrate doors call +// store.MigrateRelationReferents directly — it is a store function and cannot +// answer a request-scoped question — so without this their supplied overrides +// resolved against the database alone. This unit's own rule says a supplied +// value is an ordinary write; an ordinary write cannot name an item the +// requester may not see, and a caller able to edit both collections could +// otherwise point a relation at a hidden one. +// +// Only relation keys are probed, on a COPY of the values: the store call that +// follows does the canonicalising write, and a helper that also mutated would +// leave two functions writing one map. +// +// `role` is explicit for the reason resolveRelationReferentsAs documents — at a +// cross-workspace copy the relevant role is the caller's in the DESTINATION. +func (s *Server) refuseInvisibleRelationOverrides( + r *http.Request, + workspaceID string, + role string, + schema models.CollectionSchema, + supplied map[string]any, +) ([]store.RelationIssue, error) { + if len(supplied) == 0 { + return nil, nil + } + probe := make(map[string]any, len(supplied)) + for _, def := range schema.Fields { + if def.Type != "relation" { + continue + } + if v, ok := supplied[def.Key]; ok && v != nil { + probe[def.Key] = v + } + } + if len(probe) == 0 { + return nil, nil + } + return s.resolveRelationReferentsAs(r, workspaceID, role, schema, probe) +} diff --git a/internal/server/relation_referents_doors_test.go b/internal/server/relation_referents_doors_test.go index 3962b49e..c090e598 100644 --- a/internal/server/relation_referents_doors_test.go +++ b/internal/server/relation_referents_doors_test.go @@ -496,3 +496,134 @@ func TestRelationDoors_CrossWorkspaceCopyAndPreflight(t *testing.T) { assertRefused(t, "preflight", f.call(f.owner, reqOpts{}, body)) }) } + +// A supplied relation override at the MOVE door must name an item the +// requester can see (codex round 1, P1). +// +// The write doors have always checked this; the migrate doors call the store +// resolver directly, and the store cannot answer a request-scoped question. +// Same workspace here, so the role stashed for the request is the right one — +// the cross-workspace half of this rule, where it is NOT, is pinned in +// TestCopyEndpoint_InvisibleRelationOverrideIsRefused. +func TestRelationDoors_MoveRefusesInvisibleOverride(t *testing.T) { + f := newDoorFixture(t) + dst := f.targetCollection() + item := f.seed(`{"status":"open"}`) + + // An editor who can see the two task collections but NOT People. + blind := mustUser(t, f.srv, "blind-move@example.com", "blindmove", "") + 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, dst.ID}); err != nil { + t.Fatalf("SetMemberCollectionAccess: %v", err) + } + + body := map[string]any{ + "target_collection": dst.Slug, + "field_overrides": map[string]any{"owner_ref": f.target.Ref}, + } + + rr := f.callAs(blind, "editor", f.srv.handleMoveItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+item.Slug+"/move", + map[string]string{"itemSlug": item.Slug}, body) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for an override naming an item the caller cannot see, got %d: %s", + rr.Code, rr.Body.String()) + } + if code := errCode(t, rr); code != "validation_error" { + t.Fatalf("error code = %q, want validation_error", code) + } + + // Control: the owner, who can see People, gets the same override through. + // Without this the test passes against a build that refuses every override. + item2 := f.seed(`{"status":"open"}`) + body2 := map[string]any{ + "target_collection": dst.Slug, + "field_overrides": map[string]any{"owner_ref": f.target.Ref}, + } + if rr := f.call(f.srv.handleMoveItem, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/"+item2.Slug+"/move", + map[string]string{"itemSlug": item2.Slug}, body2); rr.Code != http.StatusOK { + t.Fatalf("the owner's identical override was refused %d — the check is not "+ + "visibility-dependent: %s", rr.Code, rr.Body.String()) + } +} + +// A BULK collection move that discards field values must say so (codex round +// 1, P1). +// +// `bulkMoveCollection` has populated `result.Dropped` since MigrateFields +// existed and NOTHING read it — BUG-2674 fixed the single-item door only, so +// the bulk door discarded values with no record anywhere. Wiring relation +// drops into that same dead list is what made it worth fixing rather than +// noting. +// +// Asserted on the ACTIVITY ROW, not the response: the bulk response is a +// per-item outcome list with no room for this, and the timeline is where +// someone asking "what happened to my item" actually looks. +func TestRelationDoors_BulkMoveReportsDroppedFields(t *testing.T) { + f := newDoorFixture(t) + dst := f.targetCollection() + item := f.seed(fmt.Sprintf(`{"status":"open","owner_ref":%q}`, badRef)) + + rr := f.call(f.srv.handleBulkItems, "POST", + "/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil, + map[string]any{"op": "move", "ids": []string{item.ID}, "collection": dst.Slug}) + if rr.Code != http.StatusOK { + t.Fatalf("bulk move: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if v, ok := f.storedRelation(item.ID); ok { + t.Fatalf("an unresolvable relation survived the bulk move as %#v", v) + } + + acts, err := f.srv.store.ListDocumentActivity(item.ID, models.ActivityListParams{Limit: 20}) + if err != nil { + t.Fatalf("ListDocumentActivity: %v", err) + } + var moved *models.Activity + for i := range acts { + if acts[i].Action == "moved" { + moved = &acts[i] + break + } + } + if moved == nil { + t.Fatalf("no `moved` activity row for a bulk collection move: %+v", acts) + } + if !strings.Contains(moved.Metadata, "dropped_fields") || !strings.Contains(moved.Metadata, "owner_ref") { + t.Fatalf("the bulk move discarded owner_ref without naming it in the activity row: %s", + moved.Metadata) + } +} + +// callAs is `call` with an explicit user and workspace role, for the legs that +// need someone other than the fixture owner. +func (f *doorFixture) callAs(user *models.User, role string, h http.HandlerFunc, method, path string, params map[string]string, body any) *httptest.ResponseRecorder { + f.t.Helper() + var reader io.Reader + if body != nil { + raw, err := json.Marshal(body) + if err != nil { + f.t.Fatalf("marshal body: %v", err) + } + reader = bytes.NewReader(raw) + } + r := httptest.NewRequest(method, path, reader) + if body != nil { + r.Header.Set("Content-Type", "application/json") + } + ctx := WithCurrentUser(r.Context(), user) + ctx = contextWithWorkspaceRoleForTest(ctx, role) + ctx = contextWithResolvedWorkspaceIDForTest(ctx, f.ws.ID) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("slug", f.ws.Slug) + for k, v := range params { + rctx.URLParams.Add(k, v) + } + ctx = context.WithValue(ctx, chi.RouteCtxKey, rctx) + rr := httptest.NewRecorder() + h(rr, r.WithContext(ctx)) + return rr +} diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 9322fd94..948f8bcd 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -1121,7 +1121,7 @@ func (s *Store) migrateCopyFields(q Queryer, destWorkspaceID, sourceFieldsJSON, mode = RelationCarryCrossWorkspace } relRefusals, relDropped, relErr := s.MigrateRelationReferentsQ(q, destWorkspaceID, - items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, mode) + items.SchemaForMigratedFields(targetSchema), migrated.Fields, overrides, currentFields, mode) if relErr != nil { return nil, nil, fmt.Errorf("copy item across workspaces: resolve relation referents: %w", relErr) } diff --git a/internal/store/relation_referents.go b/internal/store/relation_referents.go index 7e894758..56b045a9 100644 --- a/internal/store/relation_referents.go +++ b/internal/store/relation_referents.go @@ -348,6 +348,26 @@ const ( RelationCarryCrossWorkspace ) +// RelationOrigin says where a relation value in a migrate door's field map +// came from. THREE origins, not two, and the third is the one that is easy to +// miss: `items.MigrateFields` injects the DESTINATION schema's defaults into +// the map it returns, so a value can be present having been chosen by the +// destination rather than carried from the source. +type RelationOrigin int + +const ( + // RelationOriginSupplied — an explicit override on the move or copy. The + // caller asserted it, so it is a write. + RelationOriginSupplied RelationOrigin = iota + // RelationOriginCarried — present on the SOURCE item. Asserted by nobody. + RelationOriginCarried + // RelationOriginDestinationDefault — absent from the source item and not + // supplied, so `MigrateFields` filled it from the destination schema's + // `default`. It never pointed at the source workspace, which is why it + // must not be dropped as `referent_not_portable`. + RelationOriginDestinationDefault +) + // MigrateRelationReferents is the ONE decision the four migrate doors share // (PLAN-2857 U1 / TASK-2878). Same-workspace move, bulk move, cross-workspace // copy and the copy preflight all call this; none of them reimplements it. @@ -358,28 +378,39 @@ const ( // preflight that says "carried" while the copy drops — or the reverse — is one // request answered two ways, which is the exact defect that comment describes. // -// PROVENANCE, not door, decides the outcome: +// ORIGIN, not door, decides the outcome: // // - SUPPLIED (present in `supplied`, i.e. an explicit `--field` override on // the move or copy): the caller asserted this value, so it is a write and // an unresolvable one is REFUSED. Returned in `refusals`. -// - CARRIED (everything else): not asserted by anyone, so refusing it would -// make a legacy item — and `internal/items` has accepted any string for a -// relation all along, so most of them are legacy — unmovable and -// uncopyable. Dropped and REPORTED instead, through the same -// `dropped_fields` channel BUG-2674 established for a move and the same +// - CARRIED (present in `sourceFields`): not asserted by anyone, so refusing +// it would make a legacy item — and `internal/items` has accepted any +// string for a relation all along, so most of them are legacy — unmovable +// and uncopyable. Dropped and REPORTED instead, through the same +// `dropped_fields` channel BUG-2674 established and the same // `referent_not_portable` bucket the copy already uses for `github_pr`. +// - DESTINATION DEFAULT (neither): `MigrateFields` filled it in from the +// destination schema. Resolved against the destination REGARDLESS of mode, +// because a value the destination chose is not a source-workspace referent +// and dropping it as "not portable" would be a false statement about where +// it came from. A default that does not resolve is a schema problem, so it +// is dropped and reported with the resolver's own reason rather than +// refused — nobody in this request typed it. // -// Mutates fieldMap: dropped keys are deleted, and carried values that survive -// are canonicalised to their target's ID. +// EMPTY VALUES ARE NOT REFERENTS and are skipped at every origin. Reporting a +// blank field as dropped tells a user they lost something they never had. +// +// Mutates fieldMap: dropped keys are deleted, and values that survive are +// canonicalised to their target's ID. func (s *Store) MigrateRelationReferents( workspaceID string, schema models.CollectionSchema, fieldMap map[string]any, supplied map[string]any, + sourceFields map[string]any, mode RelationCarryMode, ) (refusals []RelationIssue, dropped []RelationIssue, err error) { - return s.MigrateRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, supplied, mode) + return s.MigrateRelationReferentsQ(s.Q(), workspaceID, schema, fieldMap, supplied, sourceFields, mode) } // MigrateRelationReferentsQ is MigrateRelationReferents on a caller-supplied @@ -390,13 +421,16 @@ func (s *Store) MigrateRelationReferentsQ( schema models.CollectionSchema, fieldMap map[string]any, supplied map[string]any, + sourceFields map[string]any, mode RelationCarryMode, ) (refusals []RelationIssue, dropped []RelationIssue, err error) { - // Split the map by provenance FIRST, so the two halves cannot be confused - // by anything below. Schema order, for the determinism the preflight - // promises its callers. - carried := map[string]any{} - suppliedRelations := map[string]any{} + // Split the map by ORIGIN first, so nothing below can confuse the three. + // Schema order, for the determinism the preflight promises its callers. + byOrigin := map[RelationOrigin]map[string]any{ + RelationOriginSupplied: {}, + RelationOriginCarried: {}, + RelationOriginDestinationDefault: {}, + } for _, def := range schema.Fields { if def.Type != "relation" { continue @@ -405,15 +439,24 @@ func (s *Store) MigrateRelationReferentsQ( if !exists || raw == nil { continue } - if _, isSupplied := supplied[def.Key]; isSupplied { - suppliedRelations[def.Key] = raw + // An empty value is a cleared relation, not a referent. Skipping it + // here keeps it out of every bucket, so it is neither refused nor + // reported as a drop the user cannot act on. + if str, isStr := raw.(string); isStr && strings.TrimSpace(str) == "" { continue } - carried[def.Key] = raw + switch { + case hasKey(supplied, def.Key): + byOrigin[RelationOriginSupplied][def.Key] = raw + case hasKey(sourceFields, def.Key): + byOrigin[RelationOriginCarried][def.Key] = raw + default: + byOrigin[RelationOriginDestinationDefault][def.Key] = raw + } } // Supplied values are ordinary writes. - if len(suppliedRelations) > 0 { + if suppliedRelations := byOrigin[RelationOriginSupplied]; len(suppliedRelations) > 0 { issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, suppliedRelations) if resolveErr != nil { return nil, nil, resolveErr @@ -425,6 +468,26 @@ func (s *Store) MigrateRelationReferentsQ( } } + // Destination defaults resolve against the DESTINATION in both modes — the + // destination picked the value, so the mode says nothing about it. + if defaults := byOrigin[RelationOriginDestinationDefault]; len(defaults) > 0 { + issues, resolveErr := s.ResolveRelationReferentsQ(q, workspaceID, schema, defaults) + if resolveErr != nil { + return nil, nil, resolveErr + } + for _, ri := range issues { + dropped = append(dropped, ri) + delete(fieldMap, ri.Key) + } + for k, v := range defaults { + if _, survived := fieldMap[k]; !survived { + continue + } + fieldMap[k] = v + } + } + + carried := byOrigin[RelationOriginCarried] switch mode { case RelationCarryCrossWorkspace: // No lookup: a source-workspace id cannot mean anything here. @@ -465,3 +528,10 @@ func (s *Store) MigrateRelationReferentsQ( } return refusals, dropped, nil } + +// hasKey reports whether m declares key. A nil map has no keys, which is how a +// door with no overrides (bulk move) or no source item says so. +func hasKey(m map[string]any, key string) bool { + _, ok := m[key] + return ok +} diff --git a/internal/store/relation_referents_test.go b/internal/store/relation_referents_test.go index 4b37e7db..c4585dd3 100644 --- a/internal/store/relation_referents_test.go +++ b/internal/store/relation_referents_test.go @@ -263,7 +263,7 @@ func TestMigrateRelationReferents_SameWorkspaceKeepsWhatResolves(t *testing.T) { // relation must survive. Dropping it would lose data on every move of a // correctly-related item. fields := map[string]any{"color": red.Ref, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -283,7 +283,7 @@ func TestMigrateRelationReferents_SameWorkspaceDropsWhatDoesNot(t *testing.T) { // accepted any string for a relation all along, so an item carrying "red" // must stay MOVABLE. Dropped and reported, never refused. fields := map[string]any{"color": "red", "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryWithinWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryWithinWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -310,7 +310,7 @@ func TestMigrateRelationReferents_CrossWorkspaceDropsEveryCarriedRelation(t *tes // so there is nothing in the destination it could mean. Same reason // github_pr uses, because it is the same fact about the same kind of value. fields := map[string]any{"color": red.ID, "status": "open"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, nil, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -335,7 +335,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing for _, mode := range []RelationCarryMode{RelationCarryWithinWorkspace, RelationCarryCrossWorkspace} { fields := map[string]any{"color": "nope"} supplied := map[string]any{"color": "nope"} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, mode) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), mode) if err != nil { t.Fatalf("mode %v: %v", mode, err) } @@ -356,7 +356,7 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing // on a copy at all. fields := map[string]any{"color": red.ID} supplied := map[string]any{"color": red.ID} - refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, RelationCarryCrossWorkspace) + refusals, dropped, err := s.MigrateRelationReferents(ws.ID, u1RelationSchema("colors"), fields, supplied, carriedFrom(fields), RelationCarryCrossWorkspace) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -367,3 +367,16 @@ func TestMigrateRelationReferents_SuppliedOverrideRefusesOnEitherMode(t *testing t.Fatalf("supplied override lost: %v", fields["color"]) } } + +// carriedFrom snapshots a field map so it can be passed as the migrate +// function's `sourceFields` — i.e. "every one of these came off the source +// item". A snapshot rather than the map itself because the function DELETES +// dropped keys from fieldMap, and aliasing the two would make the origin +// classification depend on mutation order. +func carriedFrom(fields map[string]any) map[string]any { + out := make(map[string]any, len(fields)) + for k, v := range fields { + out[k] = v + } + return out +}