Rochet2
commented
3 years ago The PR looks correct, but the auto-update system is problematic.
wouldn't it (at least for semantical correctness) be necessary to also change the date in the filename?
The custom folder for updates is a nice idea, but it breaks on fresh core if the DB structure has changed from the last dump of the entire DB, as you just noted. If the file name is updated, then anyone updating their core now (through git pull for example) will get their core automatically run the "new file" even if they already have the changes from the "old file" (new and old, because its the same file, but name is different). Changing the name thus breaks the update of any old core. Actually, changing the contents also causes the system to rerun the file, so old users would be screwed still.
Overall, I think that the custom SQL files that use the auto-update system need to be written differently. Or the system needs some overhaul. They should probably check the DB version and potentially old updates and then apply if necessary. Maybe for example:
- check if the file was already run
- if not, continue
- if was, stop
- check if the DB version is of a breaking change (has to be done for first and last breaking versions)
- if cache_id < breaking, continue
- if cache_id >= breaking, stop
When a new breaking version is released, the SQL could be updated and its skipped automatically for old users. The system wouldn't be perfect, as when the update SQL is created by TC, it will break the update until a new TDB release is made basically. For finer tuning, the SQLs could check if a specific breaking update was already run or not.
Executing migrations out of order isn't really a nice experience. A potential solution is to just not use the auto-updater and instead have the files back in the custom script folder where users have to go and run them manually.
Is it TDB > updates(FIFO) > custom (FIFO) ?
As far as I know, it's TDB > updates (by filename FIFO). So the dates do matter and custom updates can be put in between regular updates. But the TDB bundle screws things up for the custom updates as they are not included in the bundle and thus need extra care.
I don't know, why it fails in the CI
TC changed their command handling code but left a deprecation warning for the old API that still works. CI builds with warnings in place and the code uses the old API so the warning causes a fatal error in CI. Basically, the code should work but needs to be updated before the old API is removed from TC. Fixing this now actually. Need to think and fiddle a bit with the SQL updates though.
I stumbled on a small sql error while integrating this project into a fresh core. It seems, that there has been a change in Trinitycore database structure, the command table doesn't have field permission anymore, therefore it fails while trying to insert into it. I couldn't really locate, on which core rev this change took place.
The herby proposed changes should fix this issue. I didn't check other branches of the project, as I'm only using 3.3.5 for now.
ATTENTION required: this change only "hot/hack"-fixes the problem at hand. There is still a sql file in sql/custom/auth which inserts something related to these rbacs into the auth database. As I don't understand the rbac system completely, I can't really tell, if those DB changes are obsolete as well or still very much needed.
Just a (maybe insignificant) question: If a change to a "date_signed" sql file is made, wouldn't it (at least for semantical correctness) be necessary to also change the date in the filename? I don't really understand in which Order the SQL auto-Updater is processing all the sql files. Is it TDB > updates(FIFO) > custom (FIFO) ?
Changes proposed:
Branch 3.3.5 "Used Cor Rev. d665c68cfcf1"
Target branch(es): 335/6x
Issues addressed: Fixes #
Tests performed: it builds on my debian system(I don't know, why it fails in the CI, its just a sql fix, there should be no change in core-build status), sql file is processed without failure, functionality of commands tested ingame.
Known issues and TODO list: