Conversation
Contributor
Reviewer's GuideIntroduces a new domain service layer with SWR-aware caching for heroes, players, maps, gamemodes and roles, replaces controller-based parsing with stateless parsers and SQLite-backed services, enhances Valkey API cache envelope and nginx/Lua interoperability, and rewires FastAPI routers/dependencies, tests, and metrics to use the new services and background task queue abstraction. Sequence diagram for heroes list request with SWR cachingsequenceDiagram
actor User
participant Nginx
participant Valkey
participant FastAPI
participant HeroService
participant StaticDataService
participant SQLite as StoragePort
participant BaseService
User->>Nginx: GET /heroes?role=tank
Nginx->>Valkey: GET api-cache:/heroes?role=tank
alt cache_miss_or_stale_at_nginx
Valkey-->>Nginx: null
Nginx->>FastAPI: proxy request
FastAPI->>HeroService: list_heroes(locale, role, gamemode, cache_key)
HeroService->>StaticDataService: get_or_fetch(config)
StaticDataService->>SQLite: get_static_data("heroes:locale")
alt sqlite_hit
SQLite-->>StaticDataService: {data_json, updated_at}
StaticDataService->>StaticDataService: compute age, is_stale
alt fresh
StaticDataService->>HeroService: data, is_stale=false, age
HeroService->>BaseService: _update_api_cache(cache_key, data, cache_ttl, staleness_threshold)
else stale
StaticDataService->>HeroService: data, is_stale=true, age
HeroService->>BaseService: _update_api_cache(cache_key, data, stale_cache_timeout, staleness_threshold, stale_while_revalidate)
StaticDataService->>BaseService: _enqueue_refresh(entity_type="heroes", entity_id="heroes:locale", refresh_coro)
end
else sqlite_miss
SQLite-->>StaticDataService: null
StaticDataService->>StaticDataService: cold_fetch
StaticDataService->>HeroService: call fetch_heroes_html + parse_heroes_html
HeroService-->>StaticDataService: full_heroes_list
StaticDataService->>SQLite: set_static_data("heroes:locale", data)
StaticDataService->>BaseService: _update_api_cache(cache_key, filtered_data, cache_ttl, staleness_threshold)
StaticDataService-->>HeroService: filtered_data, is_stale=false, age=0
end
HeroService-->>FastAPI: data, is_stale, age
FastAPI->>FastAPI: apply_swr_headers(response, cache_ttl, is_stale, age, staleness_threshold)
FastAPI-->>Nginx: JSON body + SWR headers
Nginx-->>User: 200 OK
else fresh_hit_at_nginx
Valkey-->>Nginx: cached_envelope
Nginx-->>User: 200 OK (served from Valkey)
end
Sequence diagram for player summary request with identity resolution and SQLite profile cachesequenceDiagram
actor User
participant Nginx
participant Valkey
participant FastAPI
participant PlayerService
participant Storage as StoragePort
participant Blizzard as BlizzardClientPort
participant BaseService
User->>Nginx: GET /players/{player_id}/summary
Nginx->>Valkey: GET api-cache:/players/{player_id}/summary
alt cache_miss_or_stale_at_nginx
Valkey-->>Nginx: null
Nginx->>FastAPI: proxy request
FastAPI->>PlayerService: get_player_summary(player_id, cache_key)
PlayerService->>PlayerService: _execute_player_request(PlayerRequest)
rect rgb(235,235,255)
PlayerService->>PlayerService: _resolve_player_identity(player_id)
alt player_id_is_blizzard_id
PlayerService->>Blizzard: fetch_player_html(blizzard_id)
Blizzard-->>PlayerService: html, blizzard_id
PlayerService->>PlayerService: _enrich_from_blizzard_id(blizzard_id)
else player_id_is_battletag
PlayerService->>Blizzard: fetch_player_summary_json(battletag)
Blizzard-->>PlayerService: search_json
PlayerService->>PlayerService: parse_player_summary_json(search_json)
alt no_summary
PlayerService->>Storage: get_player_id_by_battletag(battletag)
Storage-->>PlayerService: cached_blizzard_id or null
alt cached_blizzard_id
PlayerService->>PlayerService: parse_player_summary_json(search_json, cached_blizzard_id)
else resolve_via_redirect
PlayerService->>Blizzard: fetch_player_html(battletag)
Blizzard-->>PlayerService: html, blizzard_id
PlayerService->>PlayerService: parse_player_summary_json(search_json, blizzard_id)
end
end
end
PlayerService-->>PlayerService: PlayerIdentity(blizzard_id, player_summary, cached_html, battletag_input)
end
PlayerService->>PlayerService: _get_player_html(effective_id, identity)
alt identity.cached_html_present
PlayerService->>Storage: set_player_profile(effective_id, html, summary, battletag, name)
else no_cached_html
PlayerService->>Storage: get_player_profile(effective_id)
alt sqlite_profile_hit_and_lastUpdated_matches
Storage-->>PlayerService: {html, summary, battletag, name}
else profile_miss_or_outdated
Storage-->>PlayerService: null_or_old
PlayerService->>Blizzard: fetch_player_html(effective_id)
Blizzard-->>PlayerService: html, blizzard_id
PlayerService->>Storage: set_player_profile(effective_id, html, summary, battletag, name)
end
end
PlayerService->>PlayerService: data_factory(html, player_summary) -> summary_dict
PlayerService->>PlayerService: _check_player_staleness(effective_id) (Phase4 always False)
PlayerService->>BaseService: _update_api_cache(cache_key, summary_dict, career_path_cache_timeout)
PlayerService-->>FastAPI: summary_dict, is_stale=false, age=0
FastAPI->>FastAPI: apply_swr_headers(response, career_path_cache_timeout, false, 0)
FastAPI-->>Nginx: JSON summary + headers
Nginx-->>User: 200 OK
else fresh_hit_at_nginx
Valkey-->>Nginx: cached_envelope
Nginx-->>User: 200 OK (served from Valkey)
end
Class diagram for new domain services and infrastructure adaptersclassDiagram
class BaseService {
+CachePort cache
+StoragePort storage
+BlizzardClientPort blizzard_client
+TaskQueuePort task_queue
+_update_api_cache(cache_key, data, cache_ttl, staleness_threshold, stale_while_revalidate) void
+_enqueue_refresh(entity_type, entity_id, refresh_coro) void
}
class StaticDataService {
+get_or_fetch(config) (Any, bool, int)
+_load_from_storage(storage_key) dict
+_serve_from_storage(stored, config) (Any, bool, int)
+_apply_filter(data, result_filter) Any
+_refresh_static(config) void
+_fetch_and_store(config) Any
+_cold_fetch(config) (Any, bool, int)
+_store_in_storage(storage_key, data) void
}
class PlayerService {
+search_players(name, order_by, offset, limit, cache_key) dict
+get_player_summary(player_id, cache_key) (dict, bool, int)
+get_player_career(player_id, gamemode, platform, cache_key) (dict, bool, int)
+get_player_stats(player_id, gamemode, platform, hero, cache_key) (dict, bool, int)
+get_player_stats_summary(player_id, gamemode, platform, cache_key) (dict, bool, int)
+get_player_career_stats(player_id, gamemode, platform, hero, cache_key) (dict, bool, int)
+get_player_profile_cache(player_id) dict
+update_player_profile_cache(player_id, player_summary, html, battletag, name) void
+_execute_player_request(request) (dict, bool, int)
+_get_player_html(effective_id, identity) str
+_resolve_player_identity(player_id) PlayerIdentity
+_enrich_from_blizzard_id(blizzard_id) (dict, str)
+_calculate_retry_after(check_count) int
+_mark_player_unknown(blizzard_id, exception, battletag) void
+_handle_player_exceptions(error, player_id, identity) Never
-_check_player_staleness(player_id) bool
}
class HeroService {
+list_heroes(locale, role, gamemode, cache_key) (list~dict~, bool, int)
+get_hero(hero_key, locale, cache_key) (dict, bool, int)
+get_hero_stats(platform, gamemode, region, role, map_filter, competitive_division, order_by, cache_key) (list~dict~, bool, int)
}
class MapService {
+list_maps(gamemode, cache_key) (list~dict~, bool, int)
}
class GamemodeService {
+list_gamemodes(cache_key) (list~dict~, bool, int)
}
class RoleService {
+list_roles(locale, cache_key) (list~dict~, bool, int)
}
class StaticFetchConfig {
+str storage_key
+Callable fetcher
+str cache_key
+int cache_ttl
+int staleness_threshold
+str entity_type
+Callable parser
+Callable result_filter
}
class PlayerIdentity {
+str blizzard_id
+dict player_summary
+str cached_html
+str battletag_input
}
class PlayerRequest {
+str player_id
+str cache_key
+Callable data_factory
}
class CachePort {
<<protocol>>
+get_api_cache(cache_key) dict
+update_api_cache(cache_key, value, expire, stored_at, staleness_threshold, stale_while_revalidate) void
+get_player_status(player_id) dict
+set_player_status(player_id, check_count, retry_after, battletag) void
}
class StoragePort {
<<protocol>>
+get_static_data(key) dict
+set_static_data(key, data, data_type) void
+get_player_profile(player_id) dict
+set_player_profile(player_id, html, summary, battletag, name) void
+get_player_id_by_battletag(battletag) str
}
class BlizzardClientPort {
<<protocol>>
+request(method, url, params, headers, timeout) Any
}
class TaskQueuePort {
<<protocol>>
+enqueue(task_name, *args, job_id, coro, on_complete, on_failure, **kwargs) str
+is_job_pending_or_running(job_id) bool
}
class AsyncioTaskQueue {
-ClassVar~set~ _pending_jobs
+enqueue(task_name, *args, job_id, coro, on_complete, on_failure, **kwargs) str
+is_job_pending_or_running(job_id) bool
-_run() void
}
class StorageTable {
<<enum>>
STATIC_DATA
PLAYER_PROFILES
}
BaseService <|-- StaticDataService
BaseService <|-- PlayerService
StaticDataService <|-- HeroService
StaticDataService <|-- MapService
StaticDataService <|-- GamemodeService
StaticDataService <|-- RoleService
StaticDataService --> StaticFetchConfig
PlayerService --> PlayerIdentity
PlayerService --> PlayerRequest
BaseService --> CachePort
BaseService --> StoragePort
BaseService --> BlizzardClientPort
BaseService --> TaskQueuePort
AsyncioTaskQueue ..|> TaskQueuePort
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The test setup in
tests/conftest.pystill patchesapp.adapters.cache.valkey_cache.CacheManager.valkey_server, butCacheManagerhas been removed in favor ofValkeyCache, so this patch target should be updated (or removed) to avoid runtime import/patch errors during tests. PlayerService._check_player_stalenesscurrently ignores itsplayer_idparameter and always returnsFalse; either implement a minimal staleness check based on persisted metadata (e.g.updated_atinplayer_profiles) or drop the parameter/usage to avoid a misleading API until Phase 5 introduces the full strategy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test setup in `tests/conftest.py` still patches `app.adapters.cache.valkey_cache.CacheManager.valkey_server`, but `CacheManager` has been removed in favor of `ValkeyCache`, so this patch target should be updated (or removed) to avoid runtime import/patch errors during tests.
- `PlayerService._check_player_staleness` currently ignores its `player_id` parameter and always returns `False`; either implement a minimal staleness check based on persisted metadata (e.g. `updated_at` in `player_profiles`) or drop the parameter/usage to avoid a misleading API until Phase 5 introduces the full strategy.
## Individual Comments
### Comment 1
<location> `app/api/dependencies.py:25-27` </location>
<code_context>
-def get_blizzard_client() -> BlizzardClient:
- """Dependency for Blizzard HTTP client"""
+
+def get_blizzard_client() -> BlizzardClientPort:
+ """Dependency for Blizzard HTTP client (Singleton)."""
return BlizzardClient()
</code_context>
<issue_to_address>
**suggestion:** Docstring claims singleton, but a new BlizzardClient is instantiated per dependency call.
FastAPI will instantiate this dependency on every request unless you explicitly implement a shared/lifespan-based instance. If BlizzardClient should be a true singleton (e.g. shared HTTPX client), this conflicts with the current behavior. Please either remove the “(Singleton)” wording or introduce a shared instance via startup/lifespan or caching so the docstring matches reality.
```suggestion
def get_blizzard_client() -> BlizzardClientPort:
"""Dependency for Blizzard HTTP client."""
return BlizzardClient()
```
</issue_to_address>
### Comment 2
<location> `tests/heroes/parsers/test_hero_stats_summary.py:22` </location>
<code_context>
- ),
+ ({}, False),
+ ({"competitive_division": CompetitiveDivision.DIAMOND}, False),
+ ({"map_filter": "hanaoka"}, True),
],
)
</code_context>
<issue_to_address>
**suggestion (testing):** Extend hero stats summary parser tests to cover query-parameter mapping and error messages
Previously, tests asserted the exact Blizzard query parameters (including tier/map mapping) and the specific error message for incompatible maps. The new tests only check success vs `ParserBlizzardError`, so they won’t catch regressions in how queries are built or how validation errors are reported.
Please add:
- A test that inspects `httpx.AsyncClient.get` call arguments to confirm the correct query parameters for combinations of platform, region, `competitive_division`, `map_filter`, etc.
- A test that asserts the `ParserBlizzardError` message for the invalid map case.
This will ensure the new parser’s behavior and tier/map mappings remain fully covered by tests.
Suggested implementation:
```python
"order_by": "hero:asc",
@pytest.mark.asyncio
async def test_hero_stats_summary_builds_correct_query_params(
hero_stats_response_mock: Mock,
):
"""
Ensure the parser builds the expected Blizzard query parameters, including
tier/map mapping, when competitive_division and map_filter are provided.
"""
# Call the parser with a combination of platform, region, competitive_division and map_filter
await parse_hero_stats_summary(
platform=PlayerPlatform.PC,
region=PlayerRegion.EUROPE,
order_by="hero:asc",
competitive_division=CompetitiveDivision.DIAMOND,
map_filter="hanaoka",
)
# Verify we made exactly one HTTP call
assert hero_stats_response_mock.call_count == 1
# Inspect the call arguments to ensure the correct query parameters were used
_, kwargs = hero_stats_response_mock.call_args
params = kwargs.get("params") or {}
# These keys/values should reflect the exact Blizzard query we expect
# (including tier/map mapping).
assert params.get("platform") == "pc"
assert params.get("region") == "eu"
assert params.get("order_by") == "hero:asc"
# Mapping of CompetitiveDivision -> Blizzard tier parameter
assert params.get("tier") == "diamond"
# Mapping of map_filter -> Blizzard map parameter
assert params.get("map") == "hanaoka"
@pytest.mark.asyncio
async def test_hero_stats_summary_invalid_map_error_message(
hero_stats_response_mock: Mock,
):
"""
Ensure the parser raises ParserBlizzardError with an informative message
when an incompatible/invalid map is provided.
"""
with pytest.raises(ParserBlizzardError) as excinfo:
await parse_hero_stats_summary(
platform=PlayerPlatform.PC,
region=PlayerRegion.EUROPE,
order_by="hero:asc",
map_filter="hanaoka",
)
message = str(excinfo.value)
# Assert on a key part of the message rather than the whole string to keep
# the test resilient while still verifying the error semantics.
assert "map" in message.lower()
assert "invalid" in message.lower() or "unsupported" in message.lower()
```
1. Ensure the following names are imported at the top of `tests/heroes/parsers/test_hero_stats_summary.py` if they are not already:
- `import pytest`
- `from unittest.mock import Mock`
- `from heroes.parsers.hero_stats_summary import parse_hero_stats_summary`
- `from heroes.parsers.exceptions import ParserBlizzardError` (or the correct module where `ParserBlizzardError` lives)
- `from heroes.models import PlayerPlatform, PlayerRegion, CompetitiveDivision` (or the correct modules for these enums).
2. If your `hero_stats_response_mock` fixture wraps `httpx.AsyncClient` differently (e.g. via `respx` or a custom client wrapper), you may need to adjust how call arguments are accessed (e.g. `hero_stats_response_mock.calls[0].request.url.params` instead of `.call_args.kwargs["params"]`).
3. Update the asserted `params` keys/values (`"platform"`, `"region"`, `"tier"`, `"map"`, `"order_by"`, and their string values) to exactly match the Blizzard API contract used in your production code (for example, `"eu"` vs `"europe"`, `"pc"` vs `"platform_pc"`, or `division` vs `tier`).
4. If the invalid map scenario is triggered by a different map than `"hanaoka"` (or by a different argument name), adjust the `map_filter` argument accordingly so that the parser raises `ParserBlizzardError` in this test.
5. If your project does not use `@pytest.mark.asyncio` for async tests (e.g. uses `pytest-asyncio` with a different configuration or `anyio`), update the decorators to match your existing async test pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 4 of a Domain-Driven Design refactor, introducing domain services with Stale-While-Revalidate (SWR) caching for static content (heroes, maps, gamemodes, roles) and player profiles. It replaces legacy controller classes with a cleaner service layer that orchestrates between infrastructure adapters (Valkey cache, SQLite storage, Blizzard HTTP client, and asyncio task queue).
Changes:
- Introduces domain service layer (
HeroService,MapService,GamemodeService,RoleService,PlayerService) with SWR caching backed by SQLite persistence - Adds
AsyncioTaskQueueadapter for background refresh tasks with deduplication - Refactors Valkey cache to use metadata envelopes containing staleness thresholds and SWR directives, integrated with nginx/Lua for RFC 5861 compliance
- Updates all API routers to inject domain services instead of controllers, with SWR-aware response headers
Reviewed changes
Copilot reviewed 87 out of 98 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump to 3.43.0 |
| pyproject.toml | Adds lint exceptions for runtime imports in domain services and API dependencies |
| .env.dist | Removes PLAYER_CACHE_TIMEOUT, adds SWR staleness thresholds |
| app/config.py | Removes player cache settings, adds SWR staleness thresholds |
| app/domain/services/*.py | New service layer with SWR orchestration for static and player data |
| app/domain/ports/cache.py | Updates update_api_cache signature for SWR envelope metadata |
| app/domain/ports/task_queue.py | Extends with coro execution and callback support |
| app/adapters/cache/valkey_cache.py | Wraps API cache values in metadata envelopes with staleness metadata |
| app/adapters/tasks/asyncio_task_queue.py | New in-process task queue with deduplication |
| app/adapters/csv/csv_reader.py | Simplifies to single read method, removes legacy path |
| app/adapters/blizzard/parsers/*.py | Updates type signatures to use BlizzardClientPort protocol |
| app/api/dependencies.py | Wires up service dependencies with adapter injection |
| app/api/routers/*.py | Refactors all route handlers to use services and SWR headers |
| app/helpers.py | Adds build_cache_key, apply_swr_headers, documents new response headers |
| build/nginx/lua/valkey_handler.lua.template | Parses cache envelope, sets Age/Cache-Control headers per RFC 5861 |
| build/grafana/provisioning/dashboards/overfast-api-tasks.json | Adds SWR refresh metrics panels |
| tests/**/*.py | Updates test mocks/fixtures for service-based architecture |
| app/controllers.py, app/cache_manager.py, app/parsers.py | Removed (legacy code) |
| app/{heroes,maps,gamemodes}/controllers/*.py | Removed (replaced by services) |
| app/{heroes,maps,gamemodes}/parsers/*.py | Removed (logic moved to adapters/blizzard/parsers) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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
1 participant
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 by Sourcery
Introduce domain service layer with SWR caching for heroes, maps, gamemodes, roles, and players, replacing legacy controllers and cache manager, and integrate it with FastAPI routers and infrastructure adapters.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: