Make map folderName an indexable column to fix slow map filters#1150
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 24 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 review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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 (8)
📝 WalkthroughWalkthroughMapVersion now stores ChangesMapVersion storage and upload flow
Player avatar update hook
Branch image push script
MinIO image refresh
Sequence Diagram(s)Player avatar updatesequenceDiagram
participant Elide
participant Player
participant PlayerAvatarUpdateHook
participant RabbitTemplate
participant RabbitMQ
Elide->>Player: update currentAvatar
Player->>PlayerAvatarUpdateHook: POSTCOMMIT execute(...)
PlayerAvatarUpdateHook->>PlayerAvatarUpdateHook: build player_id/avatar_id payload
PlayerAvatarUpdateHook->>RabbitTemplate: convertAndSend(EXCHANGE_FAF_LOBBY, success.player_avatar.update, payload)
RabbitTemplate->>RabbitMQ: publish message
Branch image pushsequenceDiagram
participant "push-branch-image.sh"
participant Git
participant Gradle
participant "Docker Buildx"
"push-branch-image.sh"->>Git: rev-parse current branch
"push-branch-image.sh"->>"push-branch-image.sh": validate ref and sanitize tag
alt SKIP_BUILD != 1
"push-branch-image.sh"->>Gradle: bootJar -x test
end
"push-branch-image.sh"->>"Docker Buildx": build --platform and --push
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
`/data/map` requests filtering on `versions.folderName` took a flat ~5s each. `folderName` was a transient @ComputedAttribute derived in-memory from `filename` in a @PostLoad enricher, so Elide could not push `versions.folderName == X` into SQL and instead loaded and enriched the whole map_version table on every request, filtering in memory. Map `folderName` is now a real, indexed column (DB migration faf-db V144), making the filter a sargable SQL predicate. `filename` becomes a DB-generated column (read-only in the entity), kept for backwards compatibility with faf-server, the replay server and existing API clients; its value (maps/<folder>.zip) and the derived download/thumbnail URLs are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
filename is a DB-generated column, but Bean Validation runs on pre-insert before MariaDB populates it, so @NotNull failed every map upload. The schema guarantees non-null, so the constraint is removed from the entity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brutus5000
force-pushed
the
feature/map-version-folder-name
branch
from
8c0354a to
919e98a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/faforever/api/data/domain/AvatarAssignment.java (1)
49-53: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't keep
selectedwritable ifPlayer.currentAvataris now authoritative.The deprecated field still allows owner updates, but the new audit/hook path hangs off
Player.currentAvatar. That creates two writable sources of truth: older clients can still PATCHavatarAssignment.selected, yetlogin.avatar_idand the RabbitMQ event will not change. Either synchronize this compatibility path intoPlayer.currentAvatar, or reject updates here.🤖 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/AvatarAssignment.java` around lines 49 - 53, The deprecated AvatarAssignment.isSelected path still allows owner updates through `@UpdatePermission` even though Player.currentAvatar is now the authoritative source of truth, so older clients can change avatarAssignment.selected without updating login.avatar_id or emitting the RabbitMQ event. Update the AvatarAssignment compatibility path to either delegate writes into the Player.currentAvatar flow (using the existing AvatarAssignment and Player entities) or reject PATCH/update attempts on selected entirely, so there is only one writable source of truth.
🧹 Nitpick comments (1)
src/test/java/com/faforever/api/map/MapServiceTest.java (1)
413-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep coverage for the generated
filenamecontract.This assertion now only checks the pre-persist
folderName, so it won't catch a mismatch between the Hibernate generated-column mapping and thev144schema. Please add a DB-backed test that saves and reloads aMapVersionand verifiesfilenamestill resolves tomaps/<folder>.zip.🤖 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/test/java/com/faforever/api/map/MapServiceTest.java` at line 413, The current assertion in MapServiceTest only checks the in-memory folderName value, so it does not validate the generated filename contract against the database mapping. Update the test around MapServiceTest and MapVersion to persist a MapVersion, reload it from the DB, and assert that the generated filename still resolves to maps/<folder>.zip, ensuring the Hibernate generated-column mapping matches the v144 schema.
🤖 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 `@compose.yaml`:
- Line 38: The MinIO service in the Compose setup is using a mutable image tag,
so pin it to a fixed release tag or digest instead of latest-release. Update the
image reference for the MinIO service in compose.yaml to a specific
RELEASE.YYYY-MM-DDTHH-MM-SSZ tag or immutable digest so the deployment stays
reproducible.
In `@push-branch-image.sh`:
- Around line 29-32: The branch-to-tag sanitization in push-branch-image.sh is
too lossy and can make different branch names collide on the same Docker tag.
Update the tag generation logic around the current branch sanitization so it
preserves uniqueness for distinct refs, using a deterministic scheme in the
push-branch-image.sh flow instead of collapsing every non-matching character to
a dash. Keep the Docker tag valid while ensuring branches like feature/foo and
feature-foo cannot overwrite each other’s image artifacts.
- Around line 29-32: The Docker tag normalization in push-branch-image.sh only
replaces invalid characters, but it still allows tags that start with a
disallowed character or exceed Docker’s 128-character limit. Update the tag
generation logic around the tag assignment so it also enforces the required
start-character rule and truncates overly long branch-derived tags before they
reach docker buildx build, while keeping the existing branch-to-tag sanitization
behavior.
In `@src/main/java/com/faforever/api/data/domain/MapVersion.java`:
- Around line 145-146: Make the folderName field immutable after insert because
it is used as the source for the generated filename and derived URLs. Update the
MapVersion entity mapping on the folder_name column so it cannot be changed
after creation, keeping the DB-to-filesystem contract intact; locate the change
in MapVersion where the folderName/@Column annotation is defined.
In `@src/main/java/com/faforever/api/data/domain/Player.java`:
- Around line 43-52: The current `Player.getCurrentAvatar` mapping only checks
`IsEntityOwner.EXPRESSION`, so it still allows writing any readable `Avatar` ID
into `avatar_id` without validating the player’s active `AvatarAssignment`s.
Update the write path for `currentAvatar` to enforce that the selected avatar
belongs to the player’s non-expired assignments, or change the association to
resolve through `AvatarAssignment` instead; use `getCurrentAvatar`,
`AvatarAssignment`, and `PlayerAvatarUpdateHook` as the key places to apply the
validation.
In `@src/main/java/com/faforever/api/data/hook/PlayerAvatarUpdateHook.java`:
- Around line 39-43: The PlayerAvatarUpdateHook publisher is sending the payload
through RabbitTemplate without a JSON message converter, so it can still be
serialized as a Java object instead of JSON. Update the RabbitTemplate path used
in PlayerAvatarUpdateHook to use the same JacksonJsonMessageConverter as the
consumers, either by wiring the converter onto RabbitTemplate or by centralizing
a shared message-converter bean/customizer, so convertAndSend publishes JSON
consistently.
---
Outside diff comments:
In `@src/main/java/com/faforever/api/data/domain/AvatarAssignment.java`:
- Around line 49-53: The deprecated AvatarAssignment.isSelected path still
allows owner updates through `@UpdatePermission` even though Player.currentAvatar
is now the authoritative source of truth, so older clients can change
avatarAssignment.selected without updating login.avatar_id or emitting the
RabbitMQ event. Update the AvatarAssignment compatibility path to either
delegate writes into the Player.currentAvatar flow (using the existing
AvatarAssignment and Player entities) or reject PATCH/update attempts on
selected entirely, so there is only one writable source of truth.
---
Nitpick comments:
In `@src/test/java/com/faforever/api/map/MapServiceTest.java`:
- Line 413: The current assertion in MapServiceTest only checks the in-memory
folderName value, so it does not validate the generated filename contract
against the database mapping. Update the test around MapServiceTest and
MapVersion to persist a MapVersion, reload it from the DB, and assert that the
generated filename still resolves to maps/<folder>.zip, ensuring the Hibernate
generated-column mapping matches the v144 schema.
🪄 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: 8cce4354-6a8f-4ee3-b114-83846acd914c
📒 Files selected for processing (12)
compose.yamlpush-branch-image.shsrc/inttest/java/com/faforever/api/config/MainDbTestContainers.javasrc/inttest/resources/sql/prepMapData.sqlsrc/inttest/resources/sql/prepMapVersion.sqlsrc/main/java/com/faforever/api/data/domain/AvatarAssignment.javasrc/main/java/com/faforever/api/data/domain/MapVersion.javasrc/main/java/com/faforever/api/data/domain/Player.javasrc/main/java/com/faforever/api/data/hook/PlayerAvatarUpdateHook.javasrc/main/java/com/faforever/api/data/listeners/MapVersionEnricher.javasrc/main/java/com/faforever/api/map/MapService.javasrc/test/java/com/faforever/api/map/MapServiceTest.java
folderName is the source for the DB-generated filename and the derived download/thumbnail URLs, and map versions are immutable, so it must not change after creation. Mark the folder_name column updatable = false. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Why
Every
/data/maprequest that filters onversions.folderNametakes a flat ~5s (e.g. the client's "find latest version by folder name" lookup, and game searches by map). Root cause:folderNamewas a@Transient @ComputedAttributederived in-memory fromfilenamein a@PostLoadenricher, so Elide cannot translateversions.folderName == Xinto SQL. It fell back to loading and enriching the entiremap_versiontable on every request and filtering in memory — constant cost regardless of which map is requested.What
MapVersion.folderNameis now a real@Column(name = "folder_name")(was transient/computed), soversions.folderName == Xbecomes a sargable, indexed SQL predicate.MapVersion.filenamebecomes a DB-generated column:@Generated(event = INSERT), read-only in the entity. It is kept (value unchanged:maps/<folder>.zip) purely for backwards compatibility — see below.MapService) now setsfolderNameinstead offilename; the enricher derives the download/thumbnail URLs fromfolderNamedirectly (identical output).v144.Requires faf-db PR
Depends on FAForever/db#332 (adds
map_version.folder_name, makesfilenameaVIRTUALgenerated column). This PR's integration tests pinfaforever/faf-db-migrations:v144, so that image must be released first.Backwards compatibility (verified, no changes needed in those repos)
filenameis retained as a generated column so all external readers keep seeing the identicalmaps/<folder>.zip:game_service.get_map(),ladder_service.fetch_map_pools(), coop lookups.get_game_stat_rowselectsmap_version.filename.filename; depends onfolderName/downloadUrl/thumbnails, all preserved. The attribute namefolderNameis unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit