Skip to content

Conversation

@Gatsik
Copy link

@Gatsik Gatsik commented Nov 23, 2025

Summary by CodeRabbit

  • Chores
    • Reordered faction enumeration constants, affecting the mapping between faction values and their integer representations used in database conversions.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 23, 2025

Walkthrough

The Faction enum constants are reordered, changing the sequence from AEON, CYBRAN, UEF, SERAPHIM, NOMAD, UNKNOWN to UEF, AEON, CYBRAN, SERAPHIM, NOMAD, UNKNOWN. This reordering affects ordinal-based value mappings in conversion methods.

Changes

Cohort / File(s) Summary
Faction enum constant reordering
src/main/java/com/faforever/api/data/domain/Faction.java
Enum constant order updated: UEF and AEON positions swapped, changing ordinal indices for all constants. Impacts toFaValue() ordinal-based mappings and fromFaValue() reverse lookups.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Verify that ordinal-based mappings in toFaValue() and fromFaValue() methods are intentional and correctly updated
  • Check all database queries and persisted values that depend on Faction ordinal indices for potential breakage
  • Confirm whether a database migration is required to remap stored Faction values
  • Audit all usages of Faction enum across the codebase to ensure consistency with the new ordinal positions

Poem

🐰 Factions shuffle in their lineup,
UEF and AEON swap their sign-up,
Ordinals dance in sequences new,
A rabbit hopes mappings stay true!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix factions order' directly and clearly summarizes the main change: reordering faction enum constants to correct their sequence.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/java/com/faforever/api/data/domain/Faction.java (1)

12-12: Enum reordering changes persisted/JSON mapping; verify external expectations and consider decoupling from ordinal()

This reorder will change the mapping for all usages of fromFaValue(int) / toFaValue() and for the JPA FactionConverter (database column values). If the old mapping was wrong and FA’s canonical values are 1=UEF, 2=AEON, 3=CYBRAN, 4=SERAPHIM, 5=NOMAD, then this is the right fix—but it’s still a breaking change for any existing stored values and external clients expecting the previous behavior.

I’d suggest:

  • Confirm that all producers/consumers of the numeric faction value (DB data, other services, game server) agree on the new mapping {1→UEF, 2→AEON, 3→CYBRAN, 4→SERAPHIM, 5→NOMAD} and, if needed, plan a data migration or deployment choreography.
  • Add explicit tests that lock in the intended mapping, e.g.:
    • assertEquals(UEF, fromFaValue(1)); …
    • assertEquals(1, UEF.toFaValue()); …
  • Medium‑term, consider decoupling from ordinal() by adding an explicit FA code to the enum (e.g., UEF(1, "uef"), AEON(2, "aeon"), …) and implementing fromFaValue via a lookup table rather than values()[value - 1]. That would make future enum insertions/reorders safer and remove the “order is crucial” footgun.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d7b133 and 6e3fcc0.

📒 Files selected for processing (1)
  • src/main/java/com/faforever/api/data/domain/Faction.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant