Skip to content

Cart store: a bare re-add of an existing SKU clears that line's product_id #235

Description

@vedanshujain

Summary

KyselyCartStore's add-line upsert conflicts on (cart_id, sku) and its
doUpdateSet writes product_id from the incoming request unconditionally. A
second add of a SKU already on the cart therefore overwrites that line's
product_id with whatever the new request carried — including null.

So a bare add (one naming no product) of a SKU that is already on the cart
degrades a priced, orderable line into one checkout refuses: the line keeps
its SKU, its quantity and its reservation, and loses the product reference that
every pricing path resolves through.

Why it matters

product_id is the join key both checkout paths use to resolve a line's
snapshot price, its title and its tax class. A line whose product_id is null
is rejected with PRODUCT_NOT_PRICED before anything is priced — so the effect
is a cart that looked complete a moment ago and can no longer check out, with
nothing on screen explaining why.

It is reachable from the storefront. The add endpoint accepts productId as an
optional field, so any caller can send an add for an existing SKU without it.
The endpoint's SKU guard does not help here: it validates the request it is
given, and a bare add is not something it can resolve (see below), so the
damage lands on a different line than the one being validated.

Why the contract suite does not catch it

The cart store contract suite exercises re-adding the same SKU, but every case
supplies the same product_id both times, so the overwrite is a no-op and
invisible. There is no case pinning "a null product_id may not overwrite a
non-null one"
— which is the actual invariant. Adding that case to the shared
contract suite fails on every adapter that has the bug and passes on every one
that does not, which is the right place for it.

Likely fix

Two parts:

  1. In the store, stop letting the conflict update clear the column. The
    narrow form is to preserve the stored value when the incoming one is absent
    (COALESCE(excluded.product_id, cart_lines.product_id) or the dialect
    equivalent), so an add can set a reference and can change it, but cannot
    erase one. Pin it in the contract suite first, per the repo's contract-first
    rule, since the fake and both dialects have to agree.

  2. At the boundary, close the case that makes it reachable. A bare add
    cannot be validated today because ProductCommerceStore has no by-SKU
    lookup — every read on it is keyed by product id — so the add endpoint's
    guard runs only when a productId is supplied. A getBySku / resolveSku
    port method (returning the one live sellable unit that holds a SKU, spanning
    both product rows and variant rows) would let the guard resolve a bare add
    the same way it resolves a named one, at which point a bare add either names
    a real unit or is refused.

Part 1 is worth doing on its own regardless: preserving a reference the caller
did not mention is correct whatever the endpoint above it decides to accept.

Notes

Not a regression — the behaviour predates the add endpoint's SKU guard, and the
guard neither introduces nor widens it. Filed while working nearby so it is
tracked rather than rediscovered.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions