kvesteri / sqlalchemy-continuum

Versioning extension for SQLAlchemy.
BSD 3-Clause "New" or "Revised" License
568 stars 128 forks source link

Flask SQLAlchemy Binds support #338

Open hobzcalvin opened 9 months ago

hobzcalvin commented 9 months ago

SQLAlchemy-Continuum does not appear to support Flask-SQLAlchemy's Binds: https://flask-sqlalchemy.palletsprojects.com/en/2.x/binds/#binds

When attempting to modify a versioned object, the INSERT into transaction table fails because it is attempting to connect to the default database SQLALCHEMY_DATABASE_URI, which is SQLite, even though I'm using properly-configured SQLALCHEMY_BINDS and the __bind_key__ for models. All versioning behavior should use the same bind as the models it extends.

Really, I'd just like some help getting this to work! ;-) Setting SQLALCHEMY_DATABASE_URI to the correct target helped, but I'm still seeing issues that seem like they're part of the same lack of support, e.g.:

File "/root/.local/lib/python3.9/site-packages/sqlalchemy_continuum/manager.py", line 414, in append_association_operation
     uow = self.units_of_work[conn.engine]
KeyError: Engine(mysql://url.of.mysql.db)
marksteward commented 9 months ago

This looks like a flask-sqlalchemy only thing. I'm guessing the plugin will need to copy the __bind_key__ across (with optional override?) to the version models.

I'll happily take a PR as long as it does nothing when the plugin isn't used. It probably needs some code adding here.

hobzcalvin commented 9 months ago

Working on this. It was easy to pass along the __bind_key__ to version models, but the trickier part is getting sessions to work properly. Namely, UnitOfWork does this in 3 places:

        if not self.version_session:
            self.version_session = sa.orm.session.Session(
                bind=session.connection()
            )

The passed-in session object has its __binds configured properly, but the constructed version_session will not. I may be able to add them after the fact in FlaskPlugin, though the hooks I want aren't quite there (maybe I can add another).

However, I'm now hitting (sqlite3.OperationalError) database is locked which suggests that multiple session objects may be causing issues. I assume a separate version_session is created to avoid using the end-user-code session, but this may cause issues when the sessions have multiple binds. My reading suggests that this may be an issue only on sqlite since it doesn't handle multithreading very well. Any thoughts appreciated.

marksteward commented 9 months ago

Ahh that's annoying. Sqlite can support multiple writing from multiple threads but doesn't by default. We might need some sqlite-specific documentation.

hobzcalvin commented 9 months ago

Well that error went away if I didn't pass bind to the Session constructor, so it may have just been an issue where we assume the default bind when we really shouldn't. I think, more generally, we have an issue where now that a Session can have multiple binds and thus multiple engines, necessitating multiple connections, the session :: connection :: unit_of_work 1:1:1 mapping I see in VersioningManager is no longer sufficient. So maybe the real question is, do we need a UnitOfWork for each bind? Probably. Taking all of this with a grain of salt: I am learning about SQLAlchemy sessions/engines/connections as I go, so I am no expert!