fix(shop): read the sold count from the purchase rows, not the meta counter (#4942)

* fix(shop): read the sold count from the purchase rows, not the meta counter

A listing had two live answers to "how many sold". `_count.purchases` counts
real purchase rows and is what the sold-out gate, the quantity floor, the
delete guard and the MostPopular sort use. `meta.purchases` is a denormalised
JSONB counter, and it was what every displayed count read.

They disagree on 47 of 1,902 prod listings. One of those renders "20 remaining"
on a sold-out item behind a buy button that throws.

Two selects gain `_count` — the shared `cosmeticShopItemSelect` and
`getPackDetail`'s own, which does not use it. Four sanitizers emit the row
count. Three further paths return `meta` to the client as-is and have no
whitelist to change, so `withSoldCount` writes the row count onto the key they
read; without it the same ShopItem component would show a correct number on a
creator storefront and the drifting one on /shop.

The counter is still written and nothing is backfilled. Fixing the writer is a
separate PR.

The index is required and goes in BEFORE the deploy — not because a reader
would 500 without it, but so the first shop page after the deploy is not the
one that discovers the seq scan. Applied by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(shop): keep the sold-count change a read, and pin the selects that carry it

Review round on #4942. Three real defects in the first pass.

The change was not read-only. `getShopItemById` seeds the moderator item
editor, that form posts the whole `meta` object back, and `purchases` is a
declared key so zod keeps it — so every save wrote the derived row count into
the stored counter, from a client cache that can be older than the value it
replaced. The update now keeps what is stored; only a purchase moves it. The
response still reports the rows, like every other read.

Both `_count` select lines were pinned by nothing. Prisma mocks ignore
`select` and every fixture hand-writes `_count`, so deleting either line left
the whole suite green and threw on six read paths in production. Asserted
against the query the code emits.

The migration and schema comments measured a plan Prisma does not produce. A
relation `_count` is a LEFT JOIN to one whole-table GROUP BY, not a correlated
per-row subquery: 13.7 ms and 1,190 buffers, not 140 ms and 71,400, and the
cost does not scale with page size. Rewritten with the real plans, where the
index actually pays (single-item reads), and what it does not fix.

Also: `getSectionById` and the upsert return were the two paths still serving
the counter; both now go through `withSoldCount`, with controls. The
`StickerShopPanel` comment justified its non-interleaved shelves on a sort-key
mismatch this change removes.

Every guard here was reverted and re-run; each fails naming the wrong value.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(shop): pin the selects and the response the last round left unguarded

Second review round, run mutants rather than argued ones. Three things the
fix round itself introduced.

Two more selects were pinned by nothing, the same shape as the two the last
round closed: deleting `meta: true` from the existing-item select left every
write-back test green while every moderator save would write `purchases: 0`
over the stored counter, and redefining `_count` on `creatorStorefrontItemSelect`
killed the sold count on the creator storefront and the community hub with
nothing red. Both now assert the query the code emitted.

Tightening one test replaced an assertion instead of adding to it, and the
behaviour it covered went in the same change — so the upsert response was
unpinned in both directions. It is deliberately NOT mapped through
`withSoldCount`: its only consumer invalidates and discards the payload, so
mapping it fixed nothing and pinned a value nobody reads. That decision is now
recorded by an assertion rather than left to the next reader.

The migration header claimed the index would let multi-row paths read the index
instead of the heap. It will not: every buffer is a `shared hit`, the table is
fully cached, and `relallvisible` is 550 of 1,190 pages. Also names
`getCommunityCosmetics` as the heaviest consumer — under MostPopular it carries
two whole-table aggregates, confirmed by reading Prisma's emitted SQL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(shop): correct the pin comments, cover the create path, drop a dead aggregate

Round-3 review findings.

Three comments claimed the test assertions were the only thing holding a
`select` line. They are not: Prisma narrows the row to the select, so dropping
a field is `TS2339` at the read. Reworded to what is true — a fast readable
second signal, and the condition under which it would become the gate.

The create branch's `purchases: 0` was covered by nothing: deleting it leaves
603 tests and typecheck green, because `meta` is Json. A new listing has sold
nothing and `purchases` is client-supplied, so the zero is imposed, not trusted.

Removing `withSoldCount` from the upsert response left the `_count` in that
transaction's select dead — a whole-table aggregate on the primary, inside an
open write transaction, that nothing reads. The write path now selects without
it.

The index migration is marked NOT APPROVED: the owner's answer was to replace
the Prisma query with raw SQL first and re-measure, since the doubled aggregate
is a Prisma artifact rather than a database necessity.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Justin Maier
2026-09-18 18:15:19 -06:00
committed by GitHub
parent a658c3f6df
commit 9282fd5deb
15 changed files with 527 additions and 23 deletions
@@ -0,0 +1,85 @@
-- Index the FK every displayed sold count aggregates over.
--
-- `UserCosmeticShopPurchases` has had one index since it was created: the primary
-- key on `buzzTransactionId`. Nothing indexes `shopItemId`, which is what every
-- `_count: { select: { purchases: true } }` groups by.
--
-- Prisma resolves a relation `_count` as a LEFT JOIN to a subquery that aggregates
-- the WHOLE related table once, not a correlated per-row subquery. So the cost is
-- one aggregate per QUERY, independent of page size. Measured on the replica
-- 2026-09-18: 1,190 buffers, ~14 ms, over a 9.3 MB heap that is permanently cached
-- (every buffer in every plan is a `shared hit`, zero reads).
--
-- WHERE THIS INDEX PAYS: the single-item reads. `WHERE si.id = $1` still scans all
-- 40,600 rows to count one item's, because the planner pushes the qual into the
-- subquery and then has nothing to use. `getPackDetail` (publicProcedure,
-- unauthenticated) and `getShopItemById` (protectedProcedure, consumed by the
-- moderator item edit page) both do this.
--
-- WHAT IT DOES NOT FIX: the aggregate is O(table), not O(page), so cost grows with
-- the table regardless of indexing. The durable fix is a `groupBy` restricted to the
-- page's ids -- and that is BLOCKED ON THIS INDEX: measured, a groupBy over 60 ids
-- is still a whole-table seq scan today, and becomes a ~9-cost index-only nested
-- loop with the index. `withSoldCount` in src/server/selectors/cosmetic-shop.selector.ts
-- is the seam. The heaviest per-call consumer is `getCommunityCosmetics`, which under
-- MostPopular carries TWO of these aggregates -- one for the value, one for the
-- `orderBy` -- confirmed by reading Prisma's emitted SQL, and Postgres does not dedupe
-- them. `getShop` runs one aggregate but at higher volume.
--
-- 🔴 NOT APPROVED. DO NOT APPLY THIS YET.
--
-- This file is committed for review and history, not as an instruction. The owner's
-- answer to the request to apply it was: Prisma emits poor SQL here, so replace the
-- query with raw SQL FIRST and re-measure -- the index may not be needed at all. That
-- measurement has not been done as of this commit.
--
-- The doubled aggregate this header describes is a Prisma artifact, not a database
-- necessity, which is what makes that the right order.
--
-- What is NOT in question is the single-item read: counting one item's rows by
-- scanning 40,600 is a missing index rather than a bad query, and no rewrite fixes
-- that. So the likely outcome is that this file survives with a smaller
-- justification. Confirm with a measurement before anyone acts on it.
--
-- IF AND WHEN IT IS APPROVED: apply BEFORE the deploy. Not because a reader would
-- 500 without it -- no column is added, which is why this is an index and not a
-- `purchaseCount` column -- but so the single-item reads are not served at ~4 ms of
-- pure scan each from the moment it lands. A slowdown, not a brownout. Apply to DEV
-- too.
--
-- 🔴 APPLIED BY HAND. Feed this file to psql on STDIN so each statement runs in its
-- own implicit transaction: `CONCURRENTLY` cannot run inside a transaction block and
-- a multi-statement `-c` would wrap it in one. Takes SHARE UPDATE EXCLUSIVE, not
-- ACCESS EXCLUSIVE -- it blocks neither reads nor writes.
--
-- 🔴 `IF NOT EXISTS` WILL SKIP A BROKEN INDEX. A cancelled CONCURRENTLY build leaves
-- an INVALID index behind that is never used and never cleaned up, so a second run of
-- this file reports success over it. Confirm validity, not completion:
--
-- SELECT indexrelid::regclass, indisvalid FROM pg_index
-- WHERE indexrelid = '"UserCosmeticShopPurchases_shopItemId_idx"'::regclass;
-- -- the embedded quotes matter: regclass::text renders a mixed-case identifier
-- -- QUOTED, so comparing against the bare name silently matches nothing.
--
-- If false: DROP INDEX CONCURRENTLY IF EXISTS "UserCosmeticShopPurchases_shopItemId_idx";
-- then run this file again.
--
-- 🔴 THE PLAN CHECK, AND WHICH PLAN TO RUN IT ON. Check the SINGLE-ITEM read
-- (`WHERE si.id = $1`): it must go from a Seq Scan to an index scan of ANY kind on
-- this index. A remaining Seq Scan there means invalid or missing -- that is the
-- abort condition.
--
-- Do NOT judge it on a multi-row plan. Those DO change -- the planner flips the
-- whole-table aggregate to an Index Only Scan (measured hypothetically, 1,592.80 ->
-- 1,427.09) -- but it is ~10% of an estimate on a fully cached table with only 46% of
-- pages all-visible, so expect the wall clock to read about the same before and after.
-- The plan changing there is not a signal in either direction.
--
-- SHAPE: plain single-column, deliberately. `refunded` exists but is true on 40 rows,
-- and the counts carry no `refunded` predicate, so a partial index would not be used.
-- `INCLUDE` buys nothing: the aggregate is COUNT(*), so this is already index-only.
-- The name is Prisma's own, matching `@@index([shopItemId])` in schema.full.prisma.
SET statement_timeout = 0;
CREATE INDEX CONCURRENTLY IF NOT EXISTS "UserCosmeticShopPurchases_shopItemId_idx"
ON "UserCosmeticShopPurchases" ("shopItemId");
@@ -4719,6 +4719,12 @@ model UserCosmeticShopPurchases {
// reconstruct it after the fact. See UserCosmeticShopPurchaseMeta.
meta Json?
components UserCosmeticShopPurchaseCosmetic[]
// Every displayed sold count is a `_count` over this FK, which Prisma resolves
// as one whole-table GROUP BY per query. Without the index a single-item read
// still scans the table to count one item's rows. Applied by hand — the
// migration beside this schema carries the plans and what the index does not fix.
@@index([shopItemId])
}
model UserCosmeticShopPurchaseCosmetic {
+3 -4
View File
@@ -115,10 +115,9 @@ export function StickerShopPanel({
const query = search.trim().toLowerCase();
const tiles = useMemo(() => {
// Official first, then community, each most-sold first. Not interleaved:
// the two halves are separately paged catalogs with no comparable sort key
// — `meta.purchases` against a joined row count — so a merged ordering would
// be a made-up one.
// Official first, then community, each most-sold first. Not interleaved: the
// community half is paged while the official one arrives whole, so a merged
// ordering would rank a complete catalog against whichever page is in hand.
const official = browseShopItems({
entries: (cosmeticShopSections ?? []).flatMap((section) => section.items),
shopItemOf: (entry) => entry.shopItem,
@@ -99,6 +99,7 @@ beforeEach(() => {
meta: { purchases: 0 },
addedById: LISTER,
members: memberRows.map(({ cosmeticId, floorAmount }) => ({ cosmeticId, floorAmount })),
_count: { purchases: 0 },
});
packMemberFindMany.mockResolvedValue(
memberRows.map((row) => ({ ...row, cosmetic: cosmeticFor(row.cosmeticId) }))
@@ -239,3 +240,80 @@ describe('getPackDetail agrees with what the purchase charges', () => {
await expect(charge(BUYER)).rejects.toThrow(/no longer available/i);
});
});
/**
* `meta.purchases` and the `UserCosmeticShopPurchase` rows are two live answers
* to "how many sold", and they disagree on 47 of 1,902 prod listings. The rows
* are the one the sold-out gate, the quantity floor, the delete guard and the
* MostPopular sort have always used; the counter is bumped outside the purchase
* transaction, so a concurrent buy loses an increment and a rolled-back buy
* keeps one.
*
* TO WHOEVER IS ABOUT TO DELETE THIS: the fixture is prod item 74, "Fairy Pony
* (Limited Edition)" — quantity 20, counter 0, twenty purchase rows. Reading the
* counter renders "20 remaining" on a sold-out pack behind a buy button that
* throws. Putting `packMeta.purchases` back is what these assertions exist to
* catch, so if one fails, the read moved back to the counter.
*/
describe('the sold count is the purchase rows, not the meta counter', () => {
const soldOutWithStaleCounter = () =>
shopItemFindUnique.mockResolvedValue({
id: PACK_ID,
cosmeticId: null,
title: 'A pack',
description: null,
unitAmount: PRICE,
status: CosmeticShopItemStatus.Published,
listed: true,
availableQuantity: 20,
// The two disagree, and by more than an off-by-one: a fixture where they
// agree passes under either derivation and tests nothing.
meta: { purchases: 0 },
addedById: LISTER,
members: memberRows.map(({ cosmeticId, floorAmount }) => ({ cosmeticId, floorAmount })),
_count: { purchases: 20 },
});
it('reports the row count when the counter understates it', async () => {
soldOutWithStaleCounter();
const detail = await getPackDetail({ shopItemId: PACK_ID, userId: BUYER });
expect(detail.meta.purchases).toBe(20);
});
/**
* This select is hand-written rather than the shared `cosmeticShopItemSelect`,
* so the `_count` line has to be repeated here — and nothing else can see it
* go missing. Prisma mocks ignore `select` and every fixture hand-writes
* `_count`, so deleting the line leaves the whole suite green and throws on
* every pack page in production.
*
* TO WHOEVER IS ABOUT TO DELETE THIS: it asserts the query the code built, not
* a mock's shape, and it is the only thing holding that line in place.
*/
it('asks the database for the count rather than relying on the fixture', async () => {
soldOutWithStaleCounter();
await getPackDetail({ shopItemId: PACK_ID, userId: BUYER });
expect(shopItemFindUnique.mock.calls[0][0].select._count).toEqual({
select: { purchases: true },
});
});
it('reports the row count when the counter overstates it', async () => {
shopItemFindUnique.mockResolvedValue({
id: PACK_ID,
cosmeticId: null,
title: 'A pack',
description: null,
unitAmount: PRICE,
status: CosmeticShopItemStatus.Published,
listed: true,
availableQuantity: 20,
meta: { purchases: 13 },
addedById: LISTER,
members: memberRows.map(({ cosmeticId, floorAmount }) => ({ cosmeticId, floorAmount })),
_count: { purchases: 4 },
});
const detail = await getPackDetail({ shopItemId: PACK_ID, userId: BUYER });
expect(detail.meta.purchases).toBe(4);
});
});
@@ -132,6 +132,7 @@ const rejectedPack = (status = CosmeticShopItemStatus.Archived) => ({
},
addedById: LISTER,
members: [{ cosmeticId: MEMBER, floorAmount: 2600 }],
_count: { purchases: 0 },
});
// The canonical mock resets once per FILE, not per test, so these three
@@ -152,6 +153,10 @@ beforeEach(() => {
meta: META,
addedById: LISTER,
members: [{ cosmeticId: MEMBER, floorAmount: 2600 }],
// Deliberately unequal to `META.purchases`: the sold count comes from the
// purchase rows, and a fixture where the two agree cannot show which one
// the response carried.
_count: { purchases: 9 },
});
shopItemFindMany.mockResolvedValue([
{
@@ -255,7 +260,8 @@ describe('public pack detail returns named fields, not the meta column', () => {
coverTiles: ['a.png'],
packMemberCount: 1,
acceptsBlueBuzz: true,
purchases: 3,
// The row count, not `META.purchases`, which this fixture sets to 3.
purchases: 9,
});
});
@@ -0,0 +1,66 @@
import { describe, expect, it } from 'vitest';
import { cosmeticShopItemSelect, withSoldCount } from '~/server/selectors/cosmetic-shop.selector';
/**
* The read paths that hand `meta` to the client as-is — /shop's sections, the
* moderator products table, the item editor — have no whitelist to change, so
* the row count has to be written onto `meta.purchases` for them. The same
* `<ShopItem>` renders both those and the creator storefront, which does have a
* whitelist; without this it shows two different sold counts for one item
* depending on which page you reached it from.
*
* TO WHOEVER IS ABOUT TO SIMPLIFY THIS AWAY: the overwrite is the point. A
* spread that keeps the incoming `meta.purchases` passes every other assertion
* in the suite and silently restores the drifting counter on three pages.
*/
describe('withSoldCount writes the row count onto the key clients read', () => {
it('overwrites a counter that understates the rows', () => {
// Prod item 74, "Fairy Pony (Limited Edition)": quantity 20, counter 0,
// twenty purchase rows. The counter renders "20 remaining" on a sold-out
// item behind a buy button that throws.
const out = withSoldCount({ meta: { purchases: 0 }, _count: { purchases: 20 } });
expect(out.meta.purchases).toBe(20);
});
it('overwrites a counter that overstates the rows', () => {
const out = withSoldCount({ meta: { purchases: 737 }, _count: { purchases: 732 } });
expect(out.meta.purchases).toBe(732);
});
it('writes the count onto a null meta rather than dropping the key', () => {
// Non-zero on purpose: with 0 here the case also passes under a reversed
// spread order, which is the mutation that silently restores the counter.
const out = withSoldCount({ meta: null, _count: { purchases: 4 } });
expect(out.meta.purchases).toBe(4);
});
it('keeps the rest of meta, which is what the card and checkout render', () => {
const out = withSoldCount({
id: 74,
meta: { purchases: 0, acceptsBlueBuzz: true, coverUrl: 'cover.png' },
_count: { purchases: 3 },
});
expect(out.meta).toEqual({ purchases: 3, acceptsBlueBuzz: true, coverUrl: 'cover.png' });
// An impl returning only `{ meta }` and dropping `...item` passes every
// assertion above this one.
expect(out.id).toBe(74);
});
});
/**
* The helper and the sanitizers read `item._count.purchases`. Nothing else in
* the suite checks that the QUERY asks for it: Prisma mocks ignore `select` and
* every fixture hand-writes `_count`, so deleting the select line leaves every
* test green and throws on six read paths in production.
*
* TO WHOEVER IS ABOUT TO DELETE THIS: it looks redundant beside the behaviour
* tests and is not. Reverting the select line without this reddens nothing.
*/
describe('the selects actually ask for the purchase count', () => {
it('is on the shared selector, which feeds every storefront and /shop read', () => {
expect(cosmeticShopItemSelect._count).toEqual({ select: { purchases: true } });
});
// getPackDetail's own select is pinned in pack-detail-agreement.test.ts, which
// already mocks the query it emits.
});
@@ -26,4 +26,24 @@ export const cosmeticShopItemSelect = Prisma.validator<Prisma.CosmeticShopItemSe
// A pack has no cosmetic, so cards attribute it to its lister instead.
addedBy: { select: userWithCosmeticsSelect },
meta: true,
// How many actually sold. `meta.purchases` is a denormalised counter that
// drifts — it is bumped outside the purchase transaction, so concurrent buys
// lose increments and a rolled-back buy keeps one. The sold-out gate and the
// MostPopular sort have always counted rows; this is what lets the displayed
// number agree with them.
_count: { select: { purchases: true } },
});
/**
* For the read paths that hand `meta` to the client as-is rather than through a
* whitelist. Those clients read `meta.purchases`, so the row count has to land
* on that key or they keep the drifting counter while the sanitized paths move
* — the same `<ShopItem>` renders both, and it would show two different numbers
* for one item depending on which page you reached it from.
*/
export const withSoldCount = <T extends { meta: unknown; _count: { purchases: number } }>(
item: T
) => ({
...item,
meta: { ...((item.meta ?? {}) as Record<string, unknown>), purchases: item._count.purchases },
});
@@ -37,6 +37,7 @@ const ITEM_ID = 31;
const CREATED_ID = 32;
const OWNER_ID = 7;
const MODERATOR_ID = 9;
const STORED_PURCHASES = 12;
const CLIENT_HTML = '<div data-type="blurb" data-id="7">ATTACKER SUPPLIED</div>';
const EXPANDED_HTML = '<div data-type="blurb" data-id="7">REAL</div>';
@@ -63,10 +64,21 @@ beforeEach(() => {
id: ITEM_ID,
cosmeticId: 4,
addedById: OWNER_ID,
meta: { purchases: STORED_PURCHASES },
_count: { purchases: 0 },
});
dbMock.dbWrite.cosmeticShopItem.update.mockResolvedValue({
id: ITEM_ID,
cosmeticId: null,
meta: { purchases: STORED_PURCHASES },
_count: { purchases: 20 },
});
dbMock.dbWrite.cosmeticShopItem.create.mockResolvedValue({
id: CREATED_ID,
cosmeticId: null,
meta: { purchases: 0 },
_count: { purchases: 0 },
});
dbMock.dbWrite.cosmeticShopItem.update.mockResolvedValue({ id: ITEM_ID, cosmeticId: null });
dbMock.dbWrite.cosmeticShopItem.create.mockResolvedValue({ id: CREATED_ID, cosmeticId: null });
dbMock.dbWrite.cosmeticShopItem.updateMany.mockResolvedValue({ count: 1 });
});
@@ -123,6 +135,22 @@ describe('upsertCosmeticShopItem — blurb expansion', () => {
EXPANDED_HTML
);
});
/**
* The create-side twin of "only a purchase moves the stored counter". A new
* listing has sold nothing, and `purchases` is client-supplied on the upsert
* input, so the zero has to be imposed rather than trusted.
*
* Nothing else sees this one: deleting `purchases: 0` from the create branch
* leaves 603 tests green and typechecks clean, because `meta` is Json.
*/
it('starts a new listing at zero sold, whatever the client posted', async () => {
dbMock.dbWrite.cosmeticShopItem.findUnique.mockResolvedValue(null);
await upsert({ id: undefined, meta: { purchases: 99 } });
expect(dbMock.dbWrite.cosmeticShopItem.create.mock.calls[0][0].data.meta.purchases).toBe(0);
});
});
describe('upsertCosmeticShopItem — blurb reconciliation', () => {
@@ -193,3 +221,62 @@ describe('applyCosmeticShopItemContentChange', () => {
).rejects.toThrow(/No cosmetic shop item/);
});
});
/**
* Reads now serve the purchase-row count in `meta.purchases`, and this form
* seeds itself from a read and posts the whole meta object back. Without this
* the editor writes a derived number into the stored counter on every save — of
* a value that came from a React Query cache, so it can be older than the one it
* replaces.
*
* TO WHOEVER IS ABOUT TO DELETE THIS: it is what keeps the sold-count change a
* READ change. Only a purchase moves the stored counter.
*/
describe('upsertCosmeticShopItem — the stored purchase counter', () => {
it('keeps the stored value when the client posts a different one', async () => {
await upsert({ meta: { purchases: 99, acceptsBlueBuzz: true } });
const { data } = dbMock.dbWrite.cosmeticShopItem.update.mock.calls[0][0];
expect(data.meta.purchases).toBe(STORED_PURCHASES);
});
it('still saves the rest of the meta the moderator edited', async () => {
await upsert({ meta: { purchases: 99, acceptsBlueBuzz: true } });
const { data } = dbMock.dbWrite.cosmeticShopItem.update.mock.calls[0][0];
expect(data.meta.acceptsBlueBuzz).toBe(true);
});
/**
* A fast second signal, NOT the gate. Dropping `meta: true` from the select is
* already a compile error — Prisma narrows the row to the select, so
* `existingItem.meta` stops existing and the read is `TS2339` on a named line.
* This just fails in a second rather than after a typecheck, and says why.
*
* It would stop covering anything if that select's typing were ever loosened —
* a hand-written type, an `as` cast, a widened shared constant — because a
* mock hands back whatever the fixture says regardless of what was asked for.
*/
it('asks the database for the stored meta it preserves', async () => {
await upsert({ meta: { purchases: 99 } });
expect(dbMock.dbWrite.cosmeticShopItem.findUnique.mock.calls[0][0].select.meta).toBe(true);
});
/**
* The save's response is deliberately NOT passed through `withSoldCount`,
* unlike every read path. Its only consumer invalidates the paged query and
* discards the payload, so mapping it fixed nothing and pinned a value nobody
* reads — which would have handed the next person a red test for correctly
* deleting dead code.
*
* Records the decision. It is not the gate: the write path's select has
* `_count` destructured off, so passing that row to `withSoldCount` fails its
* `_count: { purchases: number }` constraint at compile time.
*/
it('hands back what it wrote, not a row-derived count', async () => {
const saved = await upsert({ meta: { purchases: 99 } });
expect(saved.meta.purchases).toBe(STORED_PURCHASES);
});
});
@@ -15,7 +15,7 @@ vi.mock('~/server/prom/client', async (importOriginal) => ({
dbReadFallbackCounter: { inc: vi.fn() },
}));
import { getPaginatedCosmeticShopItems } from '../cosmetic-shop.service';
import { getPaginatedCosmeticShopItems, getShopItemById } from '../cosmetic-shop.service';
import { dbMock } from '~/__tests__/mocks/db.mock';
const capturedWhere = () =>
@@ -65,3 +65,52 @@ describe('getPaginatedCosmeticShopItems archived filter', () => {
expect(where.archivedAt).toBeNull();
});
});
/**
* Both of these return `meta` to the client as-is, so the row count is written
* onto `meta.purchases` by `withSoldCount` rather than by a whitelist. Nothing
* else executes those two call sites: removing either mapping leaves every other
* suite green.
*
* TO WHOEVER IS ABOUT TO DELETE THIS: the helper's own unit tests do not cover
* its wiring, which is the half that was missing the first time round.
*/
describe('the unsanitized read paths serve the row count', () => {
const drifted = {
id: 74,
title: 'Fairy Pony',
meta: { purchases: 0 },
_count: { purchases: 20 },
};
beforeEach(() => {
dbMock.dbRead.cosmeticShopItem.findMany.mockReset();
dbMock.dbRead.cosmeticShopItem.count.mockReset();
dbMock.dbRead.cosmeticShopItem.count.mockResolvedValue(1);
dbMock.dbRead.cosmeticShopItem.findUniqueOrThrow.mockReset();
dbMock.dbWrite.cosmeticShopItem.findUniqueOrThrow.mockReset();
});
it('getPaginatedCosmeticShopItems reports the rows, not the counter', async () => {
dbMock.dbRead.cosmeticShopItem.findMany.mockResolvedValue([drifted]);
const { items } = await getPaginatedCosmeticShopItems({ page: 1, limit: 60 });
expect(items[0].meta.purchases).toBe(20);
});
it('getShopItemById reports the rows, not the counter', async () => {
dbMock.dbRead.cosmeticShopItem.findUniqueOrThrow.mockResolvedValue(drifted);
expect((await getShopItemById({ id: 74 })).meta.purchases).toBe(20);
});
// The mapping sits AFTER the `.catch`, so the replica-fallback result is
// mapped too. Inside the catch it would not be, and nothing else would say so.
it('maps the writer-fallback result as well as the replica one', async () => {
dbMock.dbRead.cosmeticShopItem.findUniqueOrThrow.mockRejectedValue(new Error('replica down'));
dbMock.dbWrite.cosmeticShopItem.findUniqueOrThrow.mockResolvedValue(drifted);
expect((await getShopItemById({ id: 74 })).meta.purchases).toBe(20);
});
});
@@ -27,7 +27,7 @@ vi.mock('~/server/services/user-preferences.service', () => ({
}));
import { PACK_FILTER_VALUE } from '~/server/schema/creator-shop.schema';
import { getShopSectionsWithItems } from '../cosmetic-shop.service';
import { getSectionById, getShopSectionsWithItems } from '../cosmetic-shop.service';
import { loggingMock } from '~/__tests__/mocks/logging.mock';
import { dbMock } from '~/__tests__/mocks/db.mock';
dbMock.dbRead.cosmeticShopSection.findMany.mockImplementation((...args: unknown[]) =>
@@ -46,7 +46,10 @@ const officialItem = {
// Listed by a moderator — addedById is NOT null for official items.
addedById: 999,
cosmetic: { id: 10, createdById: null },
meta: {},
// The counter and the rows disagree on purpose: /shop hands `meta` to the
// client as-is, so the row count has to be written onto it here.
meta: { purchases: 2 },
_count: { purchases: 5 },
},
};
@@ -105,6 +108,14 @@ describe('getShopSectionsWithItems viewer gating', () => {
expect(sections[0].items[0].shopItem.title).toBe('Official badge');
});
// /shop is the one surface with no meta whitelist to change, so the wiring —
// not just the helper — is what has to be pinned. Reverting the `.map` in the
// section return reddens nothing without this.
it('serves the purchase rows as the sold count, not the meta counter', async () => {
const sections = await getShopSectionsWithItems({});
expect(sections[0].items[0].shopItem.meta.purchases).toBe(5);
});
it('non-mod with the creatorShop flag: creator items are not filtered out, status guard stays', async () => {
await getShopSectionsWithItems({ creatorShopEnabled: true, stickersEnabled: true });
@@ -206,3 +217,24 @@ describe('getShopSectionsWithItems viewer gating', () => {
]);
});
});
/**
* The moderator section editor's read. It serves `cosmeticShopItemSelect` with
* `meta` as-is, exactly like /shop, so it needs the same overwrite — and its
* consumer renders no sold count today, which is precisely why nothing else
* would notice it being left out.
*/
describe('getSectionById serves the row count too', () => {
it('reports the rows, not the counter, on the items it returns', async () => {
dbMock.dbRead.cosmeticShopSection.findUniqueOrThrow.mockResolvedValue({
id: 5,
title: 'Badges',
image: null,
items: [{ shopItem: { id: 74, meta: { purchases: 2 }, _count: { purchases: 5 } } }],
});
const section = await getSectionById({ id: 5 });
expect(section.items[0].shopItem.meta.purchases).toBe(5);
});
});
@@ -44,6 +44,9 @@ const itemRow = (id: number, meta: Record<string, unknown> = {}) => ({
addedById: 11,
meta: { purchases: 3, submissionTxId: 'tx-1', sellerShare: 20, imageHash: 'abc', ...meta },
cosmetic: { id: id * 10, name: `Cosmetic ${id}`, type: 'Badge', createdById: 11 },
// Deliberately disagrees with `meta.purchases` above: the rows are the sold
// count, and a fixture where the two agree passes under either derivation.
_count: { purchases: 7 },
});
const baseInput = { limit: 40, page: 1, sort: CosmeticShopSort.Newest };
@@ -65,7 +68,15 @@ describe('getCommunityCosmetics', () => {
mocks.shopItemCount.mockResolvedValue(2);
const { items, totalPages } = await getCommunityCosmetics(baseInput);
expect(items.map((i) => i.id)).toEqual([2, 1]);
expect(items[0].meta).toEqual({ purchases: 3, acceptsBlueBuzz: false });
expect(items[0].meta).toEqual({ purchases: 7, acceptsBlueBuzz: false });
// `creatorStorefrontItemSelect` inherits `_count` by spreading the shared
// selector and never restates it. Redefining `_count` there for another
// relation is already a compile error at all three read sites, so this is a
// readable second signal rather than the gate — it names the relation the
// storefront depends on, which `TS2339` does not.
expect(mocks.shopItemFindMany.mock.calls[0][0].select._count).toEqual({
select: { purchases: true },
});
expect(totalPages).toBe(1);
});
@@ -119,6 +119,7 @@ const shopItemRow = (id: number) => ({
meta: { sellableByOthers: true, sellerShare: 0 },
cosmetic: { id: id * 10, name: `Cosmetic ${id}`, type: 'Badge', data: {} },
addedBy: { id: CREATOR_ID, username: 'creator', image: null },
_count: { purchases: 0 },
});
describe('listing someone elses item records the terms it was listed under', () => {
@@ -485,6 +486,40 @@ describe('getCreatorShop resold section', () => {
expect(resoldWhere().listed).toBe(true);
expect(resoldWhere().meta).toEqual({ path: ['sellableByOthers'], equals: true });
});
/**
* Both storefront sanitizers, against a fixture where the purchase rows and
* `meta.purchases` disagree. Without this, reverting either of them to the
* counter reddens nothing in this file the sold count is not otherwise
* asserted on the storefront path.
*/
it('reports the purchase rows as the sold count, not the meta counter', async () => {
const withCounterDrift = (id: number) => ({
...shopItemRow(id),
meta: { sellableByOthers: true, sellerShare: 0, purchases: 3 },
_count: { purchases: 7 },
});
mocks.resaleFindMany.mockResolvedValue([{ shopItemId: SHOP_ITEM_ID, sellerShare: 20 }]);
mocks.shopItemFindMany
.mockResolvedValueOnce([withCounterDrift(SHOP_ITEM_ID)])
.mockResolvedValueOnce([withCounterDrift(SHOP_ITEM_ID)]);
const { cosmetics, resold } = await getCreatorShop({
userId: RESELLER_ID,
viewerId: RESELLER_ID,
});
expect(cosmetics[0].meta.purchases).toBe(7);
expect(resold[0].meta.purchases).toBe(7);
// Both storefront queries carry `_count` by spreading the shared selector.
// Redefining it there is already a compile error; this is the readable
// version of that failure. Only the first call is checked — both pass the
// same `creatorStorefrontItemSelect` object, so a second assertion would be
// the identical reference.
expect(mocks.shopItemFindMany.mock.calls[0][0].select._count).toEqual({
select: { purchases: true },
});
});
});
describe('withdrawing an item ends its resale listings', () => {
+34 -8
View File
@@ -31,7 +31,7 @@ import type {
} from '~/server/schema/cosmetic-shop.schema';
import { computeCreatorShopSplit, PACK_FILTER_VALUE } from '~/server/schema/creator-shop.schema';
import type { ImageMetaProps } from '~/server/schema/image.schema';
import { cosmeticShopItemSelect } from '~/server/selectors/cosmetic-shop.selector';
import { cosmeticShopItemSelect, withSoldCount } from '~/server/selectors/cosmetic-shop.selector';
import { imageSelect } from '~/server/selectors/image.selector';
import {
createBuzzTransaction,
@@ -88,10 +88,13 @@ export const getShopItemById = async ({ id }: GetByIdInput) => {
},
select: cosmeticShopItemSelect,
} as const;
return dbRead.cosmeticShopItem.findUniqueOrThrow(shopItemFindArgs).catch(() => {
dbReadFallbackCounter.inc({ entity: 'cosmeticShopItem', caller: 'getShopItemById' });
return dbWrite.cosmeticShopItem.findUniqueOrThrow(shopItemFindArgs);
});
return dbRead.cosmeticShopItem
.findUniqueOrThrow(shopItemFindArgs)
.catch(() => {
dbReadFallbackCounter.inc({ entity: 'cosmeticShopItem', caller: 'getShopItemById' });
return dbWrite.cosmeticShopItem.findUniqueOrThrow(shopItemFindArgs);
})
.then(withSoldCount);
};
export const getPaginatedCosmeticShopItems = async (input: GetPaginatedCosmeticShopItemInput) => {
@@ -143,7 +146,11 @@ export const getPaginatedCosmeticShopItems = async (input: GetPaginatedCosmeticS
const count = await dbRead.cosmeticShopItem.count({ where });
return getPagingData({ items, count: (count as number) ?? 0 }, limit, page);
return getPagingData(
{ items: items.map(withSoldCount), count: (count as number) ?? 0 },
limit,
page
);
};
export const upsertCosmetic = async (input: UpsertCosmeticInput) => {
@@ -229,6 +236,7 @@ export const upsertCosmeticShopItem = async ({
id: true,
cosmeticId: true,
addedById: true,
meta: true,
_count: {
select: {
purchases: true,
@@ -288,19 +296,35 @@ export const upsertCosmeticShopItem = async ({
// Spread conditionally: `undefined` means "leave the column alone" to Prisma, and `null`
// clears it — neither should be overwritten with the empty string expansion returns.
...(cosmeticShopItem.description != null && { description: expansion.html }),
// The editor seeds its form from a read, and reads now serve the row count in
// `meta.purchases`, so saving any unrelated field would write that derived
// value into the stored counter. Keep whatever is stored: only a purchase
// moves it. (Create has no stored value and sets its own `meta` below.)
...(id &&
cosmeticShopItem.meta != null && {
meta: {
...cosmeticShopItem.meta,
purchases: (existingItem?.meta as CosmeticShopItemMeta | null)?.purchases ?? 0,
},
}),
availableQuantity,
availableTo,
availableFrom,
archivedAt: archived ? new Date() : null,
};
// Without `_count`: the response is not mapped through `withSoldCount` (see the
// return below), so nothing reads it — and selecting it here would run a
// whole-table aggregate on the PRIMARY inside an open write transaction.
const { _count: _unusedOnWrite, ...writeSelect } = cosmeticShopItemSelect;
const item = await dbWrite.$transaction(
async (tx) => {
const saved = id
? await tx.cosmeticShopItem.update({
where: { id },
data,
select: cosmeticShopItemSelect,
select: writeSelect,
})
: await tx.cosmeticShopItem.create({
data: {
@@ -311,7 +335,7 @@ export const upsertCosmeticShopItem = async ({
purchases: 0,
},
},
select: cosmeticShopItemSelect,
select: writeSelect,
});
if (expansion.evaluated)
@@ -466,6 +490,7 @@ export const getSectionById = async ({ id }: GetByIdInput) => {
return {
...section,
items: section.items.map((i) => ({ ...i, shopItem: withSoldCount(i.shopItem) })),
image: !!section.image
? {
...section.image,
@@ -778,6 +803,7 @@ export const getShopSectionsWithItems = async ({
.filter((s) => s.items.length > 0 || (s.meta as CosmeticShopSectionMeta | null)?.communityHub)
.map((section) => ({
...section,
items: section.items.map((i) => ({ ...i, shopItem: withSoldCount(i.shopItem) })),
image: !!section.image
? {
...section.image,
@@ -491,6 +491,10 @@ export const getPackDetail = async ({
meta: true,
addedById: true,
members: { select: { cosmeticId: true, floorAmount: true }, orderBy: { index: 'asc' } },
// This select is its own, not the shared `cosmeticShopItemSelect`, so the
// row count has to be asked for here too or the pack page is the one
// surface left reading the drifting counter.
_count: { select: { purchases: true } },
},
});
if (!item) throw throwNotFoundError('Pack not found');
@@ -549,7 +553,7 @@ export const getPackDetail = async ({
// Named fields, not the column, and the same whitelist the storefront
// sanitizers spread — a second list here is a list that stops agreeing.
meta: {
purchases: packMeta.purchases ?? 0,
purchases: item._count.purchases,
acceptsBlueBuzz: packMeta.acceptsBlueBuzz ?? false,
...packDisplayMeta(packMeta),
},
+3 -3
View File
@@ -1147,7 +1147,7 @@ export const getCreatorShop = async ({
const sanitize = (item: (typeof items)[number]) => ({
...item,
meta: {
purchases: (item.meta as CosmeticShopItemMeta)?.purchases ?? 0,
purchases: item._count.purchases,
acceptsBlueBuzz: (item.meta as CosmeticShopItemMeta)?.acceptsBlueBuzz ?? false,
...packDisplayMeta(item.meta as CosmeticShopItemMeta | null),
},
@@ -1158,7 +1158,7 @@ export const getCreatorShop = async ({
const sanitizeResold = (item: (typeof resoldItems)[number]) => ({
...item,
meta: {
purchases: (item.meta as CosmeticShopItemMeta)?.purchases ?? 0,
purchases: item._count.purchases,
sellerShare:
resaleShares.get(item.id) ?? (item.meta as CosmeticShopItemMeta)?.sellerShare ?? 0,
acceptsBlueBuzz: (item.meta as CosmeticShopItemMeta)?.acceptsBlueBuzz ?? false,
@@ -1355,7 +1355,7 @@ export const getCommunityCosmetics = async ({
const items = raw.map((item) => ({
...item,
meta: {
purchases: (item.meta as CosmeticShopItemMeta)?.purchases ?? 0,
purchases: item._count.purchases,
acceptsBlueBuzz: (item.meta as CosmeticShopItemMeta)?.acceptsBlueBuzz ?? false,
...packDisplayMeta(item.meta as CosmeticShopItemMeta | null),
},