From 475f7227c4a63da546768d1852f62b682336931f Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 12 Dec 2023 09:51:30 -0500 Subject: [PATCH 1/2] Add future=True flag to SA engine --- lib/galaxy/model/database_utils.py | 2 +- lib/galaxy/model/migrations/__init__.py | 4 ++-- lib/galaxy/model/migrations/alembic/env.py | 2 +- lib/galaxy/model/migrations/scripts.py | 6 +++--- lib/galaxy/model/orm/engine_factory.py | 2 +- .../model/unittest_utils/migration_scripts_testing_utils.py | 2 +- lib/galaxy/model/unittest_utils/model_testing_utils.py | 4 ++-- lib/tool_shed/webapp/model/migrations/__init__.py | 2 +- lib/tool_shed/webapp/model/migrations/alembic/env.py | 2 +- scripts/check_model.py | 2 +- scripts/update_shed_config_path.py | 2 +- test/unit/data/model/conftest.py | 2 +- 12 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/galaxy/model/database_utils.py b/lib/galaxy/model/database_utils.py index a67470410373..123bd22a12e6 100644 --- a/lib/galaxy/model/database_utils.py +++ b/lib/galaxy/model/database_utils.py @@ -45,7 +45,7 @@ def create_database(db_url, database=None, encoding="utf8", template=None): @contextmanager def sqlalchemy_engine(url): - engine = create_engine(url) + engine = create_engine(url, future=True) try: yield engine finally: diff --git a/lib/galaxy/model/migrations/__init__.py b/lib/galaxy/model/migrations/__init__.py index da0a1927c867..61c367c98085 100644 --- a/lib/galaxy/model/migrations/__init__.py +++ b/lib/galaxy/model/migrations/__init__.py @@ -124,10 +124,10 @@ def verify_databases_via_script( ) -> None: # This function serves a use case when an engine has not been created yet # (e.g. when called from a script). - gxy_engine = create_engine(gxy_config.url) + gxy_engine = create_engine(gxy_config.url, future=True) tsi_engine = None if tsi_config.url and tsi_config.url != gxy_config.url: - tsi_engine = create_engine(tsi_config.url) + tsi_engine = create_engine(tsi_config.url, future=True) verify_databases( gxy_engine, diff --git a/lib/galaxy/model/migrations/alembic/env.py b/lib/galaxy/model/migrations/alembic/env.py index 5c3ec11f8298..cf9cb6b36a81 100644 --- a/lib/galaxy/model/migrations/alembic/env.py +++ b/lib/galaxy/model/migrations/alembic/env.py @@ -116,7 +116,7 @@ def _configure_and_run_migrations_offline(url: str) -> None: def _configure_and_run_migrations_online(url) -> None: - engine = create_engine(url) + engine = create_engine(url, future=True) with engine.connect() as connection: context.configure(connection=connection, target_metadata=target_metadata) with context.begin_transaction(): diff --git a/lib/galaxy/model/migrations/scripts.py b/lib/galaxy/model/migrations/scripts.py index 6315c83ecd23..207928653a0a 100644 --- a/lib/galaxy/model/migrations/scripts.py +++ b/lib/galaxy/model/migrations/scripts.py @@ -59,7 +59,7 @@ def verify_database_is_initialized(db_url: str) -> None: if not database_exists(db_url): raise DatabaseDoesNotExistError(db_url) - engine = create_engine(db_url) + engine = create_engine(db_url, future=True) try: db_state = DatabaseStateCache(engine=engine) if db_state.is_database_empty() or db_state.contains_only_kombu_tables(): @@ -161,7 +161,7 @@ def get_gxy_db_version(self, gxy_db_url=None): """ db_url = gxy_db_url or self.gxy_db_url try: - engine = create_engine(db_url) + engine = create_engine(db_url, future=True) version = self._get_gxy_alembic_db_version(engine) if not version: version = self._get_gxy_sam_db_version(engine) @@ -197,7 +197,7 @@ def _rename_arg(self, argv, old_name, new_name) -> None: def _upgrade(self, db_url, model): try: - engine = create_engine(db_url) + engine = create_engine(db_url, future=True) am = get_alembic_manager(engine) am.upgrade(model) finally: diff --git a/lib/galaxy/model/orm/engine_factory.py b/lib/galaxy/model/orm/engine_factory.py index 538602fec54a..886a4e3462ab 100644 --- a/lib/galaxy/model/orm/engine_factory.py +++ b/lib/galaxy/model/orm/engine_factory.py @@ -102,7 +102,7 @@ def after_cursor_execute(conn, cursor, statement, parameters, context, executema engine_options = engine_options or {} engine_options = set_sqlite_connect_args(engine_options, url) - engine = create_engine(url, **engine_options) + engine = create_engine(url, **engine_options, future=True) # Prevent sharing connection across fork: https://docs.sqlalchemy.org/en/14/core/pooling.html#using-connection-pools-with-multiprocessing-or-os-fork register_after_fork(engine, lambda e: e.dispose()) diff --git a/lib/galaxy/model/unittest_utils/migration_scripts_testing_utils.py b/lib/galaxy/model/unittest_utils/migration_scripts_testing_utils.py index e84f38e6adac..7aad9d474081 100644 --- a/lib/galaxy/model/unittest_utils/migration_scripts_testing_utils.py +++ b/lib/galaxy/model/unittest_utils/migration_scripts_testing_utils.py @@ -75,7 +75,7 @@ def run_command(cmd: str) -> subprocess.CompletedProcess: def get_db_heads(config: Config) -> Tuple[str, ...]: """Return revision ids (version heads) stored in the database.""" dburl = config.get_main_option("sqlalchemy.url") - engine = create_engine(dburl) + engine = create_engine(dburl, future=True) with engine.connect() as conn: context = MigrationContext.configure(conn) heads = context.get_current_heads() diff --git a/lib/galaxy/model/unittest_utils/model_testing_utils.py b/lib/galaxy/model/unittest_utils/model_testing_utils.py index c128a1634b80..3f13dd79f731 100644 --- a/lib/galaxy/model/unittest_utils/model_testing_utils.py +++ b/lib/galaxy/model/unittest_utils/model_testing_utils.py @@ -67,7 +67,7 @@ def drop_existing_database(url: DbUrl) -> Iterator[None]: @contextmanager def disposing_engine(url: DbUrl) -> Iterator[Engine]: """Context manager for engine that disposes of its connection pool on exit.""" - engine = create_engine(url) + engine = create_engine(url, future=True) try: yield engine finally: @@ -233,7 +233,7 @@ def _drop_postgres_database(url: DbUrl) -> None: def _drop_database(connection_url, database_name): - engine = create_engine(connection_url, isolation_level="AUTOCOMMIT") + engine = create_engine(connection_url, isolation_level="AUTOCOMMIT", future=True) preparer = IdentifierPreparer(engine.dialect) database_name = preparer.quote(database_name) stmt = text(f"DROP DATABASE IF EXISTS {database_name}") diff --git a/lib/tool_shed/webapp/model/migrations/__init__.py b/lib/tool_shed/webapp/model/migrations/__init__.py index eae748a507d5..8cf1165661c5 100644 --- a/lib/tool_shed/webapp/model/migrations/__init__.py +++ b/lib/tool_shed/webapp/model/migrations/__init__.py @@ -46,7 +46,7 @@ def __init__(self) -> None: def verify_database(url, engine_options=None) -> None: engine_options = engine_options or {} - engine = create_engine(url, **engine_options) + engine = create_engine(url, **engine_options, future=True) verifier = DatabaseStateVerifier(engine) verifier.run() engine.dispose() diff --git a/lib/tool_shed/webapp/model/migrations/alembic/env.py b/lib/tool_shed/webapp/model/migrations/alembic/env.py index 4c05fabaf5d7..df344a900e28 100644 --- a/lib/tool_shed/webapp/model/migrations/alembic/env.py +++ b/lib/tool_shed/webapp/model/migrations/alembic/env.py @@ -62,7 +62,7 @@ def _configure_and_run_migrations_offline(url: str) -> None: def _configure_and_run_migrations_online(url) -> None: - engine = create_engine(url) + engine = create_engine(url, future=True) with engine.connect() as connection: context.configure(connection=connection, target_metadata=target_metadata) with context.begin_transaction(): diff --git a/scripts/check_model.py b/scripts/check_model.py index 1675dc76ba8f..015e3509ac89 100644 --- a/scripts/check_model.py +++ b/scripts/check_model.py @@ -48,7 +48,7 @@ def load_indexes(metadata): # create EMPTY metadata, then load from database db_url = get_config(sys.argv)["db_url"] metadata = MetaData() - engine = create_engine(db_url) + engine = create_engine(db_url, future=True) metadata.reflect(bind=engine) indexes_in_db = load_indexes(metadata) diff --git a/scripts/update_shed_config_path.py b/scripts/update_shed_config_path.py index dd13f480ccb6..faa7b8871f2c 100644 --- a/scripts/update_shed_config_path.py +++ b/scripts/update_shed_config_path.py @@ -46,7 +46,7 @@ def create_database(config_file): exit(1) # Initialize the database connection. - engine = create_engine(database_connection) + engine = create_engine(database_connection, future=True) MetaData(bind=engine) install_session = scoped_session(sessionmaker(bind=engine, autoflush=False, autocommit=True)) model = mapping.init(database_connection) diff --git a/test/unit/data/model/conftest.py b/test/unit/data/model/conftest.py index 4d8728e9f197..95908e2c8c1c 100644 --- a/test/unit/data/model/conftest.py +++ b/test/unit/data/model/conftest.py @@ -30,7 +30,7 @@ def sqlite_memory_url(): @pytest.fixture(scope="module") def engine(): db_uri = "sqlite:///:memory:" - return create_engine(db_uri) + return create_engine(db_uri, future=True) @pytest.fixture From 377f783e20b37907b87e25bb746a22ed3584ea64 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 12 Dec 2023 14:38:46 -0500 Subject: [PATCH 2/2] Commit instead of flushing in unit test Rationale: We use an in-memory sqlite database for quota tests. With SA 2.0 (or with the `future` flag enabled on the engine), the following conflict happens: (this is a simplified model) foo = Foo() session.add(foo) session.flush() engine = session.get_bind() with engine.connect() as conn: conn.execute(some-sql) foo.bar = "new value" session.commit() # BOOM!!!! sqlalchemy.orm.exc.StaleDataError: UPDATE statement on table 'galaxy_user' expected to update 1 row(s); 0 were matched. Reason for BOOM: With an in-memory database, the underlying dbapi_connection object is the same for the session and the engine.connect(). Here's what happens: line 10: foo is flushed to the db tmp buffer line 14: conn is closed on exit from context manager, which issues a rollback, which rolls back whatever is in the tmp buffer - so foo is never inserted. line 16: foo is updated line 17: error happens: the session thinks it's updating foo's record in the db, but that record does not exist therefore, "0 rows matched". Solution: commit instead of flushing - then foo is inserted. --- test/unit/data/test_galaxy_mapping.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/data/test_galaxy_mapping.py b/test/unit/data/test_galaxy_mapping.py index 20822dc70222..0dae47177a0e 100644 --- a/test/unit/data/test_galaxy_mapping.py +++ b/test/unit/data/test_galaxy_mapping.py @@ -56,12 +56,12 @@ def setUpClass(cls): @classmethod def persist(cls, *args, **kwargs): session = cls.session() - flush = kwargs.get("flush", True) + commit = kwargs.get("commit", True) for arg in args: session.add(arg) - if flush: - session.flush() - if kwargs.get("expunge", not flush): + if commit: + session.commit() + if kwargs.get("expunge", not commit): cls.expunge() return arg # Return last or only arg. @@ -255,7 +255,7 @@ def test_collection_get_interface(self): model.DatasetCollectionElement(collection=c1, element=d1, element_identifier=f"{i}", element_index=i) for i in range(elements) ] - self.persist(u, h1, d1, c1, *dces, flush=False, expunge=False) + self.persist(u, h1, d1, c1, *dces, commit=False, expunge=False) self.model.session.flush() for i in range(elements): assert c1[i] == dces[i]