ldraney/mcd-tracker-api

Archived

Fork 0

feat: seed full US McDonald's + cache-on-miss for uncovered areas #23

Merged

forgejo_admin merged 1 commit from 22-seed-full-us-mcdonald-s-cache-on-miss-fo into main

2026-03-18 17:41:12 +00:00

forgejo_admin commented

2026-03-18 17:36:38 +00:00

Contributor

Summary

Change seed script default from Denver 50km to all US McDonald's (~13k locations) using an area-based Overpass query. Add cache-on-miss to /locations/nearby: when Postgres has no seeded data for a queried area, the endpoint falls back to Overpass, caches the results, and serves them. Subsequent queries are instant from Postgres. Overpass failures during cache-on-miss return an empty list gracefully (not 503).

Changes

src/mcd_tracker_api/scripts/seed_locations.py -- Replaced Denver 50km default with full-US area query using area["ISO3166-1"="US"]. Added _build_area_query(), _build_radius_query(), fetch_locations_by_area(), and _parse_overpass_response(). Script now defaults to area mode; radius mode available via --lat/--lng/--radius flags. Increased timeout to 180s for full-US query. Added --country flag for other countries.
src/mcd_tracker_api/routes/nearby.py -- Added cache-on-miss logic: extracted _query_cached_locations() (returns coverage flag + results), _cache_overpass_results() (upserts Overpass data into mcdonalds_locations), and _cache_on_miss() (async Overpass fallback with graceful error handling). Made endpoint async to support the async Overpass client. Cache-on-miss only fires when the bounding box query finds zero rows (no coverage), not when rows exist but none pass the haversine filter.
tests/test_seed_locations.py -- Added TestBuildQueries (area query ISO3166-1, radius query around filter) and TestFetchLocationsByArea (parses area response, empty response). Existing radius-mode tests preserved.
tests/test_nearby.py -- Added TestNearbyCacheOnMiss with 7 integration tests: Overpass fallback when DB empty, Overpass failure returns empty, subsequent query serves from cache (no Overpass call), cache-on-miss does not fire when DB has data, upsert on existing osm_id, multiple locations, empty Overpass response. Updated 2 existing tests to mock Overpass where cache-on-miss would fire.

Test Plan

pytest tests/ -v -- 160 passed, 1 pre-existing flaky test (test_code_at_exact_30_day_boundary_not_active -- timing race, unrelated)
All 38 nearby + seed tests pass
Verify: python -m mcd_tracker_api.scripts.seed_locations seeds all US locations (run against real Overpass)
Verify: /locations/nearby returns cached results instantly, falls back to Overpass for uncovered areas

Review Checklist

Seed script defaults to full US instead of Denver 50km
Seed script remains idempotent (upsert on osm_id)
/locations/nearby returns cached results when available (no Overpass call)
/locations/nearby falls back to Overpass when no cached locations exist for the area
Cache-on-miss upserts into mcdonalds_locations so subsequent queries are instant
Overpass failure during cache-on-miss returns empty list gracefully (not 503)
Response schema unchanged (NearbyResponse)
All existing tests pass
ruff format + ruff check clean

Closes #22
PR #21 -- foundational work (table, model, initial seed script, refactored endpoint)

