Add db migration management infratructure

[mikiher]

This adds migration management infrastructure, based on the design and discussion here.

This is for initial review - please don't merge this yet. Currently missing:

• I have not e2e-tested this yet with actual migrations (it runs, but I only ran it with an empty migrations directetory)
• I have not modified backup yet to store and restore the migrations directory.

Other than that, I think this is pretty complete.

Please review carefully - I tried to unit test everything thoroughly, but of course I might have missed stuff.

Also, please review the readme and changlog files in server/migrations

[mikiher]

Plus the integration test is failing for some reason, I need to check why (seems to be something related to pkg).

Edit: I fixed the issues causing the integration test to to fail.

[mikiher]

I encountered a couple of issues while doing e2e-testing, so please hold on with reviewing for now. I'll keep you posted.

[mikiher]

After testing e2e, I made some changes to make it a bit more robust, mainly:

• Write version and maxVersion into a separate table migrationMeta, instead of using the settings table
• Always call MigrationManager init (even if database is new), so that new/modified migration scripts are written to config as soon as possible.
• Made sure Audiobookshelf server modules and dependencies were available to migration scripts.

I believe this is now ready for review.

[advplyr]

Is this being used?

[mikiher]

It's some internal dependency of umzug that pkg is not able to pack because of some package resolution issue. If I don't add it, umzug fails to load when running inside a pkged executable.

…

On Sun, Sep 8, 2024, 00:15 advplyr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package.json <#3378 (comment)> : > @@ -35,6 +37,7 @@ "author": "advplyr", "license": "GPL-3.0", "dependencies": { + ***@***.***/terminal": "^0.14.0", Is this being used? — Reply to this email directly, view it on GitHub <#3378 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMDFVWBPUHPWKL2DVU3DC3ZVNUGZAVCNFSM6AAAAABNT6IKEKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBYGQYTCOBYHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mikiher]

Actually, this dependency isn't really required by any Umzug code that we need (it's needed by Umzug CLI which we don't use). Let me try to replace this with a server/libs/Umzug implementation.

[mikiher]

OK, I replaced the umzug package dependency with a server/libs/umzug implementation, and removed all of its external dependencies (it now requires only built-in modules). Now the only new package included is semver, which I believe is very small and may be useful in the future for other code.

[advplyr]

Great thanks! I saw that package also included many other dependencies so I'm glad we don't need it.

I've stepped through this once already and was able to follow all of the logic. I still want to do some test migrations to make sure I understand the process.

What was the issue you had with storing the version info in the Settings table?

[mikiher]

Great thanks! I saw that package also included many other dependencies so I'm glad we don't need it.

Yeah, it was really not that hard to get rid of them, given our needs from umzug.

I've stepped through this once already and was able to follow all of the logic. I still want to do some test migrations to make sure I understand the process.

Yes, please do. I of course did that myself, but it would be great to have some more testing.
Please read the readme file carefully when you're writing those migrations, and let me know if there's anything to add there.

In particular, the note about trying not to depend on non-built-in modules is important. I made it possible to do so (and it was not altogether straightforward to achieve this, given that migration scripts are not loaded from the project source tree), because I see cases where it's useful and convenient to depend on some ABS installed packages (like sequelize) or core modules, but it's also risky, because (especially when downgrading) they're not guaranteed to be available/the same.

What was the issue you had with storing the version info in the Settings table?

Not so much of an issue, more a clearer separation of migration meta information from project data.

For example, when you start a new database, it is still empty when MigrationManager.init() is run, but you still want to create the migration metadata - it's possible to create the settings table at this point and store stuff in it, but it seemed risky to do so - what if it there is an assumption somewhere in our code that it should not exist for a new database?

And just generally, using project tables for storing migration meta information seemed not a very good idea on second thought.

[nichwall]

In particular, the note about trying not to depend on non-built-in modules is important. I made it possible to do so (and it was not altogether straightforward to achieve this, given that migration scripts are not loaded from the project source tree), because I see cases where it's useful and convenient to depend on some ABS installed packages (like sequelize) or core modules, but it's also risky, because (especially when downgrading) they're not guaranteed to be available/the same.

Should we be able to depend on Sequelize being available? My thought was most of these migrations would be using Sequelize to update the database tables, like changing the series columns to be unique, or replacing a column with a VIRTUAL column.

I could see that being an issue in the future if there are breaking changes in Sequelize.

[mikiher]

The sequelize object should be available through queryInteface.sequelize

…

On Tue, Sep 10, 2024, 18:40 Nicholas W ***@***.***> wrote: In particular, the note about trying not to depend on non-built-in modules is important. I made it possible to do so (and it was not altogether straightforward to achieve this, given that migration scripts are not loaded from the project source tree), because I see cases where it's useful and convenient to depend on some ABS installed packages (like sequelize) or core modules, but it's also risky, because (especially when downgrading) they're not guaranteed to be available/the same. Should we be able to depend on Sequelize being available? My thought was most of these migrations would be using Sequelize to update the database tables, like changing the series columns to be unique, or replacing a column with a VIRTUAL column. I could see that being an issue in the future if there are breaking changes in Sequelize. — Reply to this email directly, view it on GitHub <#3378 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMDFVV47QWQM3DV3J2OOFLZV4HIFAVCNFSM6AAAAABNT6IKEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGI4TMMZXGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[advplyr]

I tested this thoroughly and couldn't find any issues. I think it is a great solution.

Thanks for setting this up!

At some point I'd like to remove the old one-way migrations from v2.3.1 to v2.3.3. Those got ugly since I was trying to fix breaking changes from the initial sqlite migration. I'm thinking it would be best to require anyone still on v2.3.2 or below to first upgrade to v2.3.3 before upgrading to latest. I'm not sure about that yet, but slowly the old db objects are getting removed and it would be nice to also remove the old migration files.

[nichwall]

Are users able to update from 2.2.23 to 2.13.4?

If so, I think requiring people to update to 2.3.3 or 2.13.4 before continuing to upgrade is a fair solution, maybe just throw an error at startup once the migrations are removed to let people know they need to first upgrade to the intermediate image (and then update documentation on the website).

[advplyr]

Yeah, they would be able to update to whatever the last version was before we remove the old migration scripts, but could also just update to v2.3.3 then jump to latest.

If we do end up doing then that would be a good call to add in an explicit error message with that info.