Add Player.currentAvatar FK and publish avatar update events#1146
Add Player.currentAvatar FK and publish avatar update events#1146Brutus5000 wants to merge 7 commits into
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>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesPlayer Avatar Update and Validation
Sequence DiagramsequenceDiagram
participant Client
participant ElideRuntime
participant PlayerAvatarValidationHook
participant AvatarAssignmentRepository
participant PlayerAvatarUpdateHook
participant RabbitTemplate
Client->>ElideRuntime: PATCH /players/{id} (update currentAvatar)
ElideRuntime->>ElideRuntime: check UpdatePermission (IsEntityOwner)
ElideRuntime->>PlayerAvatarValidationHook: execute(UPDATE, PRECOMMIT, player)
PlayerAvatarValidationHook->>AvatarAssignmentRepository: findOneByAvatarIdAndPlayerId(avatarId, playerId)
AvatarAssignmentRepository->>PlayerAvatarValidationHook: Optional<AvatarAssignment>
alt assignment exists
PlayerAvatarValidationHook->>ElideRuntime: validation passes
else no assignment
PlayerAvatarValidationHook->>ElideRuntime: throw ApiException (AVATAR_NOT_ASSIGNED)
end
ElideRuntime->>ElideRuntime: persist avatar_id change
ElideRuntime->>PlayerAvatarUpdateHook: execute(UPDATE, POSTCOMMIT, player)
PlayerAvatarUpdateHook->>RabbitTemplate: convertAndSend(EXCHANGE_FAF_LOBBY, "success.player_avatar.update", {player_id, avatar_id})
RabbitTemplate->>Client: message published
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
🤖 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
| # 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.
| # 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>
1 participant
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
AvatarAssignment.isSelected()is deprecated in favor of tracking selection via the player’s current avatar.v143and added a script to push branch images.