## Summary Change seed script default from Denver 50km to all US McDonald's (~13k locations) using an area-based Overpass query. Add cache-on-miss to `/locations/nearby`: when Postgres has no seeded data for a queried area, the endpoint falls back to Overpass, caches the results, and serves them. Subsequent queries are instant from Postgres. Overpass failures during cache-on-miss return an empty list gracefully (not 503). ## Changes - `src/mcd_tracker_api/scripts/seed_locations.py` -- Replaced Denver 50km default with full-US area query using `area["ISO3166-1"="US"]`. Added `_build_area_query()`, `_build_radius_query()`, `fetch_locations_by_area()`, and `_parse_overpass_response()`. Script now defaults to area mode; radius mode available via `--lat`/`--lng`/`--radius` flags. Increased timeout to 180s for full-US query. Added `--country` flag for other countries. - `src/mcd_tracker_api/routes/nearby.py` -- Added cache-on-miss logic: extracted `_query_cached_locations()` (returns coverage flag + results), `_cache_overpass_results()` (upserts Overpass data into `mcdonalds_locations`), and `_cache_on_miss()` (async Overpass fallback with graceful error handling). Made endpoint `async` to support the async Overpass client. Cache-on-miss only fires when the bounding box query finds zero rows (no coverage), not when rows exist but none pass the haversine filter. - `tests/test_seed_locations.py` -- Added `TestBuildQueries` (area query ISO3166-1, radius query around filter) and `TestFetchLocationsByArea` (parses area response, empty response). Existing radius-mode tests preserved. - `tests/test_nearby.py` -- Added `TestNearbyCacheOnMiss` with 7 integration tests: Overpass fallback when DB empty, Overpass failure returns empty, subsequent query serves from cache (no Overpass call), cache-on-miss does not fire when DB has data, upsert on existing osm_id, multiple locations, empty Overpass response. Updated 2 existing tests to mock Overpass where cache-on-miss would fire. ## Test Plan - `pytest tests/ -v` -- 160 passed, 1 pre-existing flaky test (`test_code_at_exact_30_day_boundary_not_active` -- timing race, unrelated) - All 38 nearby + seed tests pass - Verify: `python -m mcd_tracker_api.scripts.seed_locations` seeds all US locations (run against real Overpass) - Verify: `/locations/nearby` returns cached results instantly, falls back to Overpass for uncovered areas ## Review Checklist - [x] Seed script defaults to full US instead of Denver 50km - [x] Seed script remains idempotent (upsert on osm_id) - [x] `/locations/nearby` returns cached results when available (no Overpass call) - [x] `/locations/nearby` falls back to Overpass when no cached locations exist for the area - [x] Cache-on-miss upserts into `mcdonalds_locations` so subsequent queries are instant - [x] Overpass failure during cache-on-miss returns empty list gracefully (not 503) - [x] Response schema unchanged (`NearbyResponse`) - [x] All existing tests pass - [x] ruff format + ruff check clean ## Related - Closes #22 - PR #21 -- foundational work (table, model, initial seed script, refactored endpoint)

forgejo_admin added 1 commit

2026-03-18 17:36:38 +00:00

feat: seed full US McDonald's + cache-on-miss for uncovered areas

ci/woodpecker/pr/woodpecker Pipeline failed

Details

452122bd48

Change seed script default from Denver 50km to all US McDonald's using
area-based Overpass query (ISO3166-1). Add cache-on-miss to /locations/nearby:
when Postgres has no data for a queried area, falls back to Overpass, caches
the results, and serves them. Subsequent queries are instant from Postgres.
Overpass failures during cache-on-miss return empty list gracefully.

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

forgejo_admin commented

2026-03-18 17:37:16 +00:00

Author

Contributor

Self-Review

Reviewed the full diff (4 files, +456/-43). No blocking issues found.

Verified

Seed script: Default changed from Denver 50km radius to full-US area query (ISO3166-1). Radius mode preserved via --lat/--lng/--radius flags. Timeout increased to 180s. Parsing logic extracted to shared _parse_overpass_response() -- no duplication.
Cache-on-miss: Triggers only when the bounding box query returns zero rows (no coverage for the area). If the bounding box has rows but none pass haversine, the area is considered covered -- Overpass is not called. This prevents unnecessary Overpass calls in areas that are seeded but just have no McDonald's within the exact radius.
Graceful degradation: OverpassError is caught and returns empty list (200, not 503). OverpassTimeoutError is a subclass of OverpassError, so timeouts are also handled.
Upsert correctness: _cache_overpass_results queries by osm_id and updates existing rows, matching the seed script's upsert pattern.
Async: Endpoint changed from def to async def to support await query_nearby(). FastAPI handles this correctly.
Session safety: _cache_overpass_results commits on the request's DB session, same pattern used elsewhere. db.refresh() after commit ensures generated IDs are available.
Tests: 7 new cache-on-miss tests cover all acceptance criteria. 2 existing tests updated to mock Overpass where cache-on-miss would fire. All 160 tests pass (1 pre-existing flaky test unrelated to this PR).

