Add SQLite migration to fix mapzones having NULL id for all rows#1177
Add SQLite migration to fix mapzones having NULL id for all rows#1177rtldg merged 6 commits intoshavitush:masterfrom
Conversation
|
Just had a thought that SQLite DBs created prior to this PR with an empty mapzones table will always return 0 for |
| } | ||
|
|
||
| char sQuery[256]; | ||
| FormatEx(sQuery, sizeof(sQuery), "SELECT EXISTS(SELECT 1 FROM `%smapzones` WHERE id IS NULL);", gS_SQLPrefix); |
There was a problem hiding this comment.
Do you think it would be fine to replace this with SELECT 1 FROM mapzones WHERE id IS NULL LIMIT 1;?
There was a problem hiding this comment.
Like the original query, this doesn't handle the scenario I mentioned above (empty mapzones table with the old table creation statement). So ideally you'd need to directly test id's type to definitively know if the migration is required or not.
You could theoretically replace the query using a PRAGMA function, but it is listed as experimental so might break in the future:
| FormatEx(sQuery, sizeof(sQuery), "SELECT EXISTS(SELECT 1 FROM `%smapzones` WHERE id IS NULL);", gS_SQLPrefix); | |
| FormatEx(sQuery, sizeof(sQuery), "SELECT 1 FROM pragma_table_info('%smapzones') WHERE name = 'id' AND type = 'INT AUTO_INCREMENT' LIMIT 1;", gS_SQLPrefix); |
This is the only way of accessing PRAGMA results with a SELECT statement (i.e. SELECT * FROM PRAGMA table_info(mapzones) WHERE ...; doesn't work). Then again, the docs do say that "specific pragma statements may be removed and others added in future releases of SQLite. There is no guarantee of backwards compatibility" so probably avoiding PRAGMA statements and functions is the best move long term.
Which means, as far as I can tell, the best way of testing id's type is to use SELECT sql FROM sqlite_master + StrContains as I did in #1176. Let me know if you agree with that approach and I'll commit it.
There was a problem hiding this comment.
Let me know if you agree with that approach and I'll commit it.
I didn't see this, sorry. Yeah that definitely seems to be a good/easy approach
There was a problem hiding this comment.
OK, done. Should be good now.
| LogError("Timer error! SQLiteMapzonesROWID migration failed. Reason: %s", error); | ||
| return; | ||
| } | ||
| else if (!results.FetchInt(0)) // No NULL ids |
There was a problem hiding this comment.
This needs results.FetchRow() to be called first.
In fact, since .FetchRow() returns a bool if a row was fetched, it should be fine to swap it to this:
| else if (!results.FetchInt(0)) // No NULL ids | |
| else if (!results.FetchRow()) // No NULL ids |
Edit: Actually, this suggestion is fine to commit as it if the other thing for SELECT 1 FROM mapzones WHERE id IS NULL LIMIT 1; is done.
Otherwise, it'd need if (!results.FetchRow() || !results.FetchInt(0)) or something
There was a problem hiding this comment.
Ah, yeah good catch. Made me wonder why I didn't get an error during testing, and it turns out that up until recently the SQLite driver did not enforce calling FetchRow() before accessing the first row. I'm still on SM 1.10.0.6545 which doesn't have the fix.
|
I'm not a fan of doing the |
|
I resolved the conflicts on the branch since I just merged the other fk pr. So note to self: this migration number is (now) 29. |
SQLite uses ROWID to uniquely identify each row in a table, and a column with type INTEGER PRIMARY KEY becomes an alias for that ROWID value. The CREATE TABLE statement for mapzones since the beginning (1.1b) has been
id INT AUTO_INCREMENT, but the column has to be of INTEGER type for it to act as a ROWID alias (not INT). Because id is therefore acting as a ordinary integer table column, all id rows in mapzones have been NULL in SQLite DBs since the plugin was released.This migration fixes that and gives each map zone a unique identifier for SQLite DBs.