Add Player.currentAvatar FK and publish avatar update events#1146
Add Player.currentAvatar FK and publish avatar update events#1146Brutus5000 wants to merge 2 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 54 minutes and 38 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 (2)
📝 WalkthroughWalkthroughAdds a ChangesPlayer Avatar Update Hook
Sequence DiagramsequenceDiagram
participant Client
participant ElideRuntime
participant PlayerAvatarUpdateHook
participant RabbitTemplate
Client->>ElideRuntime: PATCH /players/{id} (update currentAvatar)
ElideRuntime->>ElideRuntime: check UpdatePermission (IsEntityOwner)
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})
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
| @UpdatePermission(expression = IsEntityOwner.EXPRESSION) | ||
| @LifeCycleHookBinding( | ||
| operation = LifeCycleHookBinding.Operation.UPDATE, | ||
| phase = LifeCycleHookBinding.TransactionPhase.POSTCOMMIT, | ||
| hook = PlayerAvatarUpdateHook.class | ||
| ) | ||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "avatar_id") | ||
| public Avatar getCurrentAvatar() { | ||
| return currentAvatar; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for validation logic ensuring currentAvatar is an assigned avatar
# Look for any security checks or validators related to avatar assignment
rg -n -C3 'avatar.*assignment|assigned.*avatar' --type java -i
# Check if there's a database constraint on avatar_id
fd -e sql -e java | xargs rg -l 'avatar_id' | head -10 | xargs rg -n 'avatar_id' -C2
# Look for any Elide security checks on Avatar relationships
rg -n 'AvatarAssignment' --type java -C3 | head -60Repository: FAForever/faf-java-api
Length of output: 40604
🏁 Script executed:
# Check the full PlayerAvatarUpdateHook implementation
cat -n src/main/java/com/faforever/api/data/hook/PlayerAvatarUpdateHook.javaRepository: FAForever/faf-java-api
Length of output: 1998
🏁 Script executed:
# Search for any validators or security checks related to avatar updates on Player
rg -n 'class.*Validator|validateAvatar|checkAvatar' --type java
# Look for Elide hooks or checks on Player class
rg -n '@.*Hook|@.*Check' src/main/java/com/faforever/api/data/domain/Player.java -A 1 -B 1Repository: FAForever/faf-java-api
Length of output: 960
🏁 Script executed:
# Search for integration tests that update currentAvatar
rg -n 'currentAvatar|getCurrentAvatar' --type java -A 3 -B 1 | head -80Repository: FAForever/faf-java-api
Length of output: 1760
🏁 Script executed:
# Check if there are any tests that try to update currentAvatar with unassigned avatar
rg -n 'currentAvatar|setCurrentAvatar' --type java -B 3 -A 5 | head -120Repository: FAForever/faf-java-api
Length of output: 1763
🏁 Script executed:
# Look for any security checks or permission validators in the codebase
rg -n 'currentAvatar.*Permission|Permission.*currentAvatar' --type java -B 2 -A 2Repository: FAForever/faf-java-api
Length of output: 48
🏁 Script executed:
# Check if there's a custom setter or validation on Player.setCurrentAvatar
fd -e java -path '*/domain/*' | xargs rg -l 'class Player' | xargs cat -n | grep -A 150 'setCurrentAvatar\|getCurrentAvatar'Repository: FAForever/faf-java-api
Length of output: 15376
🏁 Script executed:
# Search for any tests that update currentAvatar
fd -e java -path '*inttest*' -o -path '*test*' | xargs rg -l 'currentAvatar\|setCurrentAvatar' 2>/dev/nullRepository: FAForever/faf-java-api
Length of output: 237
🏁 Script executed:
# Look for PRECOMMIT hooks or any validation hooks
rg -n 'TransactionPhase.PRECOMMIT|`@PrePersist`|`@PreUpdate`|validates.*avatar' --type javaRepository: FAForever/faf-java-api
Length of output: 156
🏁 Script executed:
# Check if AvatarAssignmentRepository methods are used anywhere for validation
rg -n 'findOneByAvatarIdAndPlayerId|findOneByAvatarAndPlayer' --type java -B 2 -A 2Repository: FAForever/faf-java-api
Length of output: 918
🏁 Script executed:
# Find where AvatarAssignmentRepository is injected and used
rg -n 'AvatarAssignmentRepository' --type java -A 10 | grep -v testRepository: FAForever/faf-java-api
Length of output: 908
🏁 Script executed:
# Look for any Player update/service logic that might validate avatars
fd -name '*Service.java' -path '*/player/*' -o -name '*PlayerService.java' | xargs cat 2>/dev/null | head -150Repository: FAForever/faf-java-api
Length of output: 297
🏁 Script executed:
# Check if there's any Elide custom check/hook for Player avatar updates
rg -n 'class.*Check' --type java | grep -i avatarRepository: FAForever/faf-java-api
Length of output: 202
🏁 Script executed:
# Find where AvatarAssignmentRepository is actually used (not just declared)
rg -n 'avatarAssignmentRepository\.' --type java -B 2 -A 3Repository: FAForever/faf-java-api
Length of output: 48
🏁 Script executed:
# Check WriteAvatarCheck implementation
cat -n src/main/java/com/faforever/api/security/elide/permission/WriteAvatarCheck.javaRepository: FAForever/faf-java-api
Length of output: 864
🏁 Script executed:
# Search for any endpoint or controller that handles Player updates
fd -name '*Controller.java' | xargs rg -l 'player\|Player' | head -3 | xargs cat | head -200Repository: FAForever/faf-java-api
Length of output: 297
🏁 Script executed:
# Search for any validators with custom logic
find src/main/java -name '*Validator.java' -type f | xargs grep -l avatarRepository: FAForever/faf-java-api
Length of output: 50
🏁 Script executed:
# Search for any aspect or interceptor that might validate
rg -n '`@Aspect`|`@Interceptor`' --type javaRepository: FAForever/faf-java-api
Length of output: 135
🏁 Script executed:
# Check if there are any database constraints on avatar_id
rg -n 'FOREIGN KEY|CONSTRAINT.*avatar' src/ 2>/dev/null | head -20Repository: FAForever/faf-java-api
Length of output: 48
Verify avatar assignment validation exists.
The IsEntityOwner permission ensures only the player can update their own currentAvatar, but it doesn't validate that the specified avatar is one actually assigned to them via AvatarAssignment. A player could potentially set any avatar ID. While AvatarAssignmentRepository provides methods to check avatar-player relationships (findOneByAvatarIdAndPlayerId), these are not used to validate the update. Consider adding validation in the update path (either as a Elide hook in PRECOMMIT phase or a custom validator) to ensure the avatar is assigned to the player.
🤖 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 `@src/main/java/com/faforever/api/data/domain/Player.java` around lines 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.
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>
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
New Features
Chores