Nit (non-blocking)

_cache_overpass_results(db: Session, osm_locations: list) -- the list type hint could be list[OsmLocation] for clarity. Not worth a fix cycle.

Test results

160 passed, 1 failed (pre-existing: test_code_at_exact_30_day_boundary_not_active -- timing race)

## Self-Review Reviewed the full diff (4 files, +456/-43). No blocking issues found. ### Verified - **Seed script**: Default changed from Denver 50km radius to full-US area query (`ISO3166-1`). Radius mode preserved via `--lat`/`--lng`/`--radius` flags. Timeout increased to 180s. Parsing logic extracted to shared `_parse_overpass_response()` -- no duplication. - **Cache-on-miss**: Triggers only when the bounding box query returns zero rows (no coverage for the area). If the bounding box has rows but none pass haversine, the area is considered covered -- Overpass is not called. This prevents unnecessary Overpass calls in areas that are seeded but just have no McDonald's within the exact radius. - **Graceful degradation**: `OverpassError` is caught and returns empty list (200, not 503). `OverpassTimeoutError` is a subclass of `OverpassError`, so timeouts are also handled. - **Upsert correctness**: `_cache_overpass_results` queries by `osm_id` and updates existing rows, matching the seed script's upsert pattern. - **Async**: Endpoint changed from `def` to `async def` to support `await query_nearby()`. FastAPI handles this correctly. - **Session safety**: `_cache_overpass_results` commits on the request's DB session, same pattern used elsewhere. `db.refresh()` after commit ensures generated IDs are available. - **Tests**: 7 new cache-on-miss tests cover all acceptance criteria. 2 existing tests updated to mock Overpass where cache-on-miss would fire. All 160 tests pass (1 pre-existing flaky test unrelated to this PR). ### Nit (non-blocking) - `_cache_overpass_results(db: Session, osm_locations: list)` -- the `list` type hint could be `list[OsmLocation]` for clarity. Not worth a fix cycle. ### Test results ``` 160 passed, 1 failed (pre-existing: test_code_at_exact_30_day_boundary_not_active -- timing race) ```

forgejo_admin commented

2026-03-18 17:39:55 +00:00

Author

Contributor

PR #23 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / httpx / pytest. Domain checklists applied: PEP compliance, OWASP, SQLAlchemy patterns, FastAPI specifics.

Async/sync mixing in the endpoint (/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/routes/nearby.py, line 209):

The endpoint was changed from def to async def so it can await _cache_on_miss() -> await query_nearby(). This is correct for the async Overpass call. However, all SQLAlchemy operations in this endpoint (_query_cached_locations(), _cache_overpass_results(), and the saved-location queries) use synchronous db.query() calls. In an async def FastAPI endpoint, synchronous I/O blocks the main asyncio event loop, whereas def endpoints run in a threadpool automatically.

For a personal tracker app under light load, this has no practical impact. Under concurrent requests, the synchronous DB calls would serialize on the event loop. The proper fix would be run_in_executor for sync DB calls or migrating to async SQLAlchemy. Noting as a nit, not a blocker.

Type hint gap (/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/routes/nearby.py, line 128):

def _cache_overpass_results(db: Session, osm_locations: list) -> list[McDonaldsLocation]:

The osm_locations parameter uses bare list instead of list[OsmLocation]. PEP 484 type hints should be specific. This makes the function's contract unclear and defeats type-checker validation.

DRY observation: _cache_overpass_results() in nearby.py and upsert_locations() in seed_locations.py both implement osm_id-based upsert logic. They operate on different input shapes (OsmLocation dataclass vs dict), so the duplication is tolerable. If the upsert logic changes (e.g., adding a new field to McDonaldsLocation), both need updating. Worth noting but not blocking.

Seed script _parse_overpass_response extraction (/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py, line 88): Good refactor -- the parsing logic was correctly extracted from fetch_locations() and shared with fetch_locations_by_area(). DRY within the script is clean.

Overpass query construction (/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py, line 41): The country_code parameter is interpolated directly into the Overpass QL query string via f-string. This is only called from main() via CLI argparse, so the input is operator-controlled, not user-facing. Not an injection risk in this context. If this were ever exposed to web input, it would need validation.

CLI partial-args edge case (/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py, line 183): If a user provides --lat and --lng but forgets --radius, the script silently falls through to area mode instead of radius mode. This could be surprising. A validation warning would be friendlier.

Graceful degradation: The _cache_on_miss() function properly catches OverpassError and returns an empty list. The has_coverage flag logic is well-designed -- it distinguishes between "no data for this area" (trigger cache-on-miss) and "data exists but none within exact radius" (area is covered, just sparse). This is correct.

BLOCKERS

None.

Test coverage: 7 new cache-on-miss tests + 4 new seed script tests. Happy path, error handling, edge cases (upsert, multiple locations, empty responses) are all covered.
No unvalidated user input in web endpoints. Query params use FastAPI's Query() with ge/le constraints.
No secrets or credentials in code.
No duplicated auth logic.

NITS

Bare list type hint -- _cache_overpass_results(db: Session, osm_locations: list) should be list[OsmLocation] for PEP 484 compliance. (nearby.py line 128)
Async endpoint with sync DB -- The async def get_nearby_locations() endpoint blocks the event loop during synchronous SQLAlchemy queries. Consider wrapping sync DB calls in asyncio.to_thread() or documenting why this is acceptable for the current scale. Not a blocker for a personal app.
CLI partial-args silent fallthrough -- Providing --lat 39.7 and --lng -104.9 without --radius silently falls through to area mode. Consider logging a warning or requiring all three when any are provided. (seed_locations.py line 183)
Test query verification fragility -- In test_fetches_all_us_locations, lines 84-89 of test_seed_locations.py use a fragile chain of .get() / call_args unpacking to verify the query string. A simpler assertion on mock_post.call_args.kwargs["data"]["data"] would be more readable, or just assert mock_post.called since the query string is already tested in TestBuildQueries.

SOP COMPLIANCE

