Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Oct 6, 2022

Conversation

jedso
Copy link
Contributor

@jedso jedso commented Sep 26, 2022

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
Copy link
Collaborator

rtldg commented Sep 29, 2022

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
Copy link
Collaborator

rtldg commented Oct 6, 2022

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

@rtldg rtldg merged commit 4eb3182 into shavitush:master Oct 6, 2022
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.

[Bug] playertimes table has FK constraint in v2.5.5a -> v3.0.8, but none >=v3.1.0 (with no migration)
2 participants