internal product index

cmd/cart/pool-server.go

@@ -80,10 +80,10 @@ func (s *PoolServer) AddSkuToCartHandler(w http.ResponseWriter, r *http.Request,
 		// product-fetcher's "product service returned %d for sku %s" shape so
 		// upstream code that pattern-matches that string still works, AND we
 		// publish StatusNotFound so the HTTP response carries the right code
-		// (the handler's error-return path otherwise renders as 500).
-		w.Header().Set("Content-Type", "application/json")
-		w.WriteHeader(http.StatusNotFound)
-		_, _ = w.Write([]byte(fmt.Sprintf(`{"error":"product service returned %d for sku %s (bus-deleted)","sku":%q}`, 404, sku, sku)))
+		// (the handler's error-return path otherwise renders as 500). http.Error
+		// is the idiomatic stdlib call here — it does the encoding-safe text body
+		// (no fmt.Sprintf/JSON-string-escaping fragility).
+		http.Error(w, fmt.Sprintf("product service returned %d for sku %s (bus-deleted)", 404, sku), http.StatusNotFound)
 		return nil
 	}
 	msg, err := GetItemAddMessage(r.Context(), sku, 1, getCountryFromHost(r.Host), nil)

cmd/cart/projection_cache_test.go

@@ -146,6 +146,45 @@ func TestProjectionCache_BusRaceSafe(t *testing.T) {
 	}
 }
 
+// TestApply_MixedUpsertDelete verifies the realistic per-batch sequence a
+// publisher emits: a single Apply call interleaves upserts and deletes, and
+// the per-batch counters report the right split. Without this the cache could
+// drift on count semantics across batched events.
+func TestApply_MixedUpsertDelete(t *testing.T) {
+	c := newCatalogProjectionCache()
+	// Order matters: prior in-cache state for A is upserted twice (latest wins),
+	// B is tombstoned, C cold-upserted, D cold-tombstoned.
+	updates := []catalog.ProjectionUpdate{
+		{Projection: catalog.Projection{SKU: "A", PriceIncVat: money.Cents(50_00)}},
+		{Projection: catalog.Projection{SKU: "B"}, Deleted: true},
+		{Projection: catalog.Projection{SKU: "C", PriceIncVat: money.Cents(75_00)}},
+		{Projection: catalog.Projection{SKU: "A", PriceIncVat: money.Cents(60_00)}}, // later wins
+		{Projection: catalog.Projection{SKU: "D"}, Deleted: true},
+	}
+	upserts, deletes := c.Apply(updates)
+	if upserts != 3 || deletes != 2 {
+		t.Fatalf("counts: upserts=%d deletes=%d, want 3/2 (A,C live-upserts; B,D tombstones)", upserts, deletes)
+	}
+	if got, ok := c.Get("A"); !ok || got.PriceIncVat != money.Cents(60_00) {
+		t.Fatalf("A latest-wins: got %+v ok=%v, want 6000", got, ok)
+	}
+	if _, ok := c.Get("B"); ok {
+		t.Fatalf("B: tombstoned but Get returned a value")
+	}
+	if !c.IsDeleted("B") {
+		t.Fatalf("B: IsDeleted not true")
+	}
+	if got, ok := c.Get("C"); !ok || got.PriceIncVat != money.Cents(75_00) {
+		t.Fatalf("C cold-upsert: got %+v ok=%v, want 7500", got, ok)
+	}
+	if !c.IsDeleted("D") {
+		t.Fatalf("D: IsDeleted not true")
+	}
+	if c.Len() != 4 {
+		t.Fatalf("Len() after mixed batch: %d, want 4 (A live + B tombstone + C live + D tombstone)", c.Len())
+	}
+}
+
 // TestTaxResolveBp covers the static TaxClass→bp mapping used by
 // ApplyProjectionOverlay. Adds a regression net for the common Nordic rates.
 func TestTaxResolveBp(t *testing.T) {

cmd/cart/projection_overlay.go

@@ -47,6 +47,9 @@ func resolveTaxBp(class string) int {
 //	                  quietly swapping in a default 2500 and miscategorising)
 //	Title          (canonical, post-trim)
 //	Image          (canonical)
+//	ItemID         (positive bus values override HTTP; zero is the
+//	                  documented "no id" signal, so a tombstone's zero or an
+//	                  unbus-fed cache value won't clobber a real HTTP id.)
 //	InventoryTracked (only flips to true; false is the zero value in proto
 //	                  and may be unset on the wire, so we don't second-guess)
 //	DropShip       (only flips to true; same reasoning)
@@ -54,12 +57,14 @@ func resolveTaxBp(class string) int {
 // The HTTP-fetched values remain authoritative for fields the cache does not
 // carry: SellerId / SellerName (marketplace split), Stock (per docs/inventory-
 // shape.md stock is queried synchronously from inventory, not cached),
-// OrgPrice / Discount (not yet on the projection schema), the dynamic
+// OrgPrice / Discount (not yet on the projection schema), and the dynamic
 // ExtraJson product data (configurator options — width/height used by
-// accessory child-pricing), and the catalog uint32 ItemId (the product
-// service's integer id which messages.AddItem.ItemId dedupes on — Projection
-// only carries the opaque string ID, so a cache-only build path is blocked
-// and the HTTP fetch is preserved).
+// accessory child-pricing).
+//
+// Now that ItemID round-trips from the bus, a future "cache-only" build path
+// (skipping PRODUCT_BASE_URL entirely for known SKUs) is unblocked — the
+// only fields left HTTP-only in that future path are Sellers, Stock, OrgPrice
+// and configurator extras, none of which affect line dedup.
 //
 // Tombstoned SKUs (catalog.projection_published with deleted=true) → the
 // caller checks idx.IsDeleted(sku) before this helper and short-circuits.
@@ -78,6 +83,12 @@ func ApplyProjectionOverlay(msg *messages.AddItem, p catalog.Projection) {
 	if p.TaxClass != "" {
 		msg.Tax = int32(resolveTaxBp(p.TaxClass))
 	}
+	// ItemId: positive bus values override HTTP. Zero means the producer
+	// didn't publish an id (some sources have no integer id) — we leave the
+	// HTTP-fetched id intact in that case rather than zeroing out a real id.
+	if p.ItemID != 0 {
+		msg.ItemId = p.ItemID
+	}
 	// Display fields: only override if non-empty so the HTTP-fanned value
 	// remains in place when the cache hasn't populated them yet.
 	if p.Title != "" {

cmd/cart/projection_overlay_test.go

@@ -1,8 +1,11 @@
 package main
 
 import (
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
+	"git.k6n.net/mats/go-cart-actor/pkg/cart"
 	messages "git.k6n.net/mats/go-cart-actor/proto/cart"
 	"git.k6n.net/mats/platform/catalog"
 	"git.k6n.net/mats/platform/money"
@@ -125,6 +128,54 @@ func TestApplyProjectionOverlay_NilMsgSafe(t *testing.T) {
 	ApplyProjectionOverlay(nil, catalog.Projection{SKU: "X"})
 }
 
+// TestAddSkuToCartHandler_TombstoneReturns404 verifies the bus-deleted
+// rejection path: HTTP 404 with the product-fetcher-shaped error string, no
+// HTTP fetch attempted. Locks down the regression where the handler returned
+// an error and the framework rendered it as 500.
+//
+// Why a real mux: `r.PathValue("sku")` only resolves when the request is
+// dispatched through a mux that registered the path pattern (e.g.
+// `GET /cart/add/{sku}` in pool-server.go's Serve()). Calling the handler
+// directly with httptest.NewRequest produces an empty PathValue, which would
+// bypass the tombstone branch and fall through to the HTTP fetch — failing
+// the test for a wiring reason rather than the contract under test.
+func TestAddSkuToCartHandler_TombstoneReturns404(t *testing.T) {
+	idx := newCatalogProjectionCache()
+	idx.Apply([]catalog.ProjectionUpdate{
+		{Projection: catalog.Projection{SKU: "DEL", PriceIncVat: money.Cents(10_00)}},
+		{Projection: catalog.Projection{SKU: "DEL"}, Deleted: true},
+	})
+	// Use a stub PoolServer with nil grain pool: the tombstone path returns
+	// BEFORE ApplyLocal, so the embedded actor.GrainPool is never touched. If a
+	// future edit adds an early s.IsHealthy() / pool-touch on this handler, the
+	// test will panic with a nil-pointer deref and the wiring fault will be
+	// obvious.
+	s := &PoolServer{pod_name: "test", idx: idx}
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("GET /cart/add/{sku}", func(w http.ResponseWriter, r *http.Request) {
+		_ = s.AddSkuToCartHandler(w, r, cart.CartId(1))
+	})
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/cart/add/DEL", nil))
+
+	if rec.Code != http.StatusNotFound {
+		t.Fatalf("status = %d, want %d (bus-deleted SKU). body=%s",
+			rec.Code, http.StatusNotFound, rec.Body.String())
+	}
+	// http.Error-style exact shape: trailing newline, text/plain. Pinning the
+	// string catches regressions either way (body too loose AND format flips).
+	wantCT := "text/plain; charset=utf-8"
+	if got := rec.Header().Get("Content-Type"); got != wantCT {
+		t.Errorf("Content-Type = %q, want %q", got, wantCT)
+	}
+	wantBody := "product service returned 404 for sku DEL (bus-deleted)\n"
+	if got := rec.Body.String(); got != wantBody {
+		t.Errorf("body mismatch:\n  got  %q\n  want %q", got, wantBody)
+	}
+}
+
 // TestIsDeletedVsGet asserts the two methods don't conflict: a tombstoned SKU
 // is reported by both IsDeleted (true) and Get (miss).
 func TestIsDeletedVsGet(t *testing.T) {
@@ -146,3 +197,28 @@ func TestIsDeletedVsGet(t *testing.T) {
 		t.Fatalf("Get on absent entry: ok=true")
 	}
 }
+
+// TestApplyProjectionOverlay_ItemIdPositiveOnly locks the contract that a
+// positive bus ItemID overrides HTTP, but ItemID=0 must NOT clobber a real
+// HTTP id (e.g. a producer that omits an integer id, or a tombstone's zero
+// residual). This is the wire that future-unblocks the cache-only-skip-HTTP
+// path; regression here would silently drop cart-line dedup info.
+func TestApplyProjectionOverlay_ItemIdPositiveOnly(t *testing.T) {
+	base := func() *messages.AddItem {
+		return &messages.AddItem{Sku: "X", ItemId: 12345}
+	}
+
+	// Positive cache id wins.
+	m := base()
+	ApplyProjectionOverlay(m, catalog.Projection{SKU: "X", ItemID: 67890})
+	if m.ItemId != 67890 {
+		t.Errorf("positive cache ItemID: got %d, want 67890", m.ItemId)
+	}
+
+	// Zero cache id must NOT overwrite HTTP.
+	m = base()
+	ApplyProjectionOverlay(m, catalog.Projection{SKU: "X", ItemID: 0})
+	if m.ItemId != 12345 {
+		t.Errorf("zero cache ItemID wiped HTTP id: got %d, want 12345", m.ItemId)
+	}
+}