From 00a8f8b6272c604ce7c2644b6e2a18f284e9ac06 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sun, 30 Aug 2026 18:06:55 +0700 Subject: [PATCH] feat(api): score a guess against the district it was actually in A round now carries two regions. The one the player picked is public and comes back in every response; the one the panorama actually sits in is a secret, and it is what the leaderboard fans out from. Revealing the second before the guess would collapse a country-wide round to a single district, so it joins the exact coordinates on the never-serialized list -- including in the session lookup handler, which echoes fields back to whoever holds the session id, and that is the player. Session consumption is now a claim rather than a courtesy. Reading a session and then deleting it is not a guard: ten concurrent submits all read it alive, all delete it, and all score. DEL is atomic and returns how many keys it removed, so exactly one caller sees a 1 -- the route scores only if it won that. Consuming before the writes also closes the sequential case, where a failure partway through the fan-out would otherwise leave the session alive for half an hour and let a retry re-credit every level that already succeeded. A guess lost to a mid-write failure is the accepted cost. Region parsing lives in one place. Four routes accept a region, and four slightly different ideas about casing and defaulting is how a typo becomes a leaderboard key nobody reads. Unknown and uncovered codes are rejected with a 400 that names the region, rather than served as an empty board that looks exactly like a region nobody has played yet. Sessions created before this change still score, at province level, since they carry no district. They expire within half an hour, so the fallback can go a release from now. The debug coverage route serves any region with an outline rather than only the five provinces. Its directory keeps the city-coverage name for now: the page that calls it cannot be read in this environment, and renaming the route without updating its caller would break it. --- .../project-anti-cheat-invariant.md | 16 +- .../phase-04-api-surface.md | 47 +++++- plans/260830-1345-nested-region-tree/plan.md | 2 +- src/app/api/debug/city-coverage/route.js | 43 ++++-- src/app/api/guess/route.js | 51 ++++-- src/app/api/leaderboard/route.js | 27 +++- src/app/api/new-game/route.js | 67 ++++---- src/lib/region-request.js | 81 ++++++++++ src/lib/session.js | 11 +- src/lib/upstash.js | 11 +- tests/guess-route.test.js | 145 ++++++++++++++++++ tests/mapillary.test.js | 3 + tests/new-game-route.test.js | 144 +++++++++++++++++ tests/region-coverage-route.test.js | 68 ++++++++ tests/region-request.test.js | 88 +++++++++++ tests/session.test.js | 22 ++- 16 files changed, 744 insertions(+), 82 deletions(-) create mode 100644 src/lib/region-request.js create mode 100644 tests/guess-route.test.js create mode 100644 tests/new-game-route.test.js create mode 100644 tests/region-coverage-route.test.js create mode 100644 tests/region-request.test.js diff --git a/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md b/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md index 6dbd538..e0541ce 100644 --- a/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md +++ b/.claude/agent-memory/code-reviewer/project-anti-cheat-invariant.md @@ -1,6 +1,6 @@ --- name: project-anti-cheat-invariant -description: The panorama index is the answer to every round; the client-safety import walk does not cover the API surface, and /api/debug/city-coverage is an open bypass +description: The panorama index is the answer to every round; the client-safety import walk does not cover the API surface, and the debug coverage route is an open, now district-granular bypass metadata: type: project --- @@ -13,9 +13,11 @@ transitively reach them, enforced by the "client safety" import walk in **Why:** a client that can map a panorama id to coordinates scores perfectly every round. **How to apply:** the bundle-graph walk is only half the boundary. Verified 2026-08-30: -`/api/debug/city-coverage` is unauthenticated, has no `NODE_ENV` gate and no middleware, -and returns up to 40,000 exact `{id, lat, lng}` entries filtered by an arbitrary `bbox`. -`/api/new-game` hands the client the panorama id. So the answer is reachable without any -bundle leak. When reviewing changes near regions/pano-index/new-game, check the API -surface too, not just the import graph, and do not treat a passing import-walk test as -proof the property holds. +the debug coverage route is unauthenticated, has no `NODE_ENV` gate and no middleware, +and returns exact `{id, lat, lng}` entries. `/api/new-game` hands the client the +panorama id, so the answer is reachable without any bundle leak. Phase 4 widened that +route from 5 province codes to every code with a boundary (65), so a caller can now pull +a district's list complete and unsampled in one request. The user has accepted this +exposure; do not re-litigate it, but when reviewing changes near regions/pano-index/ +new-game, check whether the change makes extraction cheaper, and never treat a passing +import-walk test as proof the property holds. diff --git a/plans/260830-1345-nested-region-tree/phase-04-api-surface.md b/plans/260830-1345-nested-region-tree/phase-04-api-surface.md index 08445ef..6a24d6f 100644 --- a/plans/260830-1345-nested-region-tree/phase-04-api-surface.md +++ b/plans/260830-1345-nested-region-tree/phase-04-api-surface.md @@ -1,7 +1,7 @@ --- phase: 4 title: "API surface" -status: todo +status: completed priority: P1 effort: "0.75d" dependencies: [2, 3] @@ -9,6 +9,43 @@ dependencies: [2, 3] # Phase 4: API surface +## Outcome (recorded after execution) + +District boards now fill in production: `/api/new-game` resolves the panorama's +district server-side and stores it, and `/api/guess` fans out from that. + +**Two secrets, not one.** `regionCode` joins `exactLocation` on the +never-serialized list, and both `/api/new-game` verbs are tested by serialising +the whole response body and asserting the session's own values appear nowhere +in it. + +**Session consumption became atomic.** The first implementation read the +session then deleted it, which is not a guard: a review probe fired ten +parallel submits of one session id and **all ten scored**. `del` now returns +the DEL count, `deleteGameSession` returns it as a claim, and the route scores +only if it won. Covered by a ten-way concurrent test. + +**The route rename was NOT done.** `city-coverage` -> `region-coverage` and its +caller update are deferred: `src/app/debug/coverage/page.js` cannot be read or +written from this environment (a context hook blocks every path matching +`coverage`), and renaming the route without updating its one caller would break +the debug page. The route accepts `?region=` at any level in place, so nothing +is lost but the name. **Phase 5 cannot edit that page either until the hook is +lifted.** + +Review fixes worth recording: `??` where `||` was needed in `resolveRegion` +(`?region=&city=HN` resolved to an empty region); a `generatedAt` field +restored because its only consumer is unreadable from here; and a stale +`index.panos` reference in the coverage route's bbox branch that would have +500'd every pan on the debug map -- caught by the tests the review asked for. + +Also corrected: an integration-only failure where the test's `fetch` stub +swallowed the Upstash client's own calls. Against the fake, Redis does not use +`fetch`; against a real instance it does. + +`npm test` 260/260, `npm run test:integration` 258 + 2 pre-existing skips, +lint and build clean. + ## Overview Carry the region tree through the route handlers. The session becomes the place @@ -192,8 +229,12 @@ sessions created before the deploy. *Response:* the `?? session.cityCode` fallback, covered by a test so it survives refactors. **Consuming the session first loses a guess on a write failure.** *Signal:* a -player reports a scored round that did not count. *Response:* accepted trade — -one lost guess beats six double-credited keys. The 500 is visible to the player. +player reports a scored round that did not count. *Response:* accepted trade -- +one lost guess beats six double-credited keys. **Correction: the 500 is NOT +visible to the player.** `GameClient.submitGameResult` returns null on failure +and the handler then renders `distance 99999, score 0` as a normal result. The +player sees a confident zero-point round rather than an error. Phase 5 owns +`GameClient` and must distinguish a null result from a real one. **Renaming the coverage route breaks the debug page.** *Signal:* 404 on `/debug/coverage`. *Response:* one caller at `page.js:59`; update in the same diff --git a/plans/260830-1345-nested-region-tree/plan.md b/plans/260830-1345-nested-region-tree/plan.md index 337e317..13ba3ca 100644 --- a/plans/260830-1345-nested-region-tree/plan.md +++ b/plans/260830-1345-nested-region-tree/plan.md @@ -221,7 +221,7 @@ first two. | 1 | [Phase 1: Region tree model and boundaries](./phase-01-region-tree-model-and-boundaries.md) | Completed | — | | 2 | [Phase 2: Panorama district partition](./phase-02-panorama-district-partition.md) | Completed | 1 | | 3 | [Phase 3: Leaderboard fan-out and migration](./phase-03-leaderboard-fan-out-and-migration.md) | Completed | 1 | -| 4 | [Phase 4: API surface](./phase-04-api-surface.md) | Pending | 2, 3 | +| 4 | [Phase 4: API surface](./phase-04-api-surface.md) | Completed | 2, 3 | | 5 | [Phase 5: UI region navigation](./phase-05-ui-region-navigation.md) | Pending | 4 | | 6 | [Phase 6: Docs and verification](./phase-06-docs-and-verification.md) | Pending | 5 | diff --git a/src/app/api/debug/city-coverage/route.js b/src/app/api/debug/city-coverage/route.js index 6032c0b..26daf70 100644 --- a/src/app/api/debug/city-coverage/route.js +++ b/src/app/api/debug/city-coverage/route.js @@ -1,9 +1,10 @@ import { NextResponse } from 'next/server'; import { REGION_BOUNDARIES } from '../../../../data/boundaries/index.js'; -import { PANO_INDEXES } from '../../../../data/panos/index.js'; +import { getRegionPanos, countPanos, getCityIndex } from '../../../../lib/pano-index.js'; +import { getRegion, isRegion, provinceOf } from '../../../../lib/regions.js'; -// Serves a city's outline and its panorama locations for the coverage debug -// page. Points come back for the requested viewport only: Ha Noi holds 225,985 +// Serves a region's outline and its panorama locations for the coverage debug +// page. Works at province or district level. Points come back for the requested viewport only: Ha Noi holds 225,985 // of them, which is 13.8MB of JSON and far more than a map can draw, so the // whole index is never sent at once. const DEFAULT_LIMIT = 12000; @@ -29,21 +30,28 @@ function sampleEvenly(items, limit) { export async function GET(request) { const { searchParams } = new URL(request.url); - const code = (searchParams.get('city') || '').toUpperCase(); + // ?region= at any level; ?city= still accepted for the existing debug page. + const code = (searchParams.get('region') || searchParams.get('city') || '').toUpperCase(); + // Any node with an outline: province or district. The country has no polygon + // of its own, so there is nothing to draw for it. const boundary = REGION_BOUNDARIES[code]; - const index = PANO_INDEXES[code]; - if (!boundary || !index) { + if (!isRegion(code) || !boundary) { return NextResponse.json( { success: false, - error: `Unknown city: ${code || '(none)'}`, - available: Object.keys(PANO_INDEXES), + error: `Unknown or unmapped region: ${code || '(none)'}`, + available: Object.keys(REGION_BOUNDARIES), }, { status: 400 } ); } + const region = getRegion(code); + // A district's panoramas come from its province's index, filtered by the + // district each one was assigned to. + const allPanos = getRegionPanos(code); + const limit = Math.min( MAX_LIMIT, Math.max(1, Number(searchParams.get('limit')) || DEFAULT_LIMIT) @@ -51,11 +59,11 @@ export async function GET(request) { // bbox is west,south,east,north, matching the order the rest of the app uses. const bboxParam = searchParams.get('bbox'); - let inView = index.panos; + let inView = allPanos; if (bboxParam) { const [west, south, east, north] = bboxParam.split(',').map(Number); if ([west, south, east, north].every(Number.isFinite)) { - inView = index.panos.filter( + inView = allPanos.filter( (p) => p.lat >= south && p.lat <= north && p.lng >= west && p.lng <= east ); } @@ -65,20 +73,29 @@ export async function GET(request) { return NextResponse.json({ success: true, - city: { code, name: index.name, center: index.center, bbox: index.bbox }, + city: { code, name: region.name, center: region.center, bbox: region.bbox }, + region: { + code, + name: region.name, + level: region.level, + province: provinceOf(code), + }, // Only the first request for a city carries the outline. Returning it with // every viewport query gave the client a new object each time, which made // the map refit to the city and cancel whatever the user had zoomed into. boundary: bboxParam ? undefined : boundary, + // Restored rather than dropped. Its only consumer is the debug page, whose + // source cannot be read from here (a context hook blocks the path), so + // removing a field it might render would be an unverifiable break. + generatedAt: getCityIndex(provinceOf(code) ?? code).generatedAt, counts: { - total: index.panos.length, + total: countPanos(code), inView: inView.length, shown: panos.length, // True when the viewport holds more points than were sent, so the page // can say the dots are a sample rather than the whole picture. sampled: panos.length < inView.length, }, - generatedAt: index.generatedAt, panos, }); } diff --git a/src/app/api/guess/route.js b/src/app/api/guess/route.js index fef9871..eae47d0 100644 --- a/src/app/api/guess/route.js +++ b/src/app/api/guess/route.js @@ -2,6 +2,7 @@ import { NextResponse } from 'next/server'; import { submitScore, submitDistanceRecord } from '../../../lib/leaderboard.js'; import { getGameSession, deleteGameSession } from '../../../lib/session.js'; import { calculateDistance, calculateScore } from '../../../lib/game.js'; +import { publicRegion } from '../../../lib/region-request.js'; export async function POST(request) { try { @@ -61,11 +62,33 @@ export async function POST(request) { // Calculate score based on distance (server-side) const finalScore = calculateScore(distance); - // Submit to leaderboard with calculated score (both city and global) - const leaderboardResult = await submitScore(username.trim(), finalScore, session.cityCode); - - // Submit distance record to distance leaderboards (both city and global) - const distanceResult = await submitDistanceRecord(username.trim(), distance, session.cityCode); + // The region the panorama was actually in, resolved server-side when the + // round was created. Never read from the request: a client that could name + // its own region could farm any district's board. + // + // The `?? cityCode` fallback carries sessions created before this deploy. + // They have no district, so they credit province and country only. Sessions + // live 30 minutes, so this can be dropped a release from now. + const scoringRegion = session.regionCode ?? session.cityCode; + + // Claim the session before writing, and score only if this request is the + // one that removed it. DEL is atomic, so exactly one of N concurrent + // submits wins; reading the session and deleting it without checking the + // result lets every one of them through, because they all read it alive. + // + // Consuming first also closes the sequential case: a failure partway + // through the fan-out would otherwise leave the session alive for up to 30 + // minutes and a retry would re-credit every level that already succeeded. + const consumed = await deleteGameSession(sessionId); + if (!consumed) { + return NextResponse.json({ + success: false, + error: 'Session already submitted or expired' + }, { status: 400 }); + } + + const leaderboardResult = await submitScore(username.trim(), finalScore, scoringRegion); + const distanceResult = await submitDistanceRecord(username.trim(), distance, scoringRegion); // Log the submission for anti-cheat monitoring console.log('Game submission:', { @@ -79,18 +102,22 @@ export async function POST(request) { timestamp: new Date().toISOString() }); - // Clean up session after successful submission - await deleteGameSession(sessionId); - return NextResponse.json({ success: true, gameResult: { distance, score: finalScore, - globalRank: leaderboardResult.global?.rank || null, - cityRank: leaderboardResult.city?.rank || null, - globalDistanceRank: distanceResult.globalDistance?.rank || null, - cityDistanceRank: distanceResult.cityDistance?.rank || null, + // One entry per level credited, outermost last. The client renders + // these directly rather than a fixed global/city pair. + levels: leaderboardResult.levels, + distanceLevels: distanceResult.levels, + // Where the panorama actually was. Safe now, and only now: the guess + // is in. + region: publicRegion(scoringRegion), + globalRank: leaderboardResult.global?.rank ?? null, + cityRank: leaderboardResult.province?.rank ?? null, + globalDistanceRank: distanceResult.globalDistance?.rank ?? null, + cityDistanceRank: distanceResult.provinceDistance?.rank ?? null, exactLocation: { lat: numTargetLat, lng: numTargetLng diff --git a/src/app/api/leaderboard/route.js b/src/app/api/leaderboard/route.js index 45c7a3a..c52f448 100644 --- a/src/app/api/leaderboard/route.js +++ b/src/app/api/leaderboard/route.js @@ -1,23 +1,40 @@ import { NextResponse } from 'next/server'; import { getLeaderboard } from '../../../lib/leaderboard.js'; +import { getRegion, COUNTRY_CODE } from '../../../lib/regions.js'; +import { resolveRegion } from '../../../lib/region-request.js'; export async function GET(request) { try { const { searchParams } = new URL(request.url); - const cityCode = searchParams.get('city'); // Optional city parameter + + // Any level: country, province or district. Absent means the country, and + // ?city= still works for links made before the tree. + const resolved = resolveRegion(searchParams, false); + if (!resolved.ok) { + // Rejected rather than served as an empty board: an unknown code used to + // come back indistinguishable from a region nobody has played yet, which + // hides typos. + return NextResponse.json( + { success: false, error: resolved.error }, + { status: resolved.status } + ); + } + + const regionCode = resolved.code; const limit = parseInt(searchParams.get('limit')) || 100; const type = searchParams.get('type') || 'score'; // 'score' or 'distance' - // Get leaderboard entries (city-specific or global, score or distance) - const leaderboard = await getLeaderboard(cityCode, limit, type); + const leaderboard = await getLeaderboard(regionCode, limit, type); return NextResponse.json({ success: true, leaderboard, count: leaderboard.length, - type: cityCode ? 'city' : 'global', + region: { code: regionCode, name: getRegion(regionCode).name }, leaderboardType: type, - cityCode: cityCode || null + // Pre-tree field names, kept so existing callers keep working. + type: regionCode === COUNTRY_CODE ? 'global' : 'city', + cityCode: regionCode === COUNTRY_CODE ? null : regionCode, }); } catch (error) { diff --git a/src/app/api/new-game/route.js b/src/app/api/new-game/route.js index 4b7d132..8b1dab5 100644 --- a/src/app/api/new-game/route.js +++ b/src/app/api/new-game/route.js @@ -1,6 +1,7 @@ import { NextResponse } from 'next/server'; import { v4 as uuidv4 } from 'uuid'; -import { cityNames } from '../../../lib/game.js'; +import { getRegion } from '../../../lib/regions.js'; +import { resolvePlayableRegion, publicRegion } from '../../../lib/region-request.js'; import { fetchCityPanorama } from '../../../lib/mapillary.js'; import { storeGameSession, getGameSession } from '../../../lib/session.js'; @@ -11,57 +12,65 @@ function generateSessionId() { export async function GET(request) { const { searchParams } = new URL(request.url); - const cityCode = searchParams.get('city'); const sessionId = searchParams.get('sessionId'); - if (!cityCode) { - return NextResponse.json({ success: false, error: 'Missing city parameter' }); - } - - - const cityName = cityNames[cityCode]; - if (!cityName) { - return NextResponse.json({ - success: false, - error: `Unsupported city code: ${cityCode}` - }, { status: 400 }); + // Accepts ?region= at any level, and ?city= for links made before the tree. + const resolved = resolvePlayableRegion(searchParams); + if (!resolved.ok) { + return NextResponse.json({ success: false, error: resolved.error }, { status: resolved.status }); } + const pickedRegion = resolved.code; + const pickedName = getRegion(pickedRegion).name; try { // The location comes from the prebuilt index, so this is one lookup rather // than a search over an area. - const imageResult = await fetchCityPanorama(cityCode); + const imageResult = await fetchCityPanorama(pickedRegion); if (!imageResult.success) { // The user-facing message is generic; keep the real cause in the logs so - // an API outage is not silently reported as missing city coverage. - console.error(`Mapillary search failed for ${cityName}: ${imageResult.error}`); + // an API outage is not silently reported as missing coverage. + console.error(`Mapillary search failed for ${pickedName}: ${imageResult.error}`); return NextResponse.json({ success: false, - error: `No street view images found in ${cityName}. This city may not have sufficient Mapillary coverage.` + error: `No street view images found in ${pickedName}. This region may not have sufficient Mapillary coverage.` }); } const selectedImage = imageResult.data; + + // The district the panorama actually sits in, resolved server-side from the + // panorama index. Scoring fans out from this, so an absent value is a bug + // rather than something to paper over with the picked region -- that would + // silently leave every district board empty forever. + if (!selectedImage.regionCode) { + throw new Error('Panorama came back without a resolved region'); + } + const exactLocation = { lat: selectedImage.lat, lng: selectedImage.lng }; const imageUrl = selectedImage.url; - // Create or update game session const currentSessionId = sessionId || generateSessionId(); await storeGameSession(currentSessionId, { sessionId: currentSessionId, - cityCode, + // What the player chose. Safe to echo back. + pickedRegion, + // SECRET, alongside exactLocation: this names the district the answer is + // in. Revealing it before the guess turns a country-wide round into a + // 35 km2 one. + regionCode: selectedImage.regionCode, exactLocation, imageId: selectedImage.id, createdAt: Date.now() }); - console.log(`Session ${currentSessionId} created with exact location:`, exactLocation); - console.log(`Using image URL:`, imageUrl); + console.log(`Session ${currentSessionId} created in ${selectedImage.regionCode}`); return NextResponse.json({ success: true, sessionId: currentSessionId, + // Built from the picked region only -- never from the resolved district. + region: publicRegion(pickedRegion), imageData: { id: selectedImage.id, url: imageUrl, @@ -72,13 +81,13 @@ export async function GET(request) { } catch (error) { console.error('Location/Mapillary API Error:', error); - // Provide specific error messages based on error type let errorMessage = 'Failed to fetch street view images. Please try again.'; - if (error.message.includes('City not found')) { - errorMessage = `City "${cityName}" not found in mapping database.`; - } else if (error.message.includes('No polygon data')) { - errorMessage = `No boundary data available for "${cityName}".`; + if (error.message.includes('without a resolved region')) { + errorMessage = 'Panorama index is missing its district assignments. ' + + 'Run scripts/assign-pano-districts.mjs.'; + } else if (error.message.includes('No panorama index')) { + errorMessage = `No panorama data available for "${pickedName}".`; } else if (error.message.includes('Mapillary authentication failed')) { errorMessage = 'Mapillary authentication failed. Please check API token.'; } else if (error.message.includes('fetch')) { @@ -112,9 +121,11 @@ export async function POST(request) { success: true, session: { sessionId: session.sessionId, - cityCode: session.cityCode, + // Only what the player picked. Neither exactLocation nor regionCode is + // exposed: the caller owns this session id, so either one would hand + // them the answer to their own round. + pickedRegion: session.pickedRegion ?? session.cityCode ?? null, createdAt: session.createdAt - // Don't expose exact location for security } }); } catch (error) { diff --git a/src/lib/region-request.js b/src/lib/region-request.js new file mode 100644 index 0000000..bd5bac3 --- /dev/null +++ b/src/lib/region-request.js @@ -0,0 +1,81 @@ +// Region parsing shared by the API routes. +// +// One place, because four routes accept a region and each of them lowercasing +// or defaulting slightly differently is how a typo becomes a leaderboard key +// nobody reads. + +import { getRegion, isRegion, isPlayable, regionPath, COUNTRY_CODE } from './regions.js'; + +/** + * A 400-shaped failure a route can return directly. + * @param {string} message What the caller got wrong. + * @returns {{ok: false, status: number, error: string}} Failure. + */ +function reject(message) { + return { ok: false, status: 400, error: message }; +} + +/** + * Resolve the region a request is asking about. + * + * Accepts `region` and falls back to `city`, which is what every existing link + * and bookmark still sends. + * @param {URLSearchParams} searchParams Query parameters. + * @param {boolean} required False to allow no region at all, meaning the country. + * @returns {{ok: true, code: string}|{ok: false, status: number, error: string}} + */ +export function resolveRegion(searchParams, required) { + // `||` not `??`: URLSearchParams.get returns '' for a present-but-empty + // parameter, so `?region=&city=HN` must fall through to the city rather than + // resolve to an empty region. + const raw = searchParams.get('region') || searchParams.get('city'); + + if (!raw) { + if (required) return reject('Missing region parameter'); + // An absent region has always meant the whole country. + return { ok: true, code: COUNTRY_CODE }; + } + + const code = raw.toUpperCase(); + if (!isRegion(code)) return reject(`Unknown region: ${raw}`); + return { ok: true, code }; +} + +/** + * Resolve a region that has to be playable. + * + * Rejects a region with no coverage rather than letting the draw fail deeper + * in: Cu Chi has no boundary and two Da Nang districts have no imagery, and + * "no panoramas left to try" is a worse answer than "that region has no + * coverage". + * @param {URLSearchParams} searchParams Query parameters. + * @returns {{ok: true, code: string}|{ok: false, status: number, error: string}} + */ +export function resolvePlayableRegion(searchParams) { + const resolved = resolveRegion(searchParams, true); + if (!resolved.ok) return resolved; + + if (!isPlayable(resolved.code)) { + return reject(`${getRegion(resolved.code).name} has no street view coverage yet`); + } + return resolved; +} + +/** + * The public description of a region, safe to send before a guess. + * + * Built only from what the player picked. The district the panorama actually + * sits in is a secret until the guess is in -- naming it would collapse a + * country-wide round to one district. + * @param {string} code Region code the player chose. + * @returns {{code: string, name: string, path: string[], level: string}} Description. + */ +export function publicRegion(code) { + const region = getRegion(code); + return { + code, + name: region.name, + path: regionPath(code), + level: region.level, + }; +} diff --git a/src/lib/session.js b/src/lib/session.js index 546c7a7..aaa0d23 100644 --- a/src/lib/session.js +++ b/src/lib/session.js @@ -39,16 +39,19 @@ export async function getGameSession(sessionId) { } /** - * Delete a game session from Upstash. + * Delete a game session, reporting whether this caller was the one to remove it. + * + * The return value is a claim, not a status. Two requests submitting the same + * session concurrently both read a live session, so scoring has to be gated on + * who actually deleted it -- DEL is atomic and only one caller sees a 1. * @param {string} sessionId Session identifier. - * @returns {Promise} Success status. + * @returns {Promise} True when this call removed the session. */ export async function deleteGameSession(sessionId) { try { const h = getUpstash(); const key = SESSION_KEY_PREFIX + sessionId; - await del(h, key); - return true; + return (await del(h, key)) > 0; } catch (error) { console.error('Error deleting game session:', error); throw error; diff --git a/src/lib/upstash.js b/src/lib/upstash.js index ab0d0a5..0fa073f 100644 --- a/src/lib/upstash.js +++ b/src/lib/upstash.js @@ -78,13 +78,18 @@ export async function putJson(h, key, value, ttlSeconds) { } /** - * Delete a key. + * Delete a key, reporting whether it was actually there. + * + * The count matters: DEL is atomic, so exactly one of N racing callers gets 1 + * back and the rest get 0. That makes it the cheapest available + * compare-and-swap for "claim this key", which is how a game session is + * consumed exactly once. * @param {{ client: Redis, prefix: string }} h * @param {string} key - * @returns {Promise} + * @returns {Promise} Keys removed: 1 if it existed, 0 if not. */ export async function del(h, key) { - await h.client.del(pkey(h, key)); + return Number(await h.client.del(pkey(h, key))) || 0; } /** diff --git a/tests/guess-route.test.js b/tests/guess-route.test.js new file mode 100644 index 0000000..911fcf0 --- /dev/null +++ b/tests/guess-route.test.js @@ -0,0 +1,145 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +vi.mock('@upstash/redis', async (importOriginal) => { + const { upstashModule } = await import('./mock-upstash.js'); + return upstashModule(importOriginal); +}); + +import { POST } from '../src/app/api/guess/route.js'; +import { storeGameSession } from '../src/lib/session.js'; +import { getLeaderboard } from '../src/lib/leaderboard.js'; +import { resetStore, storedKeys } from './redis-harness.js'; + +// Scoring reads the region from the session and nowhere else. This is the +// property the whole design rests on: a client that could name its own region +// could farm any district's board from a single round. + +const HCMC = { lat: 10.7712, lng: 106.7003 }; + +/** Put a playable session in the store. */ +async function seedSession(sessionId, overrides) { + await storeGameSession(sessionId, { + sessionId, + pickedRegion: 'TPHCM', + regionCode: 'TPHCM-Q7', + exactLocation: HCMC, + imageId: '123', + createdAt: Date.now(), + ...overrides, + }); +} + +/** Submit a guess. */ +function guess(body) { + return POST( + new Request('http://localhost/api/guess', { + method: 'POST', + body: JSON.stringify(body), + }) + ); +} + +describe('POST /api/guess', () => { + beforeEach(async () => { + await resetStore(); + }); + + it('credits the district on the session, its province and the country', async () => { + await seedSession('s1'); + const body = await ( + await guess({ username: 'mai', sessionId: 's1', guessLat: HCMC.lat, guessLng: HCMC.lng }) + ).json(); + + expect(body.success).toBe(true); + expect(body.gameResult.levels.map((l) => l.code)).toEqual(['TPHCM-Q7', 'TPHCM', 'VN']); + + const keys = await storedKeys(); + expect(keys).toContain('vngeoguessr:leaderboard:city:tphcm-q7'); + expect(keys).toContain('vngeoguessr:leaderboard:city:tphcm'); + expect(keys).toContain('vngeoguessr:leaderboard:vietnam'); + }); + + it('ignores a region supplied in the request body', async () => { + // The anti-cheat property. A client naming DL must not move DL's board. + await seedSession('s2'); + await guess({ + username: 'mai', + sessionId: 's2', + guessLat: HCMC.lat, + guessLng: HCMC.lng, + regionCode: 'DL', + cityCode: 'DL', + }); + + expect(await storedKeys()).not.toContain('vngeoguessr:leaderboard:city:dl'); + expect((await getLeaderboard('TPHCM-Q7')).length).toBe(1); + }); + + it('reveals where the panorama was, but only in the result', async () => { + await seedSession('s3'); + const body = await ( + await guess({ username: 'mai', sessionId: 's3', guessLat: HCMC.lat, guessLng: HCMC.lng }) + ).json(); + expect(body.gameResult.region.path).toEqual(['Vietnam', 'Ho Chi Minh', 'District 7']); + }); + + it('cannot be replayed for double credit', async () => { + // The session is consumed before the writes, so a retry finds nothing. A + // live session would let a mid-fan-out failure be re-submitted and credit + // every level that already succeeded a second time. + await seedSession('s4'); + const first = await guess({ + username: 'mai', sessionId: 's4', guessLat: HCMC.lat, guessLng: HCMC.lng, + }); + expect((await first.json()).success).toBe(true); + + const replay = await guess({ + username: 'mai', sessionId: 's4', guessLat: HCMC.lat, guessLng: HCMC.lng, + }); + expect((await replay.json()).success).toBe(false); + + // The score landed exactly once. + expect((await getLeaderboard('TPHCM-Q7'))[0].score).toBe(5); + }); + + it('scores a session created before the tree at province level', async () => { + // Sessions live 30 minutes, so a deploy strands some in the old shape. + // They have no district and must still score rather than throw. + await seedSession('s5', { regionCode: undefined, cityCode: 'TPHCM' }); + const body = await ( + await guess({ username: 'mai', sessionId: 's5', guessLat: HCMC.lat, guessLng: HCMC.lng }) + ).json(); + + expect(body.success).toBe(true); + expect(body.gameResult.levels.map((l) => l.code)).toEqual(['TPHCM', 'VN']); + }); + + it('lets exactly one of many concurrent submits score', async () => { + // Read-then-delete is not a guard: ten requests all read a live session, + // all delete it, and all write. DEL is atomic, so gating on its count is + // what actually makes consumption exclusive. + await seedSession('s6'); + const submissions = Array.from({ length: 10 }, () => + guess({ username: 'mai', sessionId: 's6', guessLat: HCMC.lat, guessLng: HCMC.lng }) + ); + const bodies = await Promise.all( + (await Promise.all(submissions)).map((response) => response.json()) + ); + + expect(bodies.filter((body) => body.success)).toHaveLength(1); + + // And the score landed once, not ten times. + expect((await getLeaderboard('TPHCM-Q7'))[0].score).toBe(5); + expect((await getLeaderboard('VN'))[0].score).toBe(5); + // One distance record, not ten: the entry id embeds a timestamp, so + // duplicates would each take their own slot on a 200-entry board. + expect(await getLeaderboard('TPHCM-Q7', 100, 'distance')).toHaveLength(1); + }); + + it('rejects an expired or unknown session', async () => { + const body = await ( + await guess({ username: 'mai', sessionId: 'gone', guessLat: 10, guessLng: 106 }) + ).json(); + expect(body.success).toBe(false); + expect(body.error).toMatch(/Session not found/); + }); +}); diff --git a/tests/mapillary.test.js b/tests/mapillary.test.js index f22403a..3d3c3ef 100644 --- a/tests/mapillary.test.js +++ b/tests/mapillary.test.js @@ -3,6 +3,9 @@ import { fetchCityPanorama } from '../src/lib/mapillary.js'; import { getRegionPanos } from '../src/lib/pano-index.js'; import { provinceOf } from '../src/lib/regions.js'; +// These stub fetch wholesale, which is safe only because nothing here touches +// Redis -- against a real instance the Upstash client speaks over fetch too. +// // The district a guess is credited to is resolved here and nowhere else, so // these tests pin the two behaviours that carry it: the leaf must come from the // attempt that actually succeeded, and an exhausted pool must not become a 500. diff --git a/tests/new-game-route.test.js b/tests/new-game-route.test.js new file mode 100644 index 0000000..e2cee6f --- /dev/null +++ b/tests/new-game-route.test.js @@ -0,0 +1,144 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +vi.mock('@upstash/redis', async (importOriginal) => { + const { upstashModule } = await import('./mock-upstash.js'); + return upstashModule(importOriginal); +}); + +import { GET, POST } from '../src/app/api/new-game/route.js'; +import { getGameSession } from '../src/lib/session.js'; +import { getRegion, provinceOf } from '../src/lib/regions.js'; +import { resetStore } from './redis-harness.js'; + +// The district a panorama sits in is the answer to the round. It is resolved +// server-side and stored on the session, and no response before the guess may +// contain it -- naming it collapses a country-wide round to one district. + +const ORIGINAL_TOKEN = process.env.MAPILLARY_ACCESS_TOKEN; + +const request = (query) => new Request(`http://localhost/api/new-game?${query}`); + +beforeEach(async () => { + await resetStore(); + process.env.MAPILLARY_ACCESS_TOKEN = 'test-token'; + // Any Mapillary image id resolves; the point under test is which region gets + // stored. Everything else passes through -- against a real Redis the Upstash + // client speaks over fetch too, and swallowing its calls would fail every + // session write rather than exercising the route. + const realFetch = globalThis.fetch; + vi.stubGlobal('fetch', async (url, init) => { + if (!String(url).includes('graph.mapillary.com')) return realFetch(url, init); + const id = String(url).split('/').pop().split('?')[0]; + return new Response( + JSON.stringify({ + id, + thumb_2048_url: `https://example.invalid/${id}.jpg`, + is_pano: true, + geometry: { coordinates: [106.7, 10.77] }, + }), + { status: 200 } + ); + }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + if (ORIGINAL_TOKEN === undefined) delete process.env.MAPILLARY_ACCESS_TOKEN; + else process.env.MAPILLARY_ACCESS_TOKEN = ORIGINAL_TOKEN; +}); + +describe('GET /api/new-game', () => { + it('stores the resolved district, not the region the player picked', async () => { + const body = await (await GET(request('region=TPHCM'))).json(); + expect(body.success).toBe(true); + + const session = await getGameSession(body.sessionId); + expect(getRegion(session.regionCode).level).toBe('district'); + expect(provinceOf(session.regionCode)).toBe('TPHCM'); + expect(session.pickedRegion).toBe('TPHCM'); + }); + + it('resolves a country round to a district', async () => { + const body = await (await GET(request('region=VN'))).json(); + const session = await getGameSession(body.sessionId); + expect(session.regionCode).not.toBe('VN'); + expect(getRegion(session.regionCode).level).toBe('district'); + }); + + it('never reveals the resolved district before the guess', async () => { + // The whole anti-cheat property in one assertion. + const response = await GET(request('region=VN')); + const body = await response.json(); + const session = await getGameSession(body.sessionId); + + const serialised = JSON.stringify(body); + expect(serialised).not.toContain(session.regionCode); + expect(serialised).not.toContain(String(session.exactLocation.lat)); + expect(serialised).not.toContain(String(session.exactLocation.lng)); + }); + + it('describes a country round as Vietnam and nothing narrower', async () => { + const body = await (await GET(request('region=VN'))).json(); + expect(body.region.path).toEqual(['Vietnam']); + }); + + it('accepts a district directly and keeps it', async () => { + const body = await (await GET(request('region=DL'))).json(); + const session = await getGameSession(body.sessionId); + expect(session.regionCode).toBe('DL'); + }); + + it('still accepts ?city= from links made before the tree', async () => { + const body = await (await GET(request('city=HN'))).json(); + expect(body.success).toBe(true); + expect((await getGameSession(body.sessionId)).pickedRegion).toBe('HN'); + }); + + it('rejects an unknown region with 400, not 500', async () => { + const response = await GET(request('region=NOPE')); + expect(response.status).toBe(400); + expect((await response.json()).error).toMatch(/Unknown region/); + }); + + it('rejects a region with no coverage, naming it', async () => { + const response = await GET(request('region=TPHCM-CUCHI')); + expect(response.status).toBe(400); + expect((await response.json()).error).toMatch(/Cu Chi/); + }); + + it('rejects a missing region', async () => { + expect((await GET(request(''))).status).toBe(400); + }); +}); + +describe('POST /api/new-game', () => { + it('exposes the picked region but never the answer', async () => { + // This handler echoes session fields back to whoever owns the session id -- + // which is the player. Returning the resolved district here would hand them + // their own answer. + const created = await (await GET(request('region=VN'))).json(); + const session = await getGameSession(created.sessionId); + + const response = await POST( + new Request('http://localhost/api/new-game', { + method: 'POST', + body: JSON.stringify({ sessionId: created.sessionId }), + }) + ); + const body = await response.json(); + + expect(body.session.pickedRegion).toBe('VN'); + const serialised = JSON.stringify(body); + expect(serialised).not.toContain(session.regionCode); + expect(serialised).not.toContain('exactLocation'); + }); + + it('404s an unknown session', async () => { + const response = await POST( + new Request('http://localhost/api/new-game', { + method: 'POST', + body: JSON.stringify({ sessionId: 'nope' }), + }) + ); + expect(response.status).toBe(404); + }); +}); diff --git a/tests/region-coverage-route.test.js b/tests/region-coverage-route.test.js new file mode 100644 index 0000000..a7949e2 --- /dev/null +++ b/tests/region-coverage-route.test.js @@ -0,0 +1,68 @@ +import { describe, it, expect } from 'vitest'; +import { GET } from '../src/app/api/debug/city-coverage/route.js'; +import { countPanos } from '../src/lib/pano-index.js'; +import { provinceOf } from '../src/lib/regions.js'; + +// The coverage route now serves any region with an outline, not just the five +// province indexes. It reaches getRegionPanos, which throws for the country -- +// guarded today only by the country having no boundary, which is a coupling +// between two generated files worth pinning. + +const request = (query) => new Request(`http://localhost/api/debug/coverage?${query}`); + +describe('GET debug coverage', () => { + it('serves a province', async () => { + const body = await (await GET(request('region=DN'))).json(); + expect(body.success).toBe(true); + expect(body.counts.total).toBe(countPanos('DN')); + expect(body.region.level).toBe('province'); + }); + + it('serves a district, scoped to that district', async () => { + const body = await (await GET(request('region=DN-HAICHAU'))).json(); + expect(body.success).toBe(true); + expect(body.counts.total).toBe(countPanos('DN-HAICHAU')); + expect(body.counts.total).toBeLessThan(countPanos('DN')); + expect(body.region.province).toBe('DN'); + }); + + it('rejects the country, which has no outline to draw', async () => { + // getRegionPanos throws for a country-level code. This must surface as a + // 400 from the boundary check, not an unhandled throw. + const response = await GET(request('region=VN')); + expect(response.status).toBe(400); + expect((await response.json()).error).toMatch(/Unknown or unmapped region/); + }); + + it('rejects a district with no boundary', async () => { + // Cu Chi is in the tree but has no OSM relation left. + expect((await GET(request('region=TPHCM-CUCHI'))).status).toBe(400); + }); + + it('rejects an unknown code', async () => { + expect((await GET(request('region=NOPE'))).status).toBe(400); + }); + + it('still accepts ?city= from the existing debug page', async () => { + expect((await (await GET(request('city=DN'))).json()).success).toBe(true); + }); + + it('reports the generation stamp its consumer may render', async () => { + // The debug page cannot be read from this session, so the field it might + // depend on is asserted here instead. + const body = await (await GET(request('region=DN-HAICHAU'))).json(); + expect(body.generatedAt).toBeTruthy(); + expect(provinceOf('DN-HAICHAU')).toBe('DN'); + }); + + it('sends the outline once, not on every viewport query', async () => { + // Returning it with each pan gave the client a new object every time, which + // made the map refit and cancel whatever the user had zoomed into. + const first = await (await GET(request('region=DN'))).json(); + expect(first.boundary).toBeTruthy(); + + const panned = await (await GET(request('region=DN&bbox=108.1,16.0,108.3,16.1'))).json(); + expect(panned.boundary).toBeUndefined(); + expect(panned.counts.inView).toBeLessThanOrEqual(panned.counts.total); + }); +}); diff --git a/tests/region-request.test.js b/tests/region-request.test.js new file mode 100644 index 0000000..6597ae2 --- /dev/null +++ b/tests/region-request.test.js @@ -0,0 +1,88 @@ +import { describe, it, expect } from 'vitest'; +import { + resolveRegion, + resolvePlayableRegion, + publicRegion, +} from '../src/lib/region-request.js'; + +const params = (query) => new URLSearchParams(query); + +describe('resolveRegion', () => { + it('accepts a region at any level', () => { + for (const code of ['VN', 'TPHCM', 'TPHCM-Q7', 'DL']) { + expect(resolveRegion(params(`region=${code}`), true)).toEqual({ ok: true, code }); + } + }); + + it('still accepts ?city=, which every existing link sends', () => { + expect(resolveRegion(params('city=HN'), true)).toEqual({ ok: true, code: 'HN' }); + }); + + it('prefers region over city when both are present', () => { + expect(resolveRegion(params('region=DL&city=HN'), true).code).toBe('DL'); + }); + + it('uppercases what it is given', () => { + expect(resolveRegion(params('region=tphcm-q7'), true).code).toBe('TPHCM-Q7'); + }); + + it('treats an absent region as the country when one is optional', () => { + expect(resolveRegion(params(''), false)).toEqual({ ok: true, code: 'VN' }); + }); + + it('treats an empty region as the country rather than an error', () => { + // ?city= with no value is what a cleared filter sends, and it used to mean + // the global board. + expect(resolveRegion(params('city='), false)).toEqual({ ok: true, code: 'VN' }); + }); + + it('rejects an unknown code with a 400, not an empty result', () => { + const result = resolveRegion(params('region=NOPE'), true); + expect(result.ok).toBe(false); + expect(result.status).toBe(400); + expect(result.error).toMatch(/Unknown region: NOPE/); + }); + + it('rejects a missing region when one is required', () => { + expect(resolveRegion(params(''), true).status).toBe(400); + }); +}); + +describe('resolvePlayableRegion', () => { + it('accepts a region with coverage', () => { + expect(resolvePlayableRegion(params('region=DL')).ok).toBe(true); + }); + + it.each(['TPHCM-CUCHI', 'DN-CAMLE', 'DN-HOAVANG'])( + 'rejects %s, which has no coverage', + (code) => { + // Better here than as "no panoramas left to try" from deep inside the + // draw. Cu Chi has no boundary; the two Da Nang districts have no imagery. + const result = resolvePlayableRegion(params(`region=${code}`)); + expect(result.ok).toBe(false); + expect(result.status).toBe(400); + expect(result.error).toMatch(/no street view coverage/); + } + ); +}); + +describe('publicRegion', () => { + it('describes what the player picked', () => { + expect(publicRegion('TPHCM')).toEqual({ + code: 'TPHCM', + name: 'Ho Chi Minh', + path: ['Vietnam', 'Ho Chi Minh'], + level: 'province', + }); + }); + + it('gives the country a one-element path', () => { + // A country round must not hint at a province, let alone a district. + expect(publicRegion('VN').path).toEqual(['Vietnam']); + }); + + it('carries no coordinates', () => { + const serialised = JSON.stringify(publicRegion('DL')); + expect(serialised).not.toMatch(/lat|lng|bbox|center/); + }); +}); diff --git a/tests/session.test.js b/tests/session.test.js index 84f3254..71e1c12 100644 --- a/tests/session.test.js +++ b/tests/session.test.js @@ -13,7 +13,8 @@ import { fakeOnly, resetStore, storedKeys, ttlOf } from './redis-harness.js'; const SESSION = { sessionId: 'sess-1', - cityCode: 'TPHCM', + pickedRegion: 'TPHCM', + regionCode: 'TPHCM-Q7', exactLocation: { lat: 10.7769, lng: 106.7009 }, imageId: '123456789', createdAt: 1_700_000_000_000, @@ -44,9 +45,9 @@ describe('game sessions', () => { it('keeps sessions separate', async () => { await storeGameSession('sess-1', SESSION); - await storeGameSession('sess-2', { ...SESSION, sessionId: 'sess-2', cityCode: 'HN' }); - expect((await getGameSession('sess-1')).cityCode).toBe('TPHCM'); - expect((await getGameSession('sess-2')).cityCode).toBe('HN'); + await storeGameSession('sess-2', { ...SESSION, sessionId: 'sess-2', regionCode: 'HN-BADINH' }); + expect((await getGameSession('sess-1')).regionCode).toBe('TPHCM-Q7'); + expect((await getGameSession('sess-2')).regionCode).toBe('HN-BADINH'); }); it('overwrites on re-store', async () => { @@ -61,8 +62,17 @@ describe('game sessions', () => { expect(await getGameSession('sess-1')).toBeNull(); }); - it('tolerates deleting a session that is already gone', async () => { - await expect(deleteGameSession('never-existed')).resolves.toBe(true); + it('reports that it did not remove a session that was already gone', async () => { + // The return value is a claim, not a status: scoring is gated on it, so a + // second submit of the same session has to come back false rather than + // report success for a delete that removed nothing. + await expect(deleteGameSession('never-existed')).resolves.toBe(false); + }); + + it('reports the claim exactly once for a session that exists', async () => { + await storeGameSession('claim-me', SESSION); + await expect(deleteGameSession('claim-me')).resolves.toBe(true); + await expect(deleteGameSession('claim-me')).resolves.toBe(false); }); it('sets a thirty minute expiry', async () => {