scVENUS / PeekabooAV

Peekaboo Extended Email Attachment Behavior Observation Owl
https://peekabooav.de
GNU General Public License v3.0
66 stars 20 forks source link

SQLAlchemy fix #186

Closed Jack28 closed 3 years ago

Jack28 commented 3 years ago

With version 1.4.0 SQLAlchemy introduced the inspect object that can check if a table exists using has_table. Our check leads to the error and the message: "the Dialect.has_table() method is for internal dialect use only; please use inspect(some_engine).has_table(<tablename>>) for public API use."

Now the original behaviour is restored pushing the minimal required version of SQLAlchemy to 1.4.0.

Jack28 commented 3 years ago

The affected function was later removed in https://github.com/scVENUS/PeekabooAV/commit/5694c4096f7ddc4dd8d80b1a2656109f5306ea2a

Jack28 commented 3 years ago

The last Travis job on this branch ran 3 months ago and successfully https://travis-ci.com/github/scVENUS/PeekabooAV/jobs/484267539

Releases of dependencies are not tracked and so it didn't rebuild since and the issue didn't surface.

Manually triggering a rebuild doesn't work either: "Oh no! An error occurred. The build could not be restarted." Will investigate

michaelweiser commented 3 years ago

The affected function was later removed in 5694c40

I had totally forgotten about that. So I can confirm that:

I very much like the codestyle changes but would not backport them to a release version. Rather we could have a separate PR to add them to devel as a permanent cleanup. I would also go a step further and also remove the other from imports with very few (mostly only one) users something like so:

@@ -27,11 +27,9 @@ SQLAlchemy. """

 import threading
 import logging
-import sqlalchemy
 from datetime import datetime, timedelta
-from sqlalchemy.ext.declarative import declarative_base
-from sqlalchemy.engine import create_engine
-from sqlalchemy.orm import sessionmaker, scoped_session, relationship
+import sqlalchemy
+import sqlalchemy.ext.declarative
 from sqlalchemy.exc import SQLAlchemyError, IntegrityError
 from peekaboo import __version__
 from peekaboo.ruleset import Result
@@ -40,7 +38,7 @@ from peekaboo.exceptions import PeekabooDatabaseError
 DB_SCHEMA_VERSION = 7

 logger = logging.getLogger(__name__)
-Base = declarative_base()
+Base = sqlalchemy.ext.declarative.declarative_base()

 #
@@ -159,7 +157,7 @@ class AnalysisJournal(Base):
                            index=True)
     sample_id = sqlalchemy.Column(sqlalchemy.Integer, sqlalchemy.ForeignKey(SampleInfo.id),
                        nullable=False, index=True)
-    sample = relationship('SampleInfo')
+    sample = sqlalchemy.orm.relationship('SampleInfo')

     def __str__(self):
         return (
@@ -198,9 +196,9 @@ class PeekabooDatabase(object):
         """
         logging.getLogger('sqlalchemy.engine').setLevel(log_level)

-        self.__engine = create_engine(db_url, pool_recycle=1)
-        session_factory = sessionmaker(bind=self.__engine)
-        self.__session = scoped_session(session_factory)
+        self.__engine = sqlalchemy.engine.create_engine(db_url, pool_recycle=1)
+        session_factory = sqlalchemy.orm.sessionmaker(bind=self.__engine)
+        self.__session = sqlalchemy.orm.scoped_session(session_factory)
         self.__lock = threading.RLock()
         self.instance_id = instance_id
         self.stale_in_flight_threshold = stale_in_flight_threshold

I'd make an exception for the exceptions (huh :) because of their number of uses and because their origin in SQLAlchemy is mostly made clear by the context of their usage. We also do it this way elsewhere in the code.

I've quickly hacked this onto your branch, so it won't apply to devel head because of the other changes there but you catch my drift. I'm not sure, why ext.declarative needs an explicit import but it does for me.

I reordered datetime above because pylint comes back with an ordering warning for it now:

peekaboo/db.py:32:0: C0411: standard import "from datetime import datetime, timedelta" should be placed before "import sqlalchemy" (wrong-import-order)

Might be an opportunity to extend the namespace cleanup to that as well and reorder imports alphabetically.

All this is personal taste and open for discussion as well as nice-to-have prioritisation-wise.

michaelweiser commented 3 years ago

Releases of dependencies are not tracked and so it didn't rebuild since and the issue didn't surface.

Travis has cron jobs. I set up jobs for branch 2.0 and devel head. This immediately led to a notification email that 2.0 is broken. So we should be good to go now. Will update the release documentation in the wiki so we add more cron jobs as we add more release branches.