fix: invalidate cache after PlayerService write operations (#32) #47

cal · 2026-03-05T19:04:07Z

cal commented

2026-03-05 19:04:07 +00:00

Summary

PlayerService defined cache_patterns but never used them after write operations
Added finally: blocks to update_player, patch_player, create_players, and delete_player that call temp_service.invalidate_related_cache(cls.cache_patterns)
This matches the pattern already correctly implemented in TeamService

Files Changed

app/services/player_service.py — 4 finally: blocks added to write methods

How It Works

cache_patterns = ["players*", "players-search*", "player*", "team-roster*"] covers all GET endpoint cache keys (keyed by key_prefix in @cache_result). The finally: block ensures invalidation runs even if the write operation raises an exception.

Other Observations

TeamService write methods also call temp_service.close_db() in their finally: blocks; PlayerService write methods do not. This inconsistency is pre-existing and out of scope for this issue.
Several Pyright type errors exist in player_service.py (Peewee model attribute access, protocol mismatches) — all pre-existing and unrelated to this fix.

## Summary - `PlayerService` defined `cache_patterns` but never used them after write operations - Added `finally:` blocks to `update_player`, `patch_player`, `create_players`, and `delete_player` that call `temp_service.invalidate_related_cache(cls.cache_patterns)` - This matches the pattern already correctly implemented in `TeamService` ## Files Changed - `app/services/player_service.py` — 4 `finally:` blocks added to write methods ## How It Works `cache_patterns = ["players*", "players-search*", "player*", "team-roster*"]` covers all GET endpoint cache keys (keyed by `key_prefix` in `@cache_result`). The `finally:` block ensures invalidation runs even if the write operation raises an exception. ## Other Observations - `TeamService` write methods also call `temp_service.close_db()` in their `finally:` blocks; `PlayerService` write methods do not. This inconsistency is pre-existing and out of scope for this issue. - Several Pyright type errors exist in `player_service.py` (Peewee model attribute access, protocol mismatches) — all pre-existing and unrelated to this fix.

cal added 1 commit 2026-03-05 19:04:07 +00:00

fix: invalidate cache after PlayerService write operations (#32 )

Build Docker Image / build (pull_request) Successful in 2m10s

Details

ea7c62c93f

Add finally blocks to update_player, patch_player, create_players, and
delete_player in PlayerService to call invalidate_related_cache() using
the existing cache_patterns. Matches the pattern already used in
TeamService.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cal referenced this pull request

2026-03-05 19:04:18 +00:00

Add cache invalidation on write operations for cached endpoints #32

cal added the

ai-reviewing

label 2026-03-05 19:15:34 +00:00

cal reviewed 2026-03-05 19:17:04 +00:00

cal left a comment

AI Code Review

Files Reviewed

app/services/player_service.py (modified)

Findings

Correctness

The four finally: blocks are correctly placed after their respective except blocks and call temp_service.invalidate_related_cache(cls.cache_patterns) — matching the TeamService pattern exactly.
temp_service is defined before each try/finally, so it is always in scope when the finally block runs.
Auth failures via require_auth(token) occur before the try block, so unauthorized requests do not trigger cache invalidation — correct behaviour.
Cache invalidation will fire on 404-class errors (player not found), but this is harmless (nothing to evict) and consistent with TeamService.
invalidate_related_cache gracefully handles a missing Redis connection (falls back to MockCacheService via base.py).
The cache_patterns list covers all relevant GET key prefixes: players*, players-search*, player*, team-roster*.

Security

No issues. No new auth surface, no new secrets, no user-controlled data injected into cache keys.

Style & Conventions

The additional diff (single→double quotes, query/dict reformatting) is cosmetic and consistent with the project's existing style elsewhere in the file.
The pre-existing inconsistency noted in the PR body (TeamService calls close_db() in finally, PlayerService does not) is correctly identified as out of scope.

Suggestions

None that block this PR. The close_db() gap versus TeamService could be addressed in a follow-up if connection exhaustion becomes a concern under load.

Verdict: APPROVED

Straightforward and correct. The fix closes the gap between cache_patterns being declared and never used, aligns PlayerService with TeamService's established invalidation pattern, and the finally placement is safe in all execution paths.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/services/player_service.py` (modified) ### Findings #### Correctness - The four `finally:` blocks are correctly placed after their respective `except` blocks and call `temp_service.invalidate_related_cache(cls.cache_patterns)` — matching the `TeamService` pattern exactly. - `temp_service` is defined before each `try/finally`, so it is always in scope when the `finally` block runs. - Auth failures via `require_auth(token)` occur before the `try` block, so unauthorized requests do not trigger cache invalidation — correct behaviour. - Cache invalidation will fire on 404-class errors (player not found), but this is harmless (nothing to evict) and consistent with `TeamService`. - `invalidate_related_cache` gracefully handles a missing Redis connection (falls back to `MockCacheService` via `base.py`). - The `cache_patterns` list covers all relevant GET key prefixes: `players*`, `players-search*`, `player*`, `team-roster*`. #### Security - No issues. No new auth surface, no new secrets, no user-controlled data injected into cache keys. #### Style & Conventions - The additional diff (single→double quotes, query/dict reformatting) is cosmetic and consistent with the project's existing style elsewhere in the file. - The pre-existing inconsistency noted in the PR body (`TeamService` calls `close_db()` in `finally`, `PlayerService` does not) is correctly identified as out of scope. #### Suggestions - None that block this PR. The `close_db()` gap versus `TeamService` could be addressed in a follow-up if connection exhaustion becomes a concern under load. ### Verdict: APPROVED Straightforward and correct. The fix closes the gap between `cache_patterns` being declared and never used, aligns `PlayerService` with `TeamService`'s established invalidation pattern, and the `finally` placement is safe in all execution paths. --- *Automated review by Claude PR Reviewer*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-05 19:17:35 +00:00

cal changed target branch from main to next-release

2026-03-07 07:32:40 +00:00

cal merged commit 8e61b512cb into next-release

2026-03-10 14:33:26 +00:00

cal deleted branch ai/major-domo-database-32

2026-03-10 14:33:26 +00:00

cal referenced this issue from a commit