diff --git a/pkg/cart/cart-mutation-helper.go b/pkg/cart/cart-mutation-helper.go index a154eb8..2ef467a 100644 --- a/pkg/cart/cart-mutation-helper.go +++ b/pkg/cart/cart-mutation-helper.go @@ -73,6 +73,33 @@ func (c *CartMutationContext) ReleaseItem(ctx context.Context, cartId CartId, sk return c.reservationService.Release(ctx, inventory.SKU(sku), l, inventory.CartID(cartId.String())) } +// equalOptional is the canonical nullable-equality primitive used by the +// cart mutations when deciding whether two identifier fields (parent/child +// links, store ids, anything else nullable) refer to the same thing. +// +// Returns true when both pointers are nil, when both alias the same +// address, or when both are non-nil and dereference to equal values. The +// same-address short-circuit is a no-op for correctness but lets a caller +// who passes the same variable get back true without traversing the +// generic's comparison path. +// +// One tested primitive covers every *T the cart currently compares — +// `*uint32` for ParentId, `*string` for StoreId — and any future +// comparable pointer type. Hand-rolled "both nil OR both non-nil and +// equal" expressions were duplicated at AddItem's merge site and were a +// footgun whenever a new field was added (which axis of the predicate +// was the relevant one?). Keeping a single helper makes future merges +// safer to copy. +func equalOptional[T comparable](a, b *T) bool { + if a == b { // both nil OR same address + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + // CascadeChildQuantities scales every direct child's quantity // proportionally when a parent's quantity is bumped (ChangeQuantity or // the AddItem-merge path). diff --git a/pkg/cart/cart-mutation-helper_test.go b/pkg/cart/cart-mutation-helper_test.go new file mode 100644 index 0000000..2cda664 --- /dev/null +++ b/pkg/cart/cart-mutation-helper_test.go @@ -0,0 +1,68 @@ +package cart + +import "testing" + +// equalOptional is the nullable-equality primitive shared by the AddItem +// merge site and any future field-comparison. Two call sites use it today +// with two distinct types — *uint32 (ParentId) and *string (StoreId) — so +// the test pins both via generic instantiation. +func TestEqualOptional(t *testing.T) { + t.Run("both nil is equal", func(t *testing.T) { + if !equalOptional[string](nil, nil) { + t.Error("both nil should be equal") + } + if !equalOptional[uint32](nil, nil) { + t.Error("both nil should be equal (uint32)") + } + }) + + t.Run("same address alias is equal", func(t *testing.T) { + s := "x" + if !equalOptional(&s, &s) { + t.Error("same-address *string should be equal") + } + v := uint32(5) + if !equalOptional(&v, &v) { + t.Error("same-address *uint32 should be equal") + } + }) + + t.Run("distinct addresses with equal values are equal", func(t *testing.T) { + a, b := "x", "x" + if !equalOptional(&a, &b) { + t.Error("equal strings at distinct addresses should be equal") + } + u, w := uint32(5), uint32(5) + if !equalOptional(&u, &w) { + t.Error("equal uint32 at distinct addresses should be equal") + } + }) + + t.Run("one nil, one set is not equal", func(t *testing.T) { + a := "x" + if equalOptional(&a, nil) { + t.Error("non-nil vs nil should differ (string)") + } + if equalOptional[string](nil, &a) { + t.Error("nil vs non-nil should differ (string)") + } + u := uint32(5) + if equalOptional(&u, nil) { + t.Error("non-nil vs nil should differ (uint32)") + } + if equalOptional[uint32](nil, &u) { + t.Error("nil vs non-nil should differ (uint32)") + } + }) + + t.Run("differing values are not equal", func(t *testing.T) { + a, b := "x", "y" + if equalOptional(&a, &b) { + t.Error("differing strings should differ") + } + u, w := uint32(5), uint32(6) + if equalOptional(&u, &w) { + t.Error("differing uint32 should differ") + } + }) +} diff --git a/pkg/cart/mutation_add_item_test.go b/pkg/cart/mutation_add_item_test.go index 825d821..4552b6d 100644 --- a/pkg/cart/mutation_add_item_test.go +++ b/pkg/cart/mutation_add_item_test.go @@ -68,3 +68,125 @@ func TestAddItem_MergeCascadesToChildren(t *testing.T) { } } } + +// Item identity for merge purposes includes ParentId: a standalone root +// line (ParentId == nil) and a child line (ParentId == &someParentLine) that +// happen to share an ItemId are different roles and MUST stay as distinct +// lines. Merging would silently turn an accessory into a +N on the root +// (or vice versa), corrupting the parent-child link. +func TestAddItem_DoesNotMergeWhenParentIdDiffers(t *testing.T) { + reg := NewCartMultationRegistry(NewCartMutationContext(nil)) + g := NewCartGrain(1, time.Now()) + ctx := context.Background() + + // Standalone root (no ParentId). + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000}); err != nil { + t.Fatalf("add standalone: %v", err) + } + parentLine := g.Items[0].Id + + // Same ItemId+StoreId but with ParentId set — must create a NEW line, + // not bump the standalone's qty. + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000, ParentId: &parentLine}); err != nil { + t.Fatalf("add child-form: %v", err) + } + + if len(g.Items) != 2 { + t.Fatalf("items = %d, want 2 (different ParentId => distinct lines)", len(g.Items)) + } + // Standalone untouched at qty 1; child-form is its own line at qty 1. + for _, it := range g.Items { + if it.Quantity != 1 { + t.Errorf("line %d (parentId=%v) qty = %d, want 1 (no merge)", it.Id, it.ParentId, it.Quantity) + } + // One of them has nil ParentId and the other has &parentLine — guard + // against an accidental merge ever flipping both to non-nil. + } + var seenNil, seenSet bool + for _, it := range g.Items { + if it.ParentId == nil { + seenNil = true + } else if *it.ParentId == parentLine { + seenSet = true + } + } + if !seenNil || !seenSet { + t.Errorf("expected one line with nil ParentId and one with &parentLine; got nil=%v set=%v", seenNil, seenSet) + } +} + +// Positive control: when BOTH sides have ParentId set to the SAME parent +// line, the merge proceeds (this guards the new sameParent guard against +// accidentally rejecting a legitimate "re-add same child of same parent" +// case — e.g. an idempotent retry from a flaky request). +func TestAddItem_MergesWhenBothParentIdMatch(t *testing.T) { + reg := NewCartMultationRegistry(NewCartMutationContext(nil)) + g := NewCartGrain(1, time.Now()) + ctx := context.Background() + + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000}); err != nil { + t.Fatalf("add parent: %v", err) + } + parentLine := g.Items[0].Id + + // First child: ItemId 200 under parent. + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentLine}); err != nil { + t.Fatalf("add child: %v", err) + } + // Re-add the SAME child (same ItemId+StoreId+ParentId) — must merge into + // qty 2, not create a second accessory line. + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentLine}); err != nil { + t.Fatalf("re-add child: %v", err) + } + + if len(g.Items) != 2 { + t.Fatalf("items = %d, want 2 (parent + merged child)", len(g.Items)) + } + for _, it := range g.Items { + switch it.Id { + case parentLine: + if it.Quantity != 1 { + t.Errorf("parent qty = %d, want 1 (untouched)", it.Quantity) + } + default: + if it.Quantity != 2 { + t.Errorf("child qty = %d, want 2 (merged on same-PARENT re-add)", it.Quantity) + } + } + } +} + +// Same ItemId+StoreId+both with ParentId set to *different* parent lines +// must also stay distinct — children belong to their own parent. +func TestAddItem_DoesNotMergeAcrossDifferentParentLines(t *testing.T) { + reg := NewCartMultationRegistry(NewCartMutationContext(nil)) + g := NewCartGrain(1, time.Now()) + ctx := context.Background() + + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P1", Quantity: 1, Price: 500}); err != nil { + t.Fatalf("add parent A: %v", err) + } + parentA := g.Items[0].Id + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "P2", Quantity: 1, Price: 500}); err != nil { + t.Fatalf("add parent B: %v", err) + } + parentB := g.Items[1].Id + + // Two children of different parents sharing an ItemId — distinct lines. + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 999, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentA}); err != nil { + t.Fatalf("add child of A: %v", err) + } + if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 999, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentB}); err != nil { + t.Fatalf("add child of B: %v", err) + } + + // 2 parent lines + 2 child lines = 4 total; no merges across. + if len(g.Items) != 4 { + t.Errorf("items = %d, want 4 (parents A,B + their distinct children)", len(g.Items)) + } + for _, it := range g.Items { + if it.Quantity != 1 { + t.Errorf("line %d qty = %d, want 1 (no accidental merge)", it.Id, it.Quantity) + } + } +} diff --git a/pkg/cart/mutation_items.go b/pkg/cart/mutation_items.go index f32cc77..b7eafc3 100644 --- a/pkg/cart/mutation_items.go +++ b/pkg/cart/mutation_items.go @@ -87,15 +87,25 @@ func (c *CartMutationContext) AddItem(g *CartGrain, m *cart_messages.AddItem) er return fmt.Errorf("AddItem: invalid quantity %d", m.Quantity) } - // Merge with any existing item having the same item id and matching StoreId - // (including both nil). Identity is the id; SKU is reference-only. + // Merge with any existing item having the same item id, matching + // StoreId, and matching ParentId (all three including both nil). + // Identity is ItemId; SKU is reference-only. + // + // ParentId MUST match: a standalone root line (ParentId == nil) and + // a child line (ParentId == &someParentLine) that happen to share an + // ItemId are different roles in the cart, even though the catalog + // item is the same. Merging them would silently convert an accessory + // into a +N on the root (or vice versa), corrupting the parent-child + // link. Both checks are delegated to equalOptional so the nullable- + // equality idiom stays in one place (cart-mutation-helper.go). for _, existing := range g.Items { if existing.ItemId != m.ItemId { continue } - sameStore := (existing.StoreId == nil && m.StoreId == nil) || - (existing.StoreId != nil && m.StoreId != nil && *existing.StoreId == *m.StoreId) - if !sameStore { + if !equalOptional(existing.StoreId, m.StoreId) { + continue + } + if !equalOptional(existing.ParentId, m.ParentId) { continue } if g.Type == cart_messages.CartType_REGULAR && c.UseReservations(existing) {