diff --git a/cmd/cart/pool-server.go b/cmd/cart/pool-server.go index 2cf6223..ec12f4d 100644 --- a/cmd/cart/pool-server.go +++ b/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) diff --git a/cmd/cart/projection_cache_test.go b/cmd/cart/projection_cache_test.go index b5428f4..0da6925 100644 --- a/cmd/cart/projection_cache_test.go +++ b/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) { diff --git a/cmd/cart/projection_overlay.go b/cmd/cart/projection_overlay.go index 09786d6..b723ceb 100644 --- a/cmd/cart/projection_overlay.go +++ b/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 != "" { diff --git a/cmd/cart/projection_overlay_test.go b/cmd/cart/projection_overlay_test.go index a913b19..b8b0d32 100644 --- a/cmd/cart/projection_overlay_test.go +++ b/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) + } +}