feat(pinballmap) Pinball Map client with on-disk cache + per-source politeness#83
Merged
Conversation
…oliteness
Phase 3 Wave 1 PR 3. Adds a typed HTTP client for the public Pinball Map
API (pinballmap.com/api/v1/region/{region}/locations.json), mirroring the
OpdbClient pattern (politeness gate + on-disk cache + atomic .tmp write).
Why now: the citation showcase surface for the Phase 3 Wizard answer flow
needs a reliable, polite source of pinball-location-by-region data with
OPDB-id linkage to our canonical machine catalog. Each location's
location_machine_xrefs[].machine.opdb_id is the bridge — a Wizard answer
that says "this machine lives at these locations" can cite a concrete
public source. The same client also lays the data foundation for future
Phase 5+ valuation features that join location density against pricing.
Polite-by-construction: extends PoliteScraperBase, routes every request
through IPolitenessGate, identifies as PinballWizard/0.1 per
feedback_polite_scraping.md. Seed manifest entry sets requestDelayMs to
5000 (vs the site's published Crawl-delay: 3) for headroom; overrides
land in the IngestionSource Cosmos doc. The Pinball Map robots.txt
disallows AI crawlers but allows our identifying UA.
DTO shape was probed against the live API on 2026-05-04 (per DL-0002 —
the OPDB integration once shipped against an assumed contract that the
real API never honored; unit tests must not pin a self-defined shape).
A live-contract test exists, gated behind PINBALL_WIZARD_LIVE_CONTRACT_TESTS=1
so CI does not hit the real API on every build.
Tests: 13 added (5-test polite-scraper template + on-disk cache hit /
miss / stale / atomic-write / per-region-key-isolation / persist-failure
tolerance + live-contract). Production-manifest pin updated 9 to 10
entries.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| manufacturer = "Stern", | ||
| year = 2017, | ||
| opdb_id = opdbId, | ||
| ipdb_id = (int?)null, |
| .Trim() | ||
| .ToLowerInvariant() | ||
| .Select(c => char.IsLetterOrDigit(c) || c == '-' || c == '_' ? c : '-')); | ||
| return Path.Combine(dir, $"locations-{safe}.json"); |
|
|
||
| public PinballMapClientTests() | ||
| { | ||
| _cacheDir = Path.Combine(Path.GetTempPath(), $"pinballmap-tests-{Guid.NewGuid():N}"); |
| var locs = await client.GetLocationsByRegionAsync("chicago", CancellationToken.None); | ||
|
|
||
| Assert.Equal(2, locs.Count); | ||
| var cachePath = Path.Combine(_cacheDir, "locations-chicago.json"); |
| { | ||
| // Cache file exists but is older than TTL → refetch. | ||
| Directory.CreateDirectory(_cacheDir); | ||
| var cachePath = Path.Combine(_cacheDir, "locations-chicago.json"); |
| await client.GetLocationsByRegionAsync("chicago", CancellationToken.None); | ||
|
|
||
| Assert.Single(handler.Requests); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); |
| // fetch must still succeed; the persist failure is logged and | ||
| // swallowed. | ||
| var conflictingFilePath = Path.GetTempFileName(); | ||
| var unwritablePath = Path.Combine(conflictingFilePath, "pinballmap-tests"); |
|
|
||
| Assert.Equal("Chicago A", chicago[0].Name); | ||
| Assert.Equal("Portland A", portland[0].Name); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); |
| Assert.Equal("Chicago A", chicago[0].Name); | ||
| Assert.Equal("Portland A", portland[0].Name); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-portland.json"))); |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+205
to
+211
| catch (Exception cleanupEx) | ||
| { | ||
| Logger.LogDebug( | ||
| cleanupEx, | ||
| "PinballMap: best-effort cleanup of temp cache file {TmpPath} failed; ignoring.", | ||
| tmpPath); | ||
| } |
| .Trim() | ||
| .ToLowerInvariant() | ||
| .Select(c => char.IsLetterOrDigit(c) || c == '-' || c == '_' ? c : '-')); | ||
| return Path.Combine(dir, $"locations-{safe}.json"); |
|
|
||
| public PinballMapClientTests() | ||
| { | ||
| _cacheDir = Path.Combine(Path.GetTempPath(), $"pinballmap-tests-{Guid.NewGuid():N}"); |
| var locs = await client.GetLocationsByRegionAsync("chicago", CancellationToken.None); | ||
|
|
||
| Assert.Equal(2, locs.Count); | ||
| var cachePath = Path.Combine(_cacheDir, "locations-chicago.json"); |
| // is never invoked AND the returned locations match the cache | ||
| // contents (not whatever the handler would have returned). | ||
| Directory.CreateDirectory(_cacheDir); | ||
| var cachePath = Path.Combine(_cacheDir, "locations-chicago.json"); |
| { | ||
| // Cache file exists but is older than TTL → refetch. | ||
| Directory.CreateDirectory(_cacheDir); | ||
| var cachePath = Path.Combine(_cacheDir, "locations-chicago.json"); |
| await client.GetLocationsByRegionAsync("chicago", CancellationToken.None); | ||
|
|
||
| Assert.Single(handler.Requests); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); |
| // fetch must still succeed; the persist failure is logged and | ||
| // swallowed. | ||
| var conflictingFilePath = Path.GetTempFileName(); | ||
| var unwritablePath = Path.Combine(conflictingFilePath, "pinballmap-tests"); |
|
|
||
| Assert.Equal("Chicago A", chicago[0].Name); | ||
| Assert.Equal("Portland A", portland[0].Name); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); |
| Assert.Equal("Chicago A", chicago[0].Name); | ||
| Assert.Equal("Portland A", portland[0].Name); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-chicago.json"))); | ||
| Assert.True(File.Exists(Path.Combine(_cacheDir, "locations-portland.json"))); |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3 Wave 1 PR 3. Adds a typed HTTP client for the public Pinball Map API (
pinballmap.com/api/v1/region/{region}/locations.json), mirroring theOpdbClientpattern: extendsPoliteScraperBase, routes every request throughIPolitenessGate, on-disk cache with atomic.tmp + File.Move(overwrite), telemetry underpinwiz.pinballmap.*. The DTO shape was probed against the live API on 2026-05-04 (per DL-0002, unit tests must not pin a self-defined contract); a live-contract test gated behindPINBALL_WIZARD_LIVE_CONTRACT_TESTS=1exercises the production code path against the real endpoint.The integration is the citation surface for the Phase 3 Wizard answer flow — each location's
location_machine_xrefs[].machine.opdb_idis the bridge from a Pinball Map location to our canonical machine catalog. Politely identifying User-AgentPinballWizard/0.1is allow-listed bypinballmap.com/robots.txt(which disallows AI crawlers but not our explicit non-AI UA); the seed manifest sets a 5 s per-request delay vs the site's publishedCrawl-delay: 3for headroom.Test Plan
dotnet build PinballWizard.slnx→ 0 warnings, 0 errorsdotnet test PinballWizard.slnx→ 579 passed, 0 failed (566 baseline + 13 new)PINBALL_WIZARD_LIVE_CONTRACT_TESTS=1 dotnet test --filter LiveContract→ 1 passed against livepinballmap.comgit log -1 --format='%an <%ae>'→Jim Keeley <94459922+jkeeley2073@users.noreply.github.com>Out of Scope
Checklist
docs/adr/— N/A, mirrors existing OPDB patternREADME.mdand/ordocs/are updated in the same PR — N/A, no user-visible CLI surface changes (client used by future consumer)~/.claude/projects/c--projects-PinballWizard/memory/is now stale, it has been updated or removed in the same PR — N/ATODO/FIXME/ commented-out code committed<NoWarn>without a comment explaining why and the removal criterionPre-push self-audit (additive PRs)
Step 0 —
/local-review(qualitative)/local-reviewand addressed every 🔴 finding before pushcatch (Exception cleanupEx)+ Debug log replacescatch { /* ignore */ }fromOpdbClientper audit rule "no bare catch{}") / 10 categories (design, drift, error handling, security, provenance, polite-by-construction, telemetry, tests, documentation, live-contract) ✅Step 1 — Mechanical checklist
*Optionsproperty has at least one real getter call insrc/. Verified by grep:BaseUrl→PinballMapClient.BuildRegionUrl+ServiceCollectionExtensionsBaseUrlKey→Program.csgatingHttpTimeoutSeconds→ServiceCollectionExtensionsCacheDirectory→PinballMapClient.TryGetCachePathCacheTtlSeconds→PinballMapClient.GetRegionLocationsBytesAsyncOpdbClient; one intentional improvement noted abovecatch { }— minimum scope iscatch (Exception)everywhere; cancellation propagatesISourceScraper? N/A — this is a Phase 3 read-side integration like OPDB, not a--sourcealias scraper.SourceAliasContractTestsunaffectedPerCallFailureIsolationactually fails one region and verifies the next succeeds;PerRegionCacheKeysDoNotCollideactually exercises two regions; cache stale/hit/miss tests verify network-call counts)Directory.Build.propsenforces warnings-as-errors)git log -1 --format='%an <%ae>'shows personal noreply, not work email