From 1e87140fb7a54bf4d2fb2dce37e0fcd4326515f0 Mon Sep 17 00:00:00 2001 From: Valentin Porchet Date: Wed, 1 Jul 2026 22:05:51 +0200 Subject: [PATCH 1/2] fix: improved DDD layering --- README.md | 12 +++++ app/domain/exceptions.py | 9 ++-- app/domain/parsers/hero.py | 5 +-- app/domain/parsers/hero_stats_summary.py | 5 +-- app/domain/parsers/player_helpers.py | 4 +- app/domain/parsers/player_profile.py | 5 +-- app/domain/services/hero_service.py | 25 +++-------- app/domain/services/player_service.py | 32 ++++++-------- pyproject.toml | 10 ++++- .../heroes/parsers/test_hero_stats_summary.py | 6 ++- tests/players/services/test_player_service.py | 44 ++++++++++++------- uv.lock | 2 +- 12 files changed, 86 insertions(+), 73 deletions(-) diff --git a/README.md b/README.md index dd1e49c7..7878703f 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,18 @@ Below is the current list of TTL values configured for the API cache. The latest ## 🐍 Architecture +The codebase follows strict DDD layering — dependencies only flow inward. `domain` has no knowledge of FastAPI, Valkey, or Postgres; it only sees `typing.Protocol` ports, which `adapters` implement. + +```mermaid +flowchart TD + api["api — routers, DI, response models"] --> domain + api --> adapters + api --> infrastructure + adapters["adapters — Blizzard client, Valkey cache, Postgres storage, taskiq"] --> domain + adapters --> infrastructure + domain["domain — parsers, services (SWR), ports"] -.->|logger only| infrastructure["infrastructure — logger, singletons, error helpers"] +``` + ### Request flow (Stale-While-Revalidate) Every cached response is stored in Valkey as an **SWR envelope** containing the payload and three timestamps: `stored_at`, `staleness_threshold`, and `stale_while_revalidate`. Nginx/OpenResty inspects the envelope on every request: diff --git a/app/domain/exceptions.py b/app/domain/exceptions.py index fd836efe..d00941f6 100644 --- a/app/domain/exceptions.py +++ b/app/domain/exceptions.py @@ -1,6 +1,7 @@ """Set of custom exceptions used in the API""" from http import HTTPStatus +from typing import Any class RateLimitedError(Exception): @@ -22,10 +23,10 @@ class OverfastError(Exception): """Generic OverFast API Exception""" status_code = HTTPStatus.INTERNAL_SERVER_ERROR.value - message = "OverFast API Error" + message: str | dict[str, Any] = "OverFast API Error" def __str__(self): - return self.message + return str(self.message) class ParserBlizzardError(OverfastError): @@ -34,9 +35,9 @@ class ParserBlizzardError(OverfastError): """ status_code = HTTPStatus.INTERNAL_SERVER_ERROR.value - message = "Parser Blizzard Error" + message: str | dict[str, Any] = "Parser Blizzard Error" - def __init__(self, status_code: int, message: str): + def __init__(self, status_code: int, message: str | dict[str, Any]): super().__init__() self.status_code = status_code self.message = message diff --git a/app/domain/parsers/hero.py b/app/domain/parsers/hero.py index d065667a..4863fc86 100644 --- a/app/domain/parsers/hero.py +++ b/app/domain/parsers/hero.py @@ -1,10 +1,9 @@ """Stateless parser functions for single hero details""" import re +from http import HTTPStatus from typing import TYPE_CHECKING -from fastapi import status - from app.config import settings from app.domain.parsers.utils import ( parse_html_root, @@ -58,7 +57,7 @@ def parse_hero_html(html: str, locale: Locale = Locale.ENGLISH_US) -> dict: abilities_section = root_tag.css_first("div.abilities-container") if not abilities_section: raise ParserBlizzardError( # noqa: TRY301 - status_code=status.HTTP_404_NOT_FOUND, + status_code=HTTPStatus.NOT_FOUND.value, message="Hero not found or not released yet", ) diff --git a/app/domain/parsers/hero_stats_summary.py b/app/domain/parsers/hero_stats_summary.py index 283264de..5b8829af 100644 --- a/app/domain/parsers/hero_stats_summary.py +++ b/app/domain/parsers/hero_stats_summary.py @@ -1,9 +1,8 @@ """Stateless parser functions for hero stats summary (pickrate/winrate from Blizzard API)""" +from http import HTTPStatus from typing import TYPE_CHECKING -from fastapi import status - from app.config import settings from app.domain.enums import PlayerGamemode, PlayerPlatform, PlayerRegion from app.domain.exceptions import ( @@ -111,7 +110,7 @@ def parse_hero_stats_json( # Validate map matches gamemode (outside try so this is never caught above). if map_filter != selected_map: raise ParserBlizzardError( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=HTTPStatus.BAD_REQUEST.value, message=f"Selected map '{map_filter}' is not compatible with '{gamemode}' gamemode.", ) diff --git a/app/domain/parsers/player_helpers.py b/app/domain/parsers/player_helpers.py index f5fa2d66..6aec5d30 100644 --- a/app/domain/parsers/player_helpers.py +++ b/app/domain/parsers/player_helpers.py @@ -74,7 +74,9 @@ def get_computed_stat_value(input_str: str) -> str | float | int: def get_division_from_icon(rank_url: str) -> CompetitiveDivision: division_name = ( rank_url.rsplit("/", maxsplit=1)[-1] # filename or inline-SVG symbol id - .split(".", maxsplit=1)[0] # drop content hash / ".png": "Rank_DiamondTier" / "goldtier" + .split(".", maxsplit=1)[ + 0 + ] # drop content hash / ".png": "Rank_DiamondTier" / "goldtier" .split("-", maxsplit=1)[0] .rsplit("_", maxsplit=1)[-1] # drop "Rank_" prefix: "DiamondTier" / "goldtier" ) diff --git a/app/domain/parsers/player_profile.py b/app/domain/parsers/player_profile.py index bc456a9b..40671178 100644 --- a/app/domain/parsers/player_profile.py +++ b/app/domain/parsers/player_profile.py @@ -7,10 +7,9 @@ - Career stats (detailed statistics per hero) """ +from http import HTTPStatus from typing import TYPE_CHECKING -from fastapi import status - from app.config import settings from app.domain.exceptions import ParserBlizzardError, ParserParsingError from app.domain.parsers.utils import ( @@ -138,7 +137,7 @@ def parse_player_profile_html( # Check if player exists if not root_tag.css_first("blz-section.Profile-masthead"): raise ParserBlizzardError( - status_code=status.HTTP_404_NOT_FOUND, + status_code=HTTPStatus.NOT_FOUND.value, message="Player not found", ) diff --git a/app/domain/services/hero_service.py b/app/domain/services/hero_service.py index c03ad39b..823ac3b3 100644 --- a/app/domain/services/hero_service.py +++ b/app/domain/services/hero_service.py @@ -3,13 +3,10 @@ import json from typing import TYPE_CHECKING, Any -from fastapi import HTTPException - from app.config import settings from app.domain.enums import Locale, PlayerGamemode, SubRole from app.domain.exceptions import ( InvalidGamemodeFilterError, - ParserBlizzardError, ParserInternalError, ParserParsingError, ) @@ -115,18 +112,12 @@ def _hero_detail_config( """Build a StaticFetchConfig for a single hero detail.""" async def _fetch() -> str: - try: - hero_html = await fetch_hero_html( - self.blizzard_client, hero_key, locale - ) - # Validate hero exists before making the second Blizzard request. - # parse_hero_html raises ParserBlizzardError (404) for unknown heroes. - parse_hero_html(hero_html, locale) - heroes_html = await fetch_heroes_html(self.blizzard_client, locale) - except ParserBlizzardError as exc: - raise HTTPException( - status_code=exc.status_code, detail=exc.message - ) from exc + hero_html = await fetch_hero_html(self.blizzard_client, hero_key, locale) + # Validate hero exists before making the second Blizzard request. + # parse_hero_html raises ParserBlizzardError (404) for unknown heroes, + # which propagates to the API layer's registered OverfastError handler. + parse_hero_html(hero_html, locale) + heroes_html = await fetch_heroes_html(self.blizzard_client, locale) return json.dumps( {"hero_html": hero_html, "heroes_html": heroes_html}, separators=(",", ":"), @@ -297,10 +288,6 @@ async def _get_hero_stats( competitive_division=competitive_division, order_by=order_by, ) - except ParserBlizzardError as exc: - raise HTTPException( - status_code=exc.status_code, detail=exc.message - ) from exc except ParserParsingError as exc: blizzard_url = f"{settings.blizzard_host}{settings.hero_stats_path}" raise ParserInternalError(blizzard_url, exc) from exc diff --git a/app/domain/services/player_service.py b/app/domain/services/player_service.py index 39af56c1..d2e63b24 100644 --- a/app/domain/services/player_service.py +++ b/app/domain/services/player_service.py @@ -7,8 +7,6 @@ if TYPE_CHECKING: from collections.abc import Callable -from fastapi import HTTPException - from app.config import settings from app.domain.enums import HeroKeyCareerFilter, PlayerGamemode, PlayerPlatform from app.domain.exceptions import ( @@ -590,7 +588,7 @@ def _calculate_retry_after(self, check_count: int) -> int: async def _mark_player_unknown( self, blizzard_id: str, - exception: HTTPException, + exception: ParserBlizzardError, battletag: str | None = None, ) -> None: if not settings.unknown_players_cache_enabled: @@ -607,7 +605,7 @@ async def _mark_player_unknown( blizzard_id, check_count, retry_after, battletag=battletag ) - exception.detail = { # ty: ignore[invalid-assignment] + exception.message = { "error": "Player not found", "retry_after": retry_after, "next_check_at": next_check_at, @@ -627,29 +625,32 @@ async def _handle_player_exceptions( player_id: str, identity: PlayerIdentity, ) -> Never: - """Translate all player exceptions to HTTPException and always raise.""" + """Translate known parser exceptions to a client-facing error and always raise. + + Raised errors are ``ParserBlizzardError``/``ParserInternalError`` — the API + layer's registered ``OverfastError`` handler turns them into HTTP responses. + """ effective_id = identity.blizzard_id or player_id battletag_input = identity.battletag_input player_summary = identity.player_summary if isinstance(error, ParserBlizzardError): - exc = HTTPException(status_code=error.status_code, detail=error.message) if error.status_code == HTTPStatus.NOT_FOUND.value: await self._mark_player_unknown( - effective_id, exc, battletag=battletag_input + effective_id, error, battletag=battletag_input ) - raise exc from error + raise error if isinstance(error, ParserParsingError): if "Could not find main content in HTML" in str(error): - exc = HTTPException( + not_found = ParserBlizzardError( status_code=HTTPStatus.NOT_FOUND.value, - detail="Player not found", + message="Player not found", ) await self._mark_player_unknown( - effective_id, exc, battletag=battletag_input + effective_id, not_found, battletag=battletag_input ) - raise exc from error + raise not_found from error blizzard_url = ( f"{settings.blizzard_host}{settings.career_path}/" @@ -657,11 +658,4 @@ async def _handle_player_exceptions( ) raise ParserInternalError(blizzard_url, error) from error - if isinstance(error, HTTPException): - if error.status_code == HTTPStatus.NOT_FOUND.value: - await self._mark_player_unknown( - effective_id, error, battletag=battletag_input - ) - raise error - raise error diff --git a/pyproject.toml b/pyproject.toml index 2cafacdc..0efa9666 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -143,11 +143,19 @@ ignore = [ allowed-confusables = ["（", "）", "："] [tool.ruff.lint.per-file-ignores] -"tests/**" = ["SLF001"] # Ignore private member access in tests +"tests/**" = ["SLF001", "TID251"] # Ignore private member access + fastapi.TestClient in tests "app/api/models/**" = ["TC001"] # Ignore type-checking block (Pydantic models definition) "app/api/routers/**" = ["TC001"] # Enums used in FastAPI path/query parameters need runtime import "app/api/dependencies.py" = ["TC001"] # Port types needed at runtime for FastAPI Depends "app/domain/services/**" = ["TC001"] # Locale/enums used in runtime method annotations +"app/api/**" = ["TID251"] # API layer owns FastAPI +"app/adapters/**" = ["TID251"] # BlizzardClient raises HTTPException for transport errors +"app/infrastructure/**" = ["TID251"] # Shared HTTPException helpers (Discord alerts, etc.) +"app/monitoring/**" = ["TID251"] # Prometheus/FastAPI router glue +"app/main.py" = ["TID251"] # App assembly + +[tool.ruff.lint.flake8-tidy-imports.banned-api] +"fastapi".msg = "Domain layer must stay framework-agnostic. Raise app.domain.exceptions instead of fastapi.HTTPException, use http.HTTPStatus instead of fastapi.status." [tool.ruff.lint.isort] # Consider app as first-party for imports in tests diff --git a/tests/heroes/parsers/test_hero_stats_summary.py b/tests/heroes/parsers/test_hero_stats_summary.py index 8fd960da..c86c5ec7 100644 --- a/tests/heroes/parsers/test_hero_stats_summary.py +++ b/tests/heroes/parsers/test_hero_stats_summary.py @@ -111,8 +111,10 @@ async def test_parse_hero_stats_summary_invalid_map_error_message( map_filter="hanaoka", ) - assert "hanaoka" in exc_info.value.message - assert "compatible" in exc_info.value.message.lower() + message = str(exc_info.value.message) + + assert "hanaoka" in message + assert "compatible" in message.lower() def test_parse_hero_stats_json_raises_invalid_gamemode_filter_error(): diff --git a/tests/players/services/test_player_service.py b/tests/players/services/test_player_service.py index 0929c6b4..04a9b5a2 100644 --- a/tests/players/services/test_player_service.py +++ b/tests/players/services/test_player_service.py @@ -269,7 +269,9 @@ class TestMarkPlayerUnknown: @pytest.mark.asyncio async def test_disabled_feature_is_noop(self): svc = _make_service() - exc = HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not found") + exc = ParserBlizzardError( + status_code=status.HTTP_404_NOT_FOUND, message="not found" + ) with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = False await svc._mark_player_unknown("abc123", exc) @@ -278,8 +280,8 @@ async def test_disabled_feature_is_noop(self): @pytest.mark.asyncio async def test_non_404_is_noop(self): svc = _make_service() - exc = HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="error" + exc = ParserBlizzardError( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, message="error" ) with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = True @@ -292,7 +294,9 @@ async def test_404_marks_player(self): cache.get_player_status = AsyncMock(return_value=None) cache.set_player_status = AsyncMock() svc = _make_service(cache=cache) - exc = HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not found") + exc = ParserBlizzardError( + status_code=status.HTTP_404_NOT_FOUND, message="not found" + ) with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = True s.unknown_player_initial_retry = 60 @@ -307,7 +311,9 @@ async def test_404_increments_check_count(self): cache.get_player_status = AsyncMock(return_value={"check_count": 3}) cache.set_player_status = AsyncMock() svc = _make_service(cache=cache) - exc = HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not found") + exc = ParserBlizzardError( + status_code=status.HTTP_404_NOT_FOUND, message="not found" + ) with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = True s.unknown_player_initial_retry = 60 @@ -336,7 +342,7 @@ async def test_blizzard_404_raises_http_not_found(self): identity = PlayerIdentity() with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = False - with pytest.raises(HTTPException) as exc_info: + with pytest.raises(ParserBlizzardError) as exc_info: await svc._handle_player_exceptions(error, "TeKrop-2217", identity) assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND @@ -348,7 +354,7 @@ async def test_parser_parsing_error_main_content_raises_404(self): identity = PlayerIdentity() with patch("app.domain.services.player_service.settings") as s: s.unknown_players_cache_enabled = False - with pytest.raises(HTTPException) as exc_info: + with pytest.raises(ParserBlizzardError) as exc_info: await svc._handle_player_exceptions(error, "TeKrop-2217", identity) assert exc_info.value.status_code == status.HTTP_404_NOT_FOUND @@ -369,14 +375,18 @@ async def test_parser_parsing_error_other_raises_parser_internal_error(self): assert exc_info.value.cause is error @pytest.mark.asyncio - async def test_http_exception_404_marks_and_reraises(self): + async def test_unrecognized_exception_type_reraises_unmarked(self): + """An HTTPException raised by the Blizzard adapter (e.g. 503 rate limit) + isn't a domain exception, so it passes through unchanged — no unknown-player + marking, since the marking logic only inspects ParserBlizzardError/ParserParsingError.""" svc = _make_service() error = HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not Found") identity = PlayerIdentity() - with patch("app.domain.services.player_service.settings") as s: - s.unknown_players_cache_enabled = False - with pytest.raises(HTTPException): - await svc._handle_player_exceptions(error, "TeKrop-2217", identity) + with pytest.raises(HTTPException) as exc_info: + await svc._handle_player_exceptions(error, "TeKrop-2217", identity) + + assert exc_info.value is error + cast("Any", svc.cache).set_player_status.assert_not_awaited() @pytest.mark.asyncio async def test_http_exception_non_404_reraises(self): @@ -699,10 +709,10 @@ async def test_updates_persistent_storage(self): assert profile is not None @pytest.mark.asyncio - async def test_blizzard_error_propagates_as_http_exception(self): - """A ParserBlizzardError from identity resolution is translated to an - HTTPException by _handle_player_exceptions and re-raised — the worker's - _run_refresh_task except block captures it.""" + async def test_blizzard_error_propagates(self): + """A ParserBlizzardError from identity resolution is re-raised as-is by + _handle_player_exceptions — the worker's _run_refresh_task except block + captures it.""" svc = _make_service() err = ParserBlizzardError( status.HTTP_503_SERVICE_UNAVAILABLE, "Blizzard unavailable" @@ -722,7 +732,7 @@ async def test_blizzard_error_propagates_as_http_exception(self): s.player_staleness_threshold = 3600 s.prometheus_enabled = False s.unknown_players_cache_enabled = False - with pytest.raises(HTTPException) as exc_info: + with pytest.raises(ParserBlizzardError) as exc_info: await svc.refresh_player_profile("TeKrop-2217") assert exc_info.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE diff --git a/uv.lock b/uv.lock index ab58f60a..a1d96319 100644 --- a/uv.lock +++ b/uv.lock @@ -791,7 +791,7 @@ wheels = [ [[package]] name = "overfast-api" -version = "4.7.0" +version = "4.7.1" source = { virtual = "." } dependencies = [ { name = "asyncpg" }, From 432be6b4733a5940f43992ff07884420b4aabc31 Mon Sep 17 00:00:00 2001 From: Valentin Porchet Date: Wed, 1 Jul 2026 22:29:59 +0200 Subject: [PATCH 2/2] Update test_player_service.py --- tests/players/services/test_player_service.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/players/services/test_player_service.py b/tests/players/services/test_player_service.py index 04a9b5a2..e6a6d078 100644 --- a/tests/players/services/test_player_service.py +++ b/tests/players/services/test_player_service.py @@ -303,7 +303,13 @@ async def test_404_marks_player(self): s.unknown_player_retry_multiplier = 2 s.unknown_player_max_retry = 3600 await svc._mark_player_unknown("abc123", exc, battletag="TeKrop-2217") + cache.set_player_status.assert_awaited_once() + assert isinstance(exc.message, dict) + assert exc.message["error"] == "Player not found" + assert "retry_after" in exc.message + assert "next_check_at" in exc.message + assert "check_count" in exc.message @pytest.mark.asyncio async def test_404_increments_check_count(self):