Support API-driven avatar changes: login.avatar_id sync + refresh consumer#1095
Merged
Conversation
The DB now has login.avatar_id as the authoritative single-avatar FK (added in faf/db PR #331). Until all writers move over, both columns must stay consistent. This change: - Adds avatar_id to the login Table definition. - Player fetch picks the avatar via COALESCE(login.avatar_id, avatars.idAvatar) so the new column wins, with fallback to the legacy selected=1 row. - command_avatar's select action now also writes login.avatar_id whenever it updates avatars.selected, so the lobby never produces drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new ChangesAuthoritative avatar_id on login with local selection
Event-driven avatar refresh via queue service
Database migration version update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/lobbyconnection.py (1)
976-984: ⚡ Quick winExtend avatar selection tests to assert
login.avatar_idsynchronization.At Line 978, the new authoritative column is written, but existing coverage (per
tests/unit_tests/test_lobbyconnection.pysnippet) only checksavatars.selected. Add assertions forlogin.avatar_idon both select and clear paths to lock the migration contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/lobbyconnection.py` around lines 976 - 984, The avatar selection logic in the lobbyconnection module now synchronizes the authoritative login.avatar_id column when avatars are selected. Update the tests in tests/unit_tests/test_lobbyconnection.py to add assertions that verify login.avatar_id is correctly synchronized on both the select avatar and clear avatar (if applicable) paths. For each test case that exercises avatar selection, assert that the login record's avatar_id matches the selected avatar_id to ensure the migration contract between the legacy avatars.selected flag and the new authoritative login.avatar_id column is properly maintained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/player_service.py`:
- Around line 111-116: The outerjoin using coalesce with login.avatar_id does
not validate that the avatar is owned by the player. Add an EXISTS or join guard
condition to the outerjoin onclause in the coalesce block to ensure that
non-null login.avatar_id values are only matched to avatars_list rows when there
is a corresponding row where both avatars.idUser equals login.id and
avatars.idAvatar equals login.avatar_id, enforcing ownership validation
consistent with the legacy path validated in the join above it.
---
Nitpick comments:
In `@server/lobbyconnection.py`:
- Around line 976-984: The avatar selection logic in the lobbyconnection module
now synchronizes the authoritative login.avatar_id column when avatars are
selected. Update the tests in tests/unit_tests/test_lobbyconnection.py to add
assertions that verify login.avatar_id is correctly synchronized on both the
select avatar and clear avatar (if applicable) paths. For each test case that
exercises avatar selection, assert that the login record's avatar_id matches the
selected avatar_id to ensure the migration contract between the legacy
avatars.selected flag and the new authoritative login.avatar_id column is
properly maintained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d92c8323-d992-4909-81a0-59f76f19d2a2
📒 Files selected for processing (3)
server/db/models.pyserver/lobbyconnection.pyserver/player_service.py
v143 adds login.avatar_id, which the avatar fetch and command_avatar write paths in this PR now reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit flagged two issues on the previous commit: 1. The COALESCE-based avatar join trusted login.avatar_id even if the underlying grant in `avatars` had been revoked. Restructure the join to route through the grant table in both cases: when avatar_id is set, match on idUser+idAvatar (= ownership); otherwise fall back to selected=1. A stale avatar_id pointing at a no-longer-granted avatar now yields no row. 2. test_command_avatar_select only checked avatars.selected. Add an assertion that login.avatar_id mirrors the selected idAvatar, and a new test_command_avatar_select_clear covering the null path (avatars.selected cleared AND login.avatar_id set to NULL). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the API changes a player's avatar (e.g. via the website), the lobby
has no way to know — its in-memory Player.avatar stays stale until the
player reconnects. Add a RabbitMQ consumer on the new
`success.player_avatar.update` routing key that re-reads the avatar from
the DB and marks the player dirty so BroadcastService emits the usual
player_info to all connected clients on the next tick.
Wire contract (for the API team to implement the publish side):
- exchange: MQ_EXCHANGE_NAME (faf-lobby)
- routing key: success.player_avatar.update
- payload: {"player_id": <int>, "avatar_id": <int|null>}
avatar_id is informational for downstream subscribers; the lobby
re-reads from DB to enforce the ownership check and pick up
url/tooltip.
Includes:
- PlayerService.refresh_player_avatar(player_id) public API + an
extracted _fetch_player_avatar helper (uses the same ownership-checked
join as fetch_player_data).
- AvatarChangeQueueService following the ClientMessageQueueService
pattern; auto-registered via Service.__init_subclass__.
- Unit tests covering valid/invalid payloads, missing player_id, and
unconnected players, plus a refresh_player_avatar test against the
shared test DB.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brutus5000
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/player_service.py (1)
111-117: ⚡ Quick winExtract the avatar join predicate into a shared helper to prevent drift.
The ownership/fallback predicate is duplicated in two query paths. Centralizing it keeps avatar resolution behavior consistent as migration logic evolves.
♻️ Proposed refactor
+ `@staticmethod` + def _avatar_grant_join_onclause(): + return and_( + avatars.c.idUser == login.c.id, + or_( + avatars.c.idAvatar == login.c.avatar_id, + and_( + login.c.avatar_id.is_(None), + avatars.c.selected == 1 + ) + ) + ) ... .outerjoin( avatars, - onclause=and_( - avatars.c.idUser == login.c.id, - or_( - avatars.c.idAvatar == login.c.avatar_id, - and_( - login.c.avatar_id.is_(None), - avatars.c.selected == 1 - ) - ) - ) + onclause=self._avatar_grant_join_onclause() ) ... .outerjoin( avatars, - onclause=and_( - avatars.c.idUser == login.c.id, - or_( - avatars.c.idAvatar == login.c.avatar_id, - and_( - login.c.avatar_id.is_(None), - avatars.c.selected == 1 - ) - ) - ) + onclause=self._avatar_grant_join_onclause() )Also applies to: 157-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/player_service.py` around lines 111 - 117, The avatar join predicate logic (the or_ condition that checks if idAvatar matches OR if avatar_id is None and selected equals 1) is duplicated across two query paths in player_service.py. Extract this predicate into a shared helper function and call it from both locations (around lines 111-117 and 157-163) to ensure consistent avatar resolution behavior as the migration logic evolves and prevent future drift when this logic needs to be updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/avatar_change_queue_service.py`:
- Around line 24-36: The import section has incorrect blank-line spacing that
does not conform to isort standards. Run isort on the
server/avatar_change_queue_service.py file to automatically correct the spacing
between import groups (standard library imports, third-party imports like
aio_pika, and local imports from the current package). This will ensure the
imports are properly formatted with the correct number of blank lines between
each group.
- Around line 94-101: The validation logic for player_id in the payload parsing
section does not reject boolean values, since int(True) and int(False) return 1
and 0 respectively without raising an exception. Before the int conversion on
raw_player_id, add an explicit check to reject boolean types and treat them as
invalid values. Include this boolean type check in the same exception handling
pattern as the existing (TypeError, ValueError) catch block to ensure malformed
messages with boolean player_id values are dropped with an appropriate warning
log.
In `@tests/unit_tests/test_avatar_change_queue_service.py`:
- Around line 6-9: The multiline import statement from
server.avatar_change_queue_service is missing a trailing comma after the last
import item AvatarChangeQueueService, which violates isort formatting standards.
Add a trailing comma after AvatarChangeQueueService in the import block
containing PLAYER_AVATAR_UPDATE_ROUTING_KEY and AvatarChangeQueueService, and
then run isort to automatically format the imports according to the project
standards.
---
Nitpick comments:
In `@server/player_service.py`:
- Around line 111-117: The avatar join predicate logic (the or_ condition that
checks if idAvatar matches OR if avatar_id is None and selected equals 1) is
duplicated across two query paths in player_service.py. Extract this predicate
into a shared helper function and call it from both locations (around lines
111-117 and 157-163) to ensure consistent avatar resolution behavior as the
migration logic evolves and prevent future drift when this logic needs to be
updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38739cdd-05d8-480c-abd1-67b35cce84e8
📒 Files selected for processing (5)
server/__init__.pyserver/avatar_change_queue_service.pyserver/player_service.pytests/unit_tests/test_avatar_change_queue_service.pytests/unit_tests/test_player_service.py
CI: - isort: drop trailing comma in test import and the extra blank line before the routing-key constant. - mypy: annotate raw_player_id as Any so int(payload.get(...)) typechecks (matches the pattern in ClientMessageQueueService._dispatch_to_user). CodeRabbit review: - Reject bool player_id explicitly; int(True) == 1 would otherwise refresh player 1 on every truthy payload. New test covers this. - Extract the avatar grant-table join onclause into _avatar_grant_join_onclause so fetch_player_data and _fetch_player_avatar can't drift apart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- D212: module docstring is now a single-line summary, with the wire contract moved into the class docstring where it's more discoverable. - D203: blank line before the class docstring. - D107: add __init__ docstring (matches ClientMessageQueueService). - D205/D415/D213 on refresh_player_avatar: restructure as second-line summary ending in a period, with a blank line before the description. D212 and D213 are mutually exclusive pydocstyle rules — the codebase elsewhere follows D213, so multi-line docstrings keep the summary-on-second-line style and the module docstring is collapsed to a one-liner to dodge the conflict entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/avatar_change_queue_service.py (1)
96-107:⚠️ Potential issue | 🟠 MajorEnforce strict integer type for
player_id(no coercion).The wire contract (lines 25-30) explicitly requires
player_idto be an int, but line 106 coerces values withint(raw_player_id). This allows malformed payloads like{"player_id": 1.9}or{"player_id": "1"}to pass;int(1.9)silently truncates to1, potentially refreshing the wrong player. Replace the coercion with a strictisinstance(raw_player_id, int)check (after bool rejection) to enforce the contract.Suggested patch
raw_player_id: Any = payload.get("player_id") - # Reject bool explicitly: int(True) == 1 would otherwise sneak - # through and refresh player 1 on every truthy payload. - if isinstance(raw_player_id, bool): + # Enforce exact wire contract type: JSON integer only. + if not isinstance(raw_player_id, int) or isinstance(raw_player_id, bool): self._logger.warning( "Dropping avatar-update message: invalid player_id %r", raw_player_id, ) return - try: - player_id = int(raw_player_id) - except (TypeError, ValueError): - self._logger.warning( - "Dropping avatar-update message: invalid player_id %r", - raw_player_id, - ) - return + player_id = raw_player_id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/avatar_change_queue_service.py` around lines 96 - 107, The code currently coerces raw_player_id to an integer using int(raw_player_id), which allows malformed payloads like floats (1.9) or strings ("1") to silently pass through. Replace the try-except block starting at line 106 that calls int(raw_player_id) with a strict isinstance check for int type (after the existing bool rejection check). Instead of attempting conversion, check if raw_player_id is strictly an instance of int, and if not, log a warning and return early, matching the pattern already established for the bool rejection check. This enforces the wire contract requirement that player_id must be an actual integer value, not a coercible value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@server/avatar_change_queue_service.py`:
- Around line 96-107: The code currently coerces raw_player_id to an integer
using int(raw_player_id), which allows malformed payloads like floats (1.9) or
strings ("1") to silently pass through. Replace the try-except block starting at
line 106 that calls int(raw_player_id) with a strict isinstance check for int
type (after the existing bool rejection check). Instead of attempting
conversion, check if raw_player_id is strictly an instance of int, and if not,
log a warning and return early, matching the pattern already established for the
bool rejection check. This enforces the wire contract requirement that player_id
must be an actual integer value, not a coercible value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b86c6be3-ef97-4e84-9c14-dd2d08e1f116
📒 Files selected for processing (3)
server/avatar_change_queue_service.pyserver/player_service.pytests/unit_tests/test_avatar_change_queue_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server/player_service.py
Brutus5000
force-pushed
the
feat/login-avatar-id-sync
branch
2 times, most recently
from
9a775de to
f23b72c
Compare
Adds an info-level log line whenever a player's avatar changes, covering both entry points: - via client connection (command_avatar select action) - via the RabbitMQ refresh consumer (refresh_player_avatar) Both log the player id and the resulting avatar URL (or None if cleared) so on-call can correlate which path triggered any unexpected change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brutus5000
force-pushed
the
feat/login-avatar-id-sync
branch
from
f23b72c to
fe3c002
Compare
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
Two related pieces of work to support shifting avatar handling to the API while keeping the lobby fully functional during the transition:
1. Read & write
login.avatar_idalongside legacyavatars.selectedThe DB now has
login.avatar_idas the authoritative single-avatar FK (added in faf/db#331, released as v143). Until all writers move over, both columns must stay consistent.avatar_idto theloginTable definition (server/db/models.py).PlayerService.fetch_player_datajoins through theavatarsgrant table onlogin.avatar_id(with ownership check), with a fallback to the legacyselected = 1row. A staleavatar_idpointing at a no-longer-granted avatar resolves to no row.command_avatar(select action) now also writeslogin.avatar_idwhenever it touchesavatars.selected, so the lobby never produces drift between the two columns going forward.FAF_DB_VERSIONtov143in CI andcompose.yaml.2. New
AvatarChangeQueueServiceconsumerWhen the API changes a player's avatar (e.g. via the website), the lobby has no idea — its in-memory
Player.avatarstays stale until the player reconnects. New consumer reacts to a fresh routing key, re-reads the DB, and marks the player dirty so the existingBroadcastServiceemits the usualplayer_infoto all connected clients.Wire contract for publishers (API side, separate PR):
MQ_EXCHANGE_NAME(faf-lobby)success.player_avatar.update— past-tense event; fan-out friendly so future services can subscribe.{"player_id": <int>, "avatar_id": <int|null>}—avatar_idis included so downstream subscribers don't need a DB roundtrip; the lobby itself re-reads from DB to enforce ownership and pick up url/tooltip.Implementation:
PlayerService.refresh_player_avatar(player_id)public method + extracted_fetch_player_avatarhelper sharing the ownership-checked join withfetch_player_data.AvatarChangeQueueServicefollows theClientMessageQueueServicepattern; auto-registered viaService.__init_subclass__.Why
End state we're working toward: API is the single writer for selected avatar; lobby reacts to API-driven changes and broadcasts via existing machinery. This PR is the lobby half — it keeps reading/writing the legacy column today, and it'll listen for API-driven changes once they start arriving.
Test plan
login.avatar_idset → fetch returns that avatar's url/tooltip.login.avatar_idNULL but a legacyavatars.selected = 1row → fetch returns the legacy avatar (backwards compatibility).login.avatar_idpointing at an avatar no longer granted inavatars→ resolves to no avatar (ownership enforced).command_avatar { action: "select", avatar: <url> }→ bothavatars.selected = 1ANDlogin.avatar_idupdated to that avatar.command_avatar { action: "select", avatar: null }→avatars.selectedcleared ANDlogin.avatar_idset to NULL.success.player_avatar.updatefor a connected player → avatar refreshed,player_infobroadcast emitted on the nextBroadcastServicetick.player_id, non-intplayer_id) → logged and acked.Out of scope / follow-ups
success.player_avatar.update— separate PR onfaf-java-api.command_avatar— wait until the API is the sole writer and legacy clients have migrated.🤖 Generated with Claude Code
Summary by CodeRabbit