Add Player.currentAvatar FK and publish avatar update events#1146
Merged
Conversation
Expose login.avatar_id on the Player entity as `currentAvatar` so the API becomes the writer for a player's selected avatar. On update, a `success.player_avatar.update` message is published on the `faf-lobby` exchange so the lobby server can refresh its in-memory state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds player avatar validation and post-commit update publishing, deprecates ChangesPlayer avatar selection flow
Branch image push script
Sequence Diagram(s)sequenceDiagram
participant ElideRuntime
participant PlayerAvatarValidationHook
participant AvatarAssignmentRepository
ElideRuntime->>PlayerAvatarValidationHook: execute(UPDATE, PRECOMMIT, player)
alt player.getCurrentAvatar() is null
PlayerAvatarValidationHook->>ElideRuntime: allow update
else avatar is present
PlayerAvatarValidationHook->>AvatarAssignmentRepository: findOneByAvatarIdAndPlayerId(avatarId, playerId)
alt assignment exists
AvatarAssignmentRepository-->>PlayerAvatarValidationHook: Optional<AvatarAssignment>
PlayerAvatarValidationHook->>ElideRuntime: allow update
else assignment missing
AvatarAssignmentRepository-->>PlayerAvatarValidationHook: Optional.empty()
PlayerAvatarValidationHook-->>ElideRuntime: ApiException(AVATAR_NOT_ASSIGNED)
end
end
sequenceDiagram
participant ElideRuntime
participant PlayerAvatarUpdateHook
participant RabbitTemplate
ElideRuntime->>PlayerAvatarUpdateHook: execute(UPDATE, POSTCOMMIT, player)
PlayerAvatarUpdateHook->>PlayerAvatarUpdateHook: read player.getCurrentAvatar()
PlayerAvatarUpdateHook->>PlayerAvatarUpdateHook: build payload {player_id, avatar_id}
PlayerAvatarUpdateHook->>RabbitTemplate: convertAndSend(EXCHANGE_FAF_LOBBY, success.player_avatar.update, payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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
🤖 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 `@src/main/java/com/faforever/api/data/domain/Player.java`:
- Around line 41-51: The getCurrentAvatar() method in the Player class has
permission checks but lacks validation that the avatar being assigned actually
belongs to the player through AvatarAssignment. Add a PRECOMMIT phase hook
(similar to the existing PlayerAvatarUpdateHook which is in POSTCOMMIT) or use a
custom validator that leverages
AvatarAssignmentRepository.findOneByAvatarIdAndPlayerId to verify the avatar is
assigned to the player before allowing the update to proceed. This validation
should throw an appropriate exception if the avatar is not assigned to the
player.
🪄 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: 08ae3f29-710e-4b2f-95d9-8e92b5cb6298
📒 Files selected for processing (2)
src/main/java/com/faforever/api/data/domain/Player.javasrc/main/java/com/faforever/api/data/hook/PlayerAvatarUpdateHook.java
The Player.currentAvatar mapping reads login.avatar_id, added in faf-db v143. Bump the integration-test and compose migration image so the column exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The selected avatar is now tracked via login.avatar_id (Player.currentAvatar). The legacy `selected` flag remains for backwards compatibility with older clients and is slated for removal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror the audit log that existed on the deprecated AvatarAssignment.selected flag so avatar selection changes via the new login.avatar_id path are recorded too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Builds the current branch's jar and pushes a linux/amd64 Docker image tagged after the branch name, for deploying test builds. Refuses to run on develop/main/master. Mirrors the script in the lobby server repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
IsEntityOwner only ensures the caller edits their own Player row, not that the avatar is granted to them. Add a PRECOMMIT LifeCycleHook that verifies an AvatarAssignment exists for the player before allowing the currentAvatar update, rejecting unowned avatars with AVATAR_NOT_ASSIGNED. Clearing the avatar (null) remains allowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@push-branch-image.sh`:
- Around line 29-32: The tag sanitization for the Docker image does not fully
validate Docker tag requirements. The sed substitution replaces invalid
characters but can still produce invalid tags if the first character becomes a
hyphen or dot (Docker requires first character to be alphanumeric or
underscore), or if the resulting tag exceeds 128 characters. After the sed
substitution that creates the tag variable, add logic to remove any leading
hyphens or dots from the tag and truncate the tag to a maximum of 128 characters
to ensure complete Docker compliance before passing it to the docker buildx
command.
In
`@src/test/java/com/faforever/api/data/hook/PlayerAvatarValidationHookTest.java`:
- Line 21: The import statement for assertThrows is using the JUnit 4 version
from org.junit.Assert, but the test class PlayerAvatarValidationHookTest is
using JUnit Jupiter annotations (`@Test`, `@BeforeEach`, `@ExtendWith`). Change the
static import to use org.junit.jupiter.api.Assertions.assertThrows instead of
org.junit.Assert.assertThrows to align with the Jupiter test framework being
used throughout the test class.
🪄 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: a36f9019-8128-42af-b983-8127394cec2b
📒 Files selected for processing (5)
push-branch-image.shsrc/main/java/com/faforever/api/data/domain/Player.javasrc/main/java/com/faforever/api/data/hook/PlayerAvatarValidationHook.javasrc/main/java/com/faforever/api/error/ErrorCode.javasrc/test/java/com/faforever/api/data/hook/PlayerAvatarValidationHookTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/faforever/api/data/domain/Player.java
Comment on lines
+29
to
+32
| # Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}. | ||
| # Replace anything else (most commonly '/' from branch prefixes) with '-'. | ||
| tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g') | ||
|
|
There was a problem hiding this comment.
Ensure sanitized tag is always Docker-valid before invoking buildx.
Current sanitization replaces invalid chars, but it can still yield an invalid first character (./-) or exceed 128 chars, causing avoidable late failures in docker buildx.
Suggested patch
-# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
-# Replace anything else (most commonly '/' from branch prefixes) with '-'.
-tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g')
+# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
+# Replace invalid chars, normalize leading '.'/'-', and cap length.
+tag=$(
+ printf '%s' "$branch" \
+ | sed 's/[^A-Za-z0-9_.-]/-/g' \
+ | sed 's/^[.-][.-]*/_/' \
+ | cut -c1-128
+)
+
+if [ -z "$tag" ]; then
+ echo "Refusing to push: sanitized tag is empty for branch '$branch'." >&2
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}. | |
| # Replace anything else (most commonly '/' from branch prefixes) with '-'. | |
| tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g') | |
| # Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}. | |
| # Replace invalid chars, normalize leading '.'/'-', and cap length. | |
| tag=$( | |
| printf '%s' "$branch" \ | |
| | sed 's/[^A-Za-z0-9_.-]/-/g' \ | |
| | sed 's/^[.-][.-]*/_/' \ | |
| | cut -c1-128 | |
| ) | |
| if [ -z "$tag" ]; then | |
| echo "Refusing to push: sanitized tag is empty for branch '$branch'." >&2 | |
| exit 1 | |
| fi |
🤖 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 `@push-branch-image.sh` around lines 29 - 32, The tag sanitization for the
Docker image does not fully validate Docker tag requirements. The sed
substitution replaces invalid characters but can still produce invalid tags if
the first character becomes a hyphen or dot (Docker requires first character to
be alphanumeric or underscore), or if the resulting tag exceeds 128 characters.
After the sed substitution that creates the tag variable, add logic to remove
any leading hyphens or dots from the tag and truncate the tag to a maximum of
128 characters to ensure complete Docker compliance before passing it to the
docker buildx command.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-id-and-rabbit-event # Conflicts: # compose.yaml # src/inttest/java/com/faforever/api/config/MainDbTestContainers.java
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.
What
Exposes the
login.avatar_idFK column (added in faf/db v143) on thePlayerentity as acurrentAvatarrelationship, making the API a writer for a player's currently selected avatar.When a player's
currentAvataris updated, asuccess.player_avatar.updatemessage is published to thefaf-lobbytopic exchange so the lobby server can refresh its in-memory state and broadcast aplayer_infoto connected clients.Changes
Player.java— newcurrentAvatar@ManyToOnemapped to theavatar_idcolumn, exposed via Elide withIsEntityOwnerupdate permission and a POSTCOMMITLifeCycleHook.PlayerAvatarUpdateHook.java(new) — publishes{"player_id": <int>, "avatar_id": <int|null>}to exchangefaf-lobbywith routing keysuccess.player_avatar.update.Wire contract
faf-lobby(topic)success.player_avatar.update{"player_id": <int>, "avatar_id": <int|null>}The lobby-side consumer is implemented separately (FAForever/server PR #1095).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Deprecations