Skip to content

Conversation

@PJB3005
Copy link
Member

@PJB3005 PJB3005 commented Jan 18, 2026

About the PR

This refactors the way bans are stored in the database, enabling bans to have multiple players/HWIDs/IPs/rounds/roles per ban. There's also significant code cleanup along the way.

This PR does not update most of the game/tooling to expose this new functionality (eg banning on multiple round IDs), that can be done in separate PRs once the main schema refactor is over with. The one exception is that role bans now make one ban with multiple roles.

Why / Balance

Our admins are weeping because role bans weren't normalized in the DB properly and needed to be separate records per job.
Having multiple players/HWIDs/IPs per ban record enables future sticky ban tracking systems sanely.
Multiple rounds per ban is because I was thinking "well if I'm gonna do a massive DB change might as well be proactive."

Technical details

The DB model has been significantly refactored:

  • The distinction between role and server bans has been removed from the schema, they are now distinguished by a type column on the single ban table.
  • Information about players/HWIDs/IPs/rounds/roles is now stored normalized in separate tables.
  • The removal of the distinction between the ban types has been plumbed through much of the codebase, removing a significant amount of code copy paste. Yuck.
  • SQL migration (for both SQLite and PostgreSQL) should be able to automatically migrate the database to the new format. It does its best to merge denormalized role ban records into single bans with multiple roles.
  • Oh yeah I fixed the fact that the PostgreSQL index for matching addresses on bans doesn't work. It's a proper GiST index now. I doubt that matters for database performance though.
  • Plumbed through ProtoId<> more thoroughly in some role ban code.
  • Removed the optimization where DB ban notification aren't checked on the server that created the ban, as now round IDs are no longer indicative of the game server placing the ban.
  • Role ban IDs (previously Job:Captain) are now more cleanly typed to avoid confusion.
  • SS14.Admin PR here: Support new DB schema SS14.Admin#82

Media

firefox_TxhK7qbvj7

Requirements

Breaking changes

  • The database schema for bans (both kinds) has been completely refactored to unify server & role bans and allow multiple players/HWIDs/IPs/rounds/roles per ban record.
    • C# types and methods have all been modified to reflect this change. In many cases, things have merged (e.g. ServerBanDef & ServerRoleBanDef -> BanDef).
    • Separate C# functions for "server bans" and "role bans" have all been merged into just "ban" where applicable (e.g. GetBanAsync().
    • This is a large change to the database schema that must be done at the same time for all servers sharing a database. Applying the migration while some game servers have not updated yet will cause a lot of issues.
  • Plumbed through ProtoId<> more thoroughly in some role ban code.
  • Role ban IDs (previously Job:Captain) are now more cleanly typed as BanRoleDef in C# to avoid confusion.

Changelog

🆑
ADMIN:

  • tweak: Role bans for multiple jobs are now stored in a single ban entry, rather than having to create a dozen individual ones.

Supports all the new functionality (multiple players/addresses/hwids/roles/rounds per ban).
The server ID check is no longer done as admins may want to place bans spanning multiple rounds irrelevant of the source server.
@PJB3005 PJB3005 added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: Refactor Type: Refactor of notable amount of codebase A: Admin Tooling Area: Admin tooling and moderation. A: Database Area: Database code labels Jan 18, 2026
@PJBot PJBot added S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. Changes: UI Changes: Might require knowledge of UI design or code. size/XL Denotes a PR that changes 5000+ lines. labels Jan 18, 2026
@PJB3005
Copy link
Member Author

PJB3005 commented Jan 18, 2026

Marking as draft since I still want to do more testing. Also I need to rebase this onto stable so we can do the server update.

@PJB3005 PJB3005 changed the base branch from master to stable January 18, 2026 22:37
@PJBot PJBot added Changes: Sprites Changes: Might require knowledge of spriting or visual design. Changes: Map Changes: Might require knowledge of mapping. Changes: Audio Changes: Might require knowledge of audio. labels Jan 19, 2026
@PJB3005 PJB3005 force-pushed the 26-01-16-ban-db-refactor branch from f637bb8 to 390d7f2 Compare January 19, 2026 20:11
@PJBot PJBot removed Changes: Map Changes: Might require knowledge of mapping. Changes: Audio Changes: Might require knowledge of audio. labels Jan 19, 2026
Not relevant, reduces query performance
Would blow up handling null player records. Not a new bug introduced by the refactor, but I ran into it.
@PJB3005 PJB3005 marked this pull request as ready for review January 20, 2026 22:21
Copy link
Member

@VasilisThePikachu VasilisThePikachu left a comment

Choose a reason for hiding this comment

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

I will do a review later however quick thing I noticed

Please update the deletion and data dump scripts to the new schema.

@VasilisThePikachu VasilisThePikachu self-requested a review January 20, 2026 22:31
@PJB3005
Copy link
Member Author

PJB3005 commented Jan 20, 2026

Please update the deletion and data dump scripts to the new schema.

oh yeah fair enough

Copy link
Contributor

@juliangiebel juliangiebel left a comment

Choose a reason for hiding this comment

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

Banning multiple departments with the same roles causes the a database exception because the unique index IX_ban_role_role_type_role_id_ban_id gets violated.

An easy way to replicate this is banning someone from command and security

stacktrace.txt

@PJBot PJBot added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Jan 21, 2026
@juliangiebel
Copy link
Contributor

The schema looks fine otherwise. The queries could be improved to cut down on roundtrips (because of split queries. The ban table really isn't large enough to require split queries imo) and joins but that is really out of scope for this PR imo

Most notably happened with role bans, as multiple departments may include the same role.
@PJB3005 PJB3005 requested a review from juliangiebel January 21, 2026 19:47
@PJBot PJBot added S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Jan 21, 2026
@VasilisThePikachu VasilisThePikachu added the S: DO NOT MERGE Status: Open item that should NOT be merged. DNM. Allows test to run unlike draft. label Jan 22, 2026
@VasilisThePikachu
Copy link
Member

Merge before maintenance starts so that vulture does not start with it before we are ready for the db migration.

@PJB3005 PJB3005 merged commit 4e42843 into space-wizards:stable Jan 23, 2026
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 Needs Review to ✅ Done in NextGen WizDen Adminning Jan 23, 2026
PJB3005 added a commit that referenced this pull request Jan 23, 2026
Ban database refactor (#42495)

* Ban DB refactor seems to work at a basic level for PostgreSQL

* New ban creation API

Supports all the new functionality (multiple players/addresses/hwids/roles/rounds per ban).

* Make the migration irreversible

* Re-implement ban notifications

The server ID check is no longer done as admins may want to place bans spanning multiple rounds irrelevant of the source server.

* Fix some split query warnings

* Implement migration on SQLite

* More comments

* Remove required from ban reason

SS14.Admin changes would like this

* More missing AsSplitQuery() calls

* Fix missing ban type filter

* Fix old CreateServerBan API with permanent time

* Fix department and role ban commands with permanent time

* Re-add banhits navigation property

Dropped this on accident, SS14.Admin needs it.

* More ban API fixes.

* Don't fetch ban exemption info for role bans

Not relevant, reduces query performance

* Regenerate migrations

* Fix adminnotes command for players that never connected

Would blow up handling null player records. Not a new bug introduced by the refactor, but I ran into it.

* Great shame... I accidentally committed submodule update...

* Update GDPR scripts

* Fix sandbox violation

* Fix bans with duplicate info causing DB exceptions

Most notably happened with role bans, as multiple departments may include the same role.
PJB3005 added a commit that referenced this pull request Jan 23, 2026
Ban database refactor (#42495)

* Ban DB refactor seems to work at a basic level for PostgreSQL

* New ban creation API

Supports all the new functionality (multiple players/addresses/hwids/roles/rounds per ban).

* Make the migration irreversible

* Re-implement ban notifications

The server ID check is no longer done as admins may want to place bans spanning multiple rounds irrelevant of the source server.

* Fix some split query warnings

* Implement migration on SQLite

* More comments

* Remove required from ban reason

SS14.Admin changes would like this

* More missing AsSplitQuery() calls

* Fix missing ban type filter

* Fix old CreateServerBan API with permanent time

* Fix department and role ban commands with permanent time

* Re-add banhits navigation property

Dropped this on accident, SS14.Admin needs it.

* More ban API fixes.

* Don't fetch ban exemption info for role bans

Not relevant, reduces query performance

* Regenerate migrations

* Fix adminnotes command for players that never connected

Would blow up handling null player records. Not a new bug introduced by the refactor, but I ran into it.

* Great shame... I accidentally committed submodule update...

* Update GDPR scripts

* Fix sandbox violation

* Fix bans with duplicate info causing DB exceptions

Most notably happened with role bans, as multiple departments may include the same role.
ThatGuyUSA pushed a commit to ThatGuyUSA/space-station-14 that referenced this pull request Feb 3, 2026
Ban database refactor (space-wizards#42495)

* Ban DB refactor seems to work at a basic level for PostgreSQL

* New ban creation API

Supports all the new functionality (multiple players/addresses/hwids/roles/rounds per ban).

* Make the migration irreversible

* Re-implement ban notifications

The server ID check is no longer done as admins may want to place bans spanning multiple rounds irrelevant of the source server.

* Fix some split query warnings

* Implement migration on SQLite

* More comments

* Remove required from ban reason

SS14.Admin changes would like this

* More missing AsSplitQuery() calls

* Fix missing ban type filter

* Fix old CreateServerBan API with permanent time

* Fix department and role ban commands with permanent time

* Re-add banhits navigation property

Dropped this on accident, SS14.Admin needs it.

* More ban API fixes.

* Don't fetch ban exemption info for role bans

Not relevant, reduces query performance

* Regenerate migrations

* Fix adminnotes command for players that never connected

Would blow up handling null player records. Not a new bug introduced by the refactor, but I ran into it.

* Great shame... I accidentally committed submodule update...

* Update GDPR scripts

* Fix sandbox violation

* Fix bans with duplicate info causing DB exceptions

Most notably happened with role bans, as multiple departments may include the same role.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: Admin Tooling Area: Admin tooling and moderation. A: Database Area: Database code Changes: Sprites Changes: Might require knowledge of spriting or visual design. Changes: UI Changes: Might require knowledge of UI design or code. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: DO NOT MERGE Status: Open item that should NOT be merged. DNM. Allows test to run unlike draft. S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. size/XL Denotes a PR that changes 5000+ lines. T: Refactor Type: Refactor of notable amount of codebase

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants