Re-enable namelayer group merging - #25
Open
grepsedawk wants to merge 2 commits into
Open
Conversation
namelayer had no working tests and a few latent bugs, which made it risky to touch. This change fixes the bugs behind a new safety net and clears the way for further cleanup, without altering the public API that the ~15 dependent plugins rely on. Seal the last facade leak: citadel looked up a player's groups through namelayer's internal DAO. Route it through the public GroupManager.getAllGroupNames so no plugin reaches past the facade. Delete the orphaned group-link command and GUI classes (LinkGroups, UnlinkGroups, LinkingGUI). They were never registered and only added noise. The group-merge code is left untouched pending a decision on whether to revive or remove it. Add a MockBukkit/JUnit/Mockito test harness, modeled on civmodcore, plus characterization tests that pin current behavior as a regression net for the fixes below. Fix three confirmed bugs: a cancelled GroupCreateEvent still inserted the group; getDefaultGroup read a ResultSet without next(); and isPassword threw on a group that has no password. Collapse the per-role N+1 member load in the Group constructor into a single query, and make cache invalidation O(1) instead of a linear scan.
Group merging had been disabled for years as "semi broken." It relied on an opaque MySQL stored procedure and a shadow-id schema in which one group name could own many faction_id rows, its command predated the ACF migration, and a guard against merging invalid groups had been defused into a no-op. This reworks the feature into plain, tested Java and turns it back on. Collapse the data model to exactly one faction_id per group name (migration 16, enforced by a UNIQUE constraint) and drop the mergeintogroup stored procedure. Reimplement the merge as a single JDBC transaction: members of the merged-in group move to the destination, the destination's role wins on any overlapping member, subgroup links are redirected, and the merged-in group's records are removed. Restore the dropped validity guard. Port the MergeGroups command to ACF and register it, and re-wire the merge entry point in the group GUI. Add a real-database test harness using Testcontainers MariaDB that runs the migrations against a live server, and cover the merge, the shadow-id collapse, and core DAO behavior.
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
Re-enables namelayer group merging, which had been disabled for years as "semi broken." It relied on an opaque MySQL stored procedure and a shadow-id schema in which one group name could own many
faction_idrows, its command predated the ACF migration, and a guard against merging invalid groups had been defused into a no-op. This reworks the feature into plain, tested Java and turns it back on.faction_idper group name (migration 16, enforced by aUNIQUEconstraint) and drop themergeintogroupstored procedure.GroupManager.mergeGroup.MergeGroupscommand to ACF and register it, and re-wire the merge action in the group GUI.Testing
Adds a real-database test harness using Testcontainers MariaDB that runs the migrations against a live MariaDB server, plus tests covering the merge (including destination-role-wins on overlap), the shadow-id collapse on multi-id data, and core DAO behavior.
Full suite: 39 tests, 0 failures. Dependents (citadel, bastion, jukealert, civchat2) compile.