shavitush / bhoptimer

A bunnyhop timer plugin for Counter-Strike: Source, Counter-Strike: Global Offensive and Team Fortress 2.
https://timer.shav.it
GNU General Public License v3.0
228 stars 93 forks source link

Add migration to update or add auth FK constraint in playertimes #1176

Closed jedso closed 1 year ago

jedso commented 1 year ago

Fixes #1175. I decided to go with ON UPDATE RESTRICT ON DELETE RESTRICT referential actions instead of the CASCADE actions used by the constraint in DBs created <= v3.0.8. RESTRICT would have stopped the REPLACE SQLite query from working/cascading to delete all of the user's playertimes. It does mean that all child playertimes have to be deleted before the user table row can be, but this is already how DeleteRestOfUser works so should be fine.

MySQL supports ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY. Unfortunately, SQLite does not, which means creating a temporary table to store existing playertimes data, dropping playertimes and re-creating playertimes with the correct FK constraint.

For SQLite I initially tried PRAGMA foreign_key_list(playertimes) to test for the existence of FKs on playertimes, but the code was starting to get ugly so I switched to using sqlite_master and checking the SQL used to create the table. This meant using StrContains to check for <= v3.0.8 and >= v3.1.0 DBs which isn't exactly elegant (but it works).

I tested this PR with a MySQL DB created at v3.0.8, v3.3.2 and a fresh DB with latest commits. Same for SQLite (with playertimes having >50k rows). Some more testing would definitely be good but should be working as-is.

Only concern I had was that this migration starts immediately after the first 9000 rows of Migration_DeprecateExactTimeInt, and for SQLite this means dropping playertimes and replacing the table without the exact_time_int column. Should be fine though since the id, exact_time_int combinations are still stored in DBResultSet results. If SQL_Migration_DeprecateExactTimeInt_Query fails for whatever reason though, it could mean loss of exact_time_int and the ability to redo Migration_DeprecateExactTimeInt. Probably should encourage SQLite users to backup before this migration.

rtldg commented 1 year ago

Only concern I had was that this migration starts immediately after the first 9000 rows of Migration_DeprecateExactTimeInt

I'm seeing what I can do to fix this

rtldg commented 1 year ago

Added a commit to put missing user auths into the users table (which my db had which made it fail when adding the foreign key). Seems to work though so I'll merge this