kvesteri / sqlalchemy-continuum

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

KeyError: Engine(sqlite://) when using before_flush with sessionmaker #275

Closed fadyZohdy closed 2 years ago

fadyZohdy commented 2 years ago

we use sessionmaker in our app and i have been getting this error when i listen to the before_flush event with sessionmaker object

Traceback (most recent call last):
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy_continuum/manager.py", line 25, in wrapper
    uow = self.units_of_work[conn]
KeyError: <sqlalchemy.engine.base.Connection object at 0x7f849c2bee80>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test.py", line 30, in <module>
    session.commit()
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 1435, in commit
    self._transaction.commit(_to_root=self.future)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 829, in commit
    self._prepare_impl()
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 808, in _prepare_impl
    self.session.flush()
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 3367, in flush
    self._flush(objects)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 3507, in _flush
    transaction.rollback(_capture_exception=True)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/session.py", line 3467, in _flush
    flush_context.execute()
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/unitofwork.py", line 456, in execute
    rec.execute(self)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/unitofwork.py", line 630, in execute
    util.preloaded.orm_persistence.save_obj(
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/persistence.py", line 253, in save_obj
    _finalize_insert_update_commands(
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/persistence.py", line 1566, in _finalize_insert_update_commands
    mapper.dispatch.after_insert(mapper, connection, state)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/event/attr.py", line 256, in __call__
    fn(*args, **kw)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy/orm/events.py", line 743, in wrap
    fn(*arg, **kw)
  File "/Users/fady/.virtualenvs/test-sqlalchemy/lib/python3.8/site-packages/sqlalchemy_continuum/manager.py", line 28, in wrapper
    uow = self.units_of_work[conn.engine]
KeyError: Engine(sqlite://)

this error can be reproduced by doing a small modification to the example in README

from sqlalchemy_continuum import make_versioned
from sqlalchemy import Column, Integer, Unicode, UnicodeText, create_engine
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import sessionmaker, configure_mappers
from sqlalchemy import event

make_versioned(user_cls=None)

Base = declarative_base()
class Article(Base):
    __versioned__ = {}
    __tablename__ = 'article'
    id = Column(Integer, primary_key=True, autoincrement=True)
    name = Column(Unicode(255))
    content = Column(UnicodeText)

configure_mappers()
engine = create_engine('sqlite://', isolation_level=None)
Base.metadata.create_all(engine)

factory = sessionmaker(bind=engine, autocommit=False)
session = factory()

@event.listens_for(factory, 'before_flush')
def receive_before_flush(session, flush_context, instances):
    pass

article = Article(name=u'Some article', content=u'Some content')
session.add(article)
session.commit()
article.versions[0].name
article.name = u'Updated name'
session.commit()
article.versions[1].name
article.versions[0].revert()
article.name

deps versions:

python 3.8.2
SQLAlchemy==1.3.5
SQLAlchemy-Continuum==1.3.12
SQLAlchemy-Utils==0.34.2

my experience with sqlalchemy is not extensive and i tried debugging it but couldn't get anywhere. i am ready to help fix this but would use a little guidance.

marksteward commented 2 years ago

Yeah, this looks like a bug. I think you can work around it by moving the call to sessionmaker (and create_engine) to before the call to make_versioned.

I'll need to dig more into sqlalchemy's event logic to create a fix.

aditya051293 commented 2 years ago

We are calling make_versioned after calling sessionmaker. But facing this issue intermittently.

ipxwork commented 2 years ago

In my case I have to call make_versioned() before each model class I __versioned__. And DB session initialization part looks like this:

configure_mappers()

engine = create_engine(
    settings.SQLALCHEMY_DATABASE_URI,
    pool_pre_ping=True,
    # echo=True
)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)

make_versioned(plugins=[UserTrack()])

@event.listens_for(SessionLocal, "before_flush")
...

My UserTrack it's to get User.id from request headers.

Anyway, I still get a warning in the console:

lib/python3.9/site-packages/sqlalchemy_continuum/unit_of_work.py:260: SAWarning: implicitly coercing SELECT object to scalar subquery; please use the .scalar_subquery() method to produce a scalar subquery.
  getattr(
AbdealiLoKo commented 2 years ago

I am facing a similar issue - but not from event listener - but when I run my alembic migrations. Not sure if I should be opening a separate ticket for that or it should be a part of this same ticket.

It looks like when I run alembic upgrade it throws this same KeyError at this same point.

Also foud a similar ticket: https://github.com/kvesteri/sqlalchemy-continuum/issues/187

AbdealiLoKo commented 2 years ago

Spent some time on this. And seems like @marksteward was right - this is about the event management with sqlalchemy. I have raised a discussion on sqlalchemy asking for the correct way to register listeners on some events https://github.com/sqlalchemy/sqlalchemy/discussions/8466

AbdealiLoKo commented 2 years ago

This was confirmed to be a bug in sqlalchemy. This has been fixed in: https://github.com/sqlalchemy/sqlalchemy/issues/8467 So, we just have to wait for the next release.

As this is not a bug in sqlalchemy-continuum - shall we close this ticket @marksteward ?

AbdealiLoKo commented 2 years ago

sqlalchemy 1.4.41 was released yesterday - when I try the original example with this new version it works without any errors

venv/bin/pip install -U sqlalchemy==1.4.40
python test.py  # Throws the KeyError

venv/bin/pip install -U sqlalchemy==1.4.41
python test.py  # No errors !!

So, this ticket can be closed ?

marksteward commented 2 years ago

Yep, thanks @AbdealiJK!