Branch named after issue (22-seed-full-us-mcdonald-s-cache-on-miss-fo references issue #22)
PR body has: Summary, Changes, Test Plan, Related
Related references plan slug -- no plan slug referenced; only references issue #22 and PR #21
Tests exist (11 new tests across 2 test files)
No secrets, .env files, or credentials committed
No unnecessary file changes (4 files changed, all directly relevant to the feature)
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment frequency: Clean, self-contained feature. No migration in this PR since the mcdonalds_locations table was added in PR #21. Ready to deploy.
Change failure risk: Low. Cache-on-miss is additive -- the existing Postgres-first path is unchanged. Overpass failures degrade gracefully to empty results.
Documentation: The module docstrings and function docstrings are thorough. The PR body is detailed with a clear review checklist.
Plan slug gap: The Related section references issue #22 and PR #21 but does not reference a plan slug from pal-e-docs. If a plan exists for this feature, it should be linked.

VERDICT: APPROVED

## PR #23 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / httpx / pytest. Domain checklists applied: PEP compliance, OWASP, SQLAlchemy patterns, FastAPI specifics. **Async/sync mixing in the endpoint** (`/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/routes/nearby.py`, line 209): The endpoint was changed from `def` to `async def` so it can `await _cache_on_miss()` -> `await query_nearby()`. This is correct for the async Overpass call. However, all SQLAlchemy operations in this endpoint (`_query_cached_locations()`, `_cache_overpass_results()`, and the saved-location queries) use synchronous `db.query()` calls. In an `async def` FastAPI endpoint, synchronous I/O blocks the main asyncio event loop, whereas `def` endpoints run in a threadpool automatically. For a personal tracker app under light load, this has no practical impact. Under concurrent requests, the synchronous DB calls would serialize on the event loop. The proper fix would be `run_in_executor` for sync DB calls or migrating to async SQLAlchemy. Noting as a nit, not a blocker. **Type hint gap** (`/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/routes/nearby.py`, line 128): ```python def _cache_overpass_results(db: Session, osm_locations: list) -> list[McDonaldsLocation]: ``` The `osm_locations` parameter uses bare `list` instead of `list[OsmLocation]`. PEP 484 type hints should be specific. This makes the function's contract unclear and defeats type-checker validation. **DRY observation**: `_cache_overpass_results()` in `nearby.py` and `upsert_locations()` in `seed_locations.py` both implement osm_id-based upsert logic. They operate on different input shapes (OsmLocation dataclass vs dict), so the duplication is tolerable. If the upsert logic changes (e.g., adding a new field to McDonaldsLocation), both need updating. Worth noting but not blocking. **Seed script `_parse_overpass_response` extraction** (`/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py`, line 88): Good refactor -- the parsing logic was correctly extracted from `fetch_locations()` and shared with `fetch_locations_by_area()`. DRY within the script is clean. **Overpass query construction** (`/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py`, line 41): The `country_code` parameter is interpolated directly into the Overpass QL query string via f-string. This is only called from `main()` via CLI argparse, so the input is operator-controlled, not user-facing. Not an injection risk in this context. If this were ever exposed to web input, it would need validation. **CLI partial-args edge case** (`/home/ldraney/mcd-tracker-api/src/mcd_tracker_api/scripts/seed_locations.py`, line 183): If a user provides `--lat` and `--lng` but forgets `--radius`, the script silently falls through to area mode instead of radius mode. This could be surprising. A validation warning would be friendlier. **Graceful degradation**: The `_cache_on_miss()` function properly catches `OverpassError` and returns an empty list. The `has_coverage` flag logic is well-designed -- it distinguishes between "no data for this area" (trigger cache-on-miss) and "data exists but none within exact radius" (area is covered, just sparse). This is correct. ### BLOCKERS None. - Test coverage: 7 new cache-on-miss tests + 4 new seed script tests. Happy path, error handling, edge cases (upsert, multiple locations, empty responses) are all covered. - No unvalidated user input in web endpoints. Query params use FastAPI's `Query()` with `ge`/`le` constraints. - No secrets or credentials in code. - No duplicated auth logic. ### NITS 1. **Bare `list` type hint** -- `_cache_overpass_results(db: Session, osm_locations: list)` should be `list[OsmLocation]` for PEP 484 compliance. (`nearby.py` line 128) 2. **Async endpoint with sync DB** -- The `async def get_nearby_locations()` endpoint blocks the event loop during synchronous SQLAlchemy queries. Consider wrapping sync DB calls in `asyncio.to_thread()` or documenting why this is acceptable for the current scale. Not a blocker for a personal app. 3. **CLI partial-args silent fallthrough** -- Providing `--lat 39.7` and `--lng -104.9` without `--radius` silently falls through to area mode. Consider logging a warning or requiring all three when any are provided. (`seed_locations.py` line 183) 4. **Test query verification fragility** -- In `test_fetches_all_us_locations`, lines 84-89 of `test_seed_locations.py` use a fragile chain of `.get()` / `call_args` unpacking to verify the query string. A simpler assertion on `mock_post.call_args.kwargs["data"]["data"]` would be more readable, or just assert `mock_post.called` since the query string is already tested in `TestBuildQueries`. ### SOP COMPLIANCE - [x] Branch named after issue (`22-seed-full-us-mcdonald-s-cache-on-miss-fo` references issue #22) - [x] PR body has: Summary, Changes, Test Plan, Related - [ ] Related references plan slug -- no plan slug referenced; only references issue #22 and PR #21 - [x] Tests exist (11 new tests across 2 test files) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (4 files changed, all directly relevant to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean, self-contained feature. No migration in this PR since the `mcdonalds_locations` table was added in PR #21. Ready to deploy. - **Change failure risk**: Low. Cache-on-miss is additive -- the existing Postgres-first path is unchanged. Overpass failures degrade gracefully to empty results. - **Documentation**: The module docstrings and function docstrings are thorough. The PR body is detailed with a clear review checklist. - **Plan slug gap**: The Related section references issue #22 and PR #21 but does not reference a plan slug from pal-e-docs. If a plan exists for this feature, it should be linked. ### VERDICT: APPROVED