fix: close dangling session, migration corrections

[CyanVoxel]

Summary

This PR fixes issues caused by a missing session context manager in the all_entries() method in conjunction with the new autocommit=False DB parameter, and makes some tweaks to the migration refactors done in #1432 and #1456, most notably removing static methods in the Library class and removing local imports ^{because I'm an idiot and forgot that I didn't completely review #1456 before signing off and pulling it}

Closes #1447, Closes #1467

Tasks Completed

• Platforms Tested:
  • Windows x86
  • Windows ARM
  • macOS x86
  • macOS ARM
  • Linux x86
  • Linux ARM
• Tested For:
  • Basic functionality
  • PyInstaller executable

[Computerdores]

Why make that non-static?

[Computerdores]

I had originally passed the library path and engine because the engine argument will be removed when the migrations are made sql alchemy independent (and thus fully independent of the model that the to-be-migrated lib doesn't yet follow). Once that is done the parameter would only be the library path which is a much looser coupling then passing the full library in¹.

For that reason I feel it is undesirable to pass the entire library to the constructor here.
Also, afaict it isn't necessary.

Note: Afaict the statements setting library_dir in Library.create_sqlite_library Library.open_sqlite_library can also be removed once this is reverted.

Footnotes

1. We want the coupling to be as loose as possible here, because the logic in the Library class will be for the current schema while the migrations deal with old schemas and so we can't (and shouldn't where we still can) rely on that logic. ↩

[Computerdores]

I like annotating the methods with the type for fmt_log, but I think it would be better to define something like LoggingMethod = Callable[[str], str] and to then set fmt_log: LoggingMethod

[Computerdores]

I had made this static since the idea is to call it when the library isn't loaded yet (and can't be loaded at all due to needing to be migrated), that's why it was static and why I still think it should be here.

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

[CyanVoxel]

The save_library_backup_to_disk() method doesn't require a library to be loaded, it just uses the Library instance's self.library_dir, which I tweaked to be set before the migrations are run in open_sqlite_library().

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

This method is also used outside of the migrations, like in the UI - so there still needs to be a non-static version of it present in the Library class for instances to use without passing the argument of the instance's own library_dir.

[Computerdores]

The save_library_backup_to_disk() method doesn't require a library to be loaded, it just uses the Library instance's self.library_dir, which I tweaked to be set before the migrations are run in open_sqlite_library().

Yeah this is exactly what I meant; normally the library_dir means "the directory of the currently open library" but here it effectively is "the directory of the library that should be backed up" (unless a library is actually open, in which the original meaning is correct again).

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

This method is also used outside of the migrations, like in the UI - so there still needs to be a non-static version of it present in the Library class for instances to use without passing the argument of the instance's own library_dir.

I think having a static method and a non-static method that calls the static one (with self.library_dir as the param) would be best then.

[Computerdores]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

[CyanVoxel]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

Where might an inconsistency arise that would be solved by passing a different engine to build a session from?

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

The current UI-facing use of this is working as intended, where it shows the current loaded DB version in comparison to the program's currently known DB_VERSION, which is useful for telling if you have a DB loaded with a minor version greater than the TagStudio version that's opening it. I'd also like to not gate the _get_version() method to the migrations, as that would limit future uses in the UI.

Perhaps the "static" version of this that includes the engine parameter and inspection conditional could be moved to the migrations class, while a simpler instance method that just uses the currently expected logic could remain in the library file? Though that's a tiny bit of code duplication

[Computerdores]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

Where might an inconsistency arise that would be solved by passing a different engine to build a session from?

The engine is specific to the DB that is currently open, so if we were to e.g. allow a user to bulk migrate libraries while a different library is open, then this get_version method would cause the migrations to see the incorrect version and not migrate anything because it would see the version of the currently open library and not the one of the DB being migrated.

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

The current UI-facing use of this is working as intended, where it shows the current loaded DB version in comparison to the program's currently known DB_VERSION, which is useful for telling if you have a DB loaded with a minor version greater than the TagStudio version that's opening it.

Good point, I missed that.

I'd also like to not gate the _get_version() method to the migrations, as that would limit future uses in the UI.

Perhaps the "static" version of this that includes the engine parameter and inspection conditional could be moved to the migrations class, while a simpler instance method that just uses the currently expected logic could remain in the library file? Though that's a tiny bit of code duplication

Yes, I think that would be best. That way get_version is basically a one-liner and the complicated DB introspection logic can be relegated to the migrations.
Also, uncoupling the migrations from sqlalchemy would probably require that anyway, so we might as well do it now.

[Computerdores]

This PR fixes issues caused by a missing session context manager in the all_entries() method in conjunction with the new autocommit=False DB parameter

RE the actual fix: I am not quite sure why this fixes anything, because the original setup was not missing any context managers at all. The _all_entries method was simply shifting the burden of opening the context manager to the caller instead of opening its own (which would have caused errors in the migrations due to two DB sessions being opened at the same time). My best guess is that because the code now used in the migrations instead is much simpler, which makes me suspicious that the same error might still occur where all_entries is still used.

[CyanVoxel]

Why make[...] non-static?

For the non-static reversions, this was mostly motivated by the idea that the Library is not intended to be a singleton class - each instance of the Library class should be completely self-contained and methods for the class that affect instanced versions should only affect data for said instance. While making some methods static didn't necessarily turn the Library class into a singleton, it still separated the self-contained nature of the Library instance by requiring a session to be independently supplied (which raised a deeper issue with the all_entries() method). This also got rid of the local imports in migrations.py, which I had missed were added.

RE the actual fix: I am not quite sure why this fixes anything, because the original setup was not missing any context managers at all. The _all_entries method was simply shifting the burden of opening the context manager to the caller instead of opening its own (which would have caused errors in the migrations due to two DB sessions being opened at the same time). My best guess is that because the code now used in the migrations instead is much simpler, which makes me suspicious that the same error might still occur where all_entries is still used.

When turning the all_entries() method back into a non-static class an using the session associated with the Library class instance, that fixed the DB lock that was occurring presumably due to a mismatched instance being passed from the migrations that was holding the lock. Following the thread of this fix raised issues with migration 9, where the results from all_entries() would not contain the flushed column that gets added in 9, which is where I switched to querying the DB directly via the migration's session instead of using the full all_entries() function that didn't contain the flushed changes (I'm pretty sure this is a reason I had the migrations split in a weird order initially, as a poor workaround). These changes also enforce using Library instances as a single unit instead of passing around session contexts that can become disjointed due to SQLAlchemy being just wonderful to work with.

So a lot of the static reversions weren't 100% necessary to pull off this fix, but sort of went hand-in-hand with it and also reflect my own intentions for how the Library class should be used.

[Computerdores]

Why make[...] non-static?

For the non-static reversions, this was mostly motivated by the idea that the Library is not intended to be a singleton class - each instance of the Library class should be completely self-contained and methods for the class that affect instanced versions should only affect data for said instance.

This only applies to the all_entries method though (which I agree shouldn't have been static, no idea why I did that ^^).
Neither _get_engine nor save_library_backup_to_disk affect (or access) the data of the Library instance...

Also, the semantics of save_library_backup_to_disk are to make a backup of the library DB found at a certain path, not to make a backup of the currently opened library. And making the method non-static implies that it could access anything available on the instance when a library is accessed, which could lead to problems, because in order to call it from the migrations only the library path is set and the library is actually opened (which would of course not be possible). This is why I have to object to making save_library_backup_to_disk an instance method.

When turning the all_entries() method back into a non-static class an using the session associated with the Library class instance,

The session is in no way associated with the Library instance

that fixed the DB lock that was occurring presumably due to a mismatched instance being passed from the migrations that was holding the lock.

If you compare how the session is built now and how it was built then, they are exactly identical, so this cannot be the reason afaict.

These changes also enforce using Library instances as a single unit instead of passing around session contexts that can become disjointed

Not quite sure what you mean here

[CyanVoxel]

Also, the semantics of save_library_backup_to_disk are to make a backup of the library DB found at a certain path, not to make a backup of the currently opened library. And making the method non-static implies that it could access anything available on the instance when a library is accessed, which could lead to problems, because in order to call it from the migrations only the library path is set and the library is actually opened (which would of course not be possible). This is why I have to object to making save_library_backup_to_disk an instance method.

The save_library_backup_to_disk() was previously an instance method and used as such outside of migrations (e.g. the backup_library() method in ts_qt.py). There can be special cases for things that the new migration system needs, but it shouldn't be assumed that these functions aren't being used elsewhere where the expectation is to just have the Library instance, and then tell that library to save a backup using its own library_dir.

The session is in no way associated with the Library instance

I'm referring to how the session context managers in the Library class are constructed using with Session(self.engine) as session:, which use the self.engine variable inside the particular Library instance. Reverting the all_entries() method to be non-static and restoring the original context manager with self.engine fixes the lock on the DB created by "something else, somewhere else".

If you compare how the session is built now and how it was built then, they are exactly identical, so this cannot be the reason afaict.

If you've got an alternate explanation for #1447 I'm all ears, but from all the investigating and testing I've done it really seems like the session currently is getting detached or desycned or something from the rest of the Library, causing a lock on the DB because there's now something outside the scope of the expected session that's using it (in conjunction with using autocommit=False). And restoring the original context manager using the instantiated Library class's self.engine resolves that lock issue without having to revert the recommended autocommit parameter.

Changes made from there were either due to other obscured issues like the DB9 migration flushing and/or due to wanting to enforce a particular pattern for accessing library methods.