refactor: split out sql migrations from library#1432
Draft
Computerdores wants to merge 27 commits into
Draft
Conversation
Functionality is entirely unchanged, but entire method is duplicated. Removing dead code from either copy is next.
Computerdores
added
Type: Refactor
CyanVoxel
reviewed
Jul 5, 2026
Assurance 1 was introduced with library version 101, specifically commmit 12e074b. Thus all libraries created on version 101 and above already fullfill it and don't need it. Furthermore, it only fails if the library didn't need the assurance, meaning the try-except and warning catching can be removed
All of these were introduced to account for the differences between new libraries and existing ones, which makes them unnecessary after this split.
The folder assurance has been present since the very first SQL commit e5e7b8a, and thus only has an effect when the library is moved, meaning that is semantically not part of the migrations.
Assurance 3 was introduced in commit 47c3d53 shortly (24h 20min) after the DB version had been bumped to 200. Since it was also included in the first release with DB version 200, all libraries created on DB version 200 and above can be presumed to have already had this applied on creation.
Moving the assurance after the migrations 7, 8, 9, and 100 is fine because it doesn't affect those.
Since every migration migrates the library to a certain DB_VERSION, we can simply update the library version after such a successfull migration. This way if a migration fails, the previous migrations won't be rerun the next time the library is opened.
None of the content of the migrations is changed, only the `with session:` statements are removed.
Also removes various try-except statements in the migrations. These were introduced to catch the case where the migration is run twice due to a later migration failing, this is not necessary anymore as every migration will only be executed at most once per library.
DB Migration 8 was previously adding all colors except for the neon ones, however, all but three of those have been around since DB version 4, meaning that they don't need to be added at all (which caused the tests to fail).
Collaborator
Author
|
Ok, the spaghetti code has been largely untangled. |
By default in SQLAlchemy schema changes are automatically committed, even if auto-commit is turned off. This commit turns off auto-commit completely, which caused some schema to not be applied correctly, but those were fixed here as well.
Collaborator
Author
|
@CyanVoxel I have now tested the full chain of migrations step by step and fixed two issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Currently creation of newly libraries, opening of existing libraries and migration of old libraries are all intertwined in the
Library.open_sqlite_librarymethod.In order to make this code more maintainable, I want to split it such that:
LibraryclassSince this will inevitably end up being a rather large refactor, I will endeavor to make commits that can be independently reviewed and be found to make sense.
This way the reviewer can review in smaller chunks.
In order to assist with this, I plan to add descriptions to the commit when I think it is necessary or helpful for understanding the intention behind the commit.
Also, I won't force-push on this PR at all, so reviewing the existing progress while the PR is still a draft and in progress shouldn't be a problem.
I tested each migration on this test library: ts_library.sqlite.zip
Tasks Completed