From da87ca920970680410508471b49f7445da72d851 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 14 Oct 2022 13:17:42 -0400 Subject: [PATCH 1/4] Allow to set connect_args for SQLAlchemy's engine in config --- lib/galaxy/model/orm/engine_factory.py | 23 +++++++----- test/unit/data/model/test_engine_factory.py | 39 +++++++++++++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 test/unit/data/model/test_engine_factory.py diff --git a/lib/galaxy/model/orm/engine_factory.py b/lib/galaxy/model/orm/engine_factory.py index 0c7e7e4fa0e..98e177fd551 100644 --- a/lib/galaxy/model/orm/engine_factory.py +++ b/lib/galaxy/model/orm/engine_factory.py @@ -99,14 +99,8 @@ def build_engine( except AttributeError: pass - # Set check_same_thread to False for sqlite, handled by request-specific session - # See https://fastapi.tiangolo.com/tutorial/sql-databases/#note - connect_args = {} - if "sqlite://" in url: - connect_args["check_same_thread"] = False - # Create the database engine - engine_options = engine_options or {} - engine = create_engine(url, connect_args=connect_args, **engine_options) + engine_options = set_sqlite_connect_args(engine_options, url) + engine = create_engine(url, **engine_options) # 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()) @@ -126,3 +120,16 @@ def build_engine( ) return engine + + +def set_sqlite_connect_args(engine_options, url): + """ + Add or update `connect_args` in `engine_options` if db is sqlite. + Set check_same_thread to False for sqlite, handled by request-specific session. + See https://fastapi.tiangolo.com/tutorial/sql-databases/#note + """ + if url and url.startswith("sqlite://"): + engine_options = engine_options or {} + connect_args = engine_options.setdefault("connect_args", {}) + connect_args["check_same_thread"] = False + return engine_options diff --git a/test/unit/data/model/test_engine_factory.py b/test/unit/data/model/test_engine_factory.py new file mode 100644 index 00000000000..2453fe0e4e3 --- /dev/null +++ b/test/unit/data/model/test_engine_factory.py @@ -0,0 +1,39 @@ +from galaxy.model.orm.engine_factory import set_sqlite_connect_args + +SQLITE_URL = "sqlite://foo.db" +NON_SQLITE_URL = "foo://foo.db" + + +class TestSetSqliteConnectArgs: + def test_engine_options_none(self): + engine_options = None + updated = set_sqlite_connect_args(engine_options, SQLITE_URL) + assert updated == {"connect_args": {"check_same_thread": False}} + + def test_engine_options_empty(self): + engine_options = {} + updated = set_sqlite_connect_args(engine_options, SQLITE_URL) + assert updated == {"connect_args": {"check_same_thread": False}} + + def test_update_nonempty_engine_options(self): + engine_options = {"foo": "some foo"} + updated = set_sqlite_connect_args(engine_options, SQLITE_URL) + assert len(updated) == 2 + assert updated["foo"] == "some foo" + assert updated["connect_args"] == {"check_same_thread": False} + + def test_overwrite_connect_args(self): + engine_options = {"foo": "some foo", "connect_args": {"check_same_thread": True}} + updated = set_sqlite_connect_args(engine_options, SQLITE_URL) + assert len(updated) == 2 + assert updated["foo"] == "some foo" + assert updated["connect_args"] == {"check_same_thread": False} + + def test_update_nonempty_connect_args(self): + engine_options = {"foo": "some foo", "connect_args": {"bar": "some bar"}} + updated = set_sqlite_connect_args(engine_options, SQLITE_URL) + assert len(updated) == 2 + assert updated["foo"] == "some foo" + assert len(updated["connect_args"]) == 2 + assert updated["connect_args"]["check_same_thread"] is False + assert updated["connect_args"]["bar"] == "some bar" From 71829ae6aff5d17b83e3a9abe834ef5b07787edc Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 14 Oct 2022 13:18:35 -0400 Subject: [PATCH 2/4] Add more details to setting db engine options to config schema --- doc/source/admin/galaxy_options.rst | 27 ++++++++------------- lib/galaxy/config/sample/galaxy.yml.sample | 10 +++++++- lib/galaxy/config/schemas/config_schema.yml | 16 +++++++++--- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index 951592c5a1e..d5f3cb2e74b 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -57,10 +57,19 @@ :Description: By default, Galaxy uses a SQLite database at - '/universe.sqlite'. You may use a SQLAlchemy connection + '/universe.sqlite'. You may use a SQLAlchemy connection string to specify an external database instead. Sample default 'sqlite:////universe.sqlite?isolation_level=IMMEDIATE' + You may specify additional options that will be passed to the + SQLAlchemy database engine by using the prefix + "database_engine_option_". For some of these options, default + values are provided (e.g. see database_engine_option_pool_size, + etc.). + The same applies to `install_database_connection`, for which you + should use the "install_database_engine_option_" prefix. + For more options, please check SQLAlchemy's documentation at + https://docs.sqlalchemy.org/en/14/core/engines.html?highlight=create_engine#sqlalchemy.create_engine :Default: ``None`` :Type: str @@ -592,22 +601,6 @@ :Type: str -~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -``load_tool_shed_datatypes`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -:Description: - This option controls whether legacy datatypes are loaded from - installed tool shed repositories. We're are in the process of - disabling Tool Shed datatypes. This option with a default of true - will be added in 22.01, we will disable the datatypes on the big - public servers during that release. This option will be switched - to False by default in 22.05 and this broken functionality will be - removed all together during some future release. -:Default: ``true`` -:Type: bool - - ~~~~~~~~~~~~~~~ ``watch_tools`` ~~~~~~~~~~~~~~~ diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index d1ed27adfc1..9471607da38 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -210,10 +210,18 @@ galaxy: #cache_dir: cache # By default, Galaxy uses a SQLite database at - # '/universe.sqlite'. You may use a SQLAlchemy connection + # '/universe.sqlite'. You may use a SQLAlchemy connection # string to specify an external database instead. # Sample default # 'sqlite:////universe.sqlite?isolation_level=IMMEDIATE' + # You may specify additional options that will be passed to the + # SQLAlchemy database engine by using the prefix + # "database_engine_option_". For some of these options, default values + # are provided (e.g. see database_engine_option_pool_size, etc.). + # The same applies to `install_database_connection`, for which you + # should use the "install_database_engine_option_" prefix. + # For more options, please check SQLAlchemy's documentation at + # https://docs.sqlalchemy.org/en/14/core/engines.html?highlight=create_engine#sqlalchemy.create_engine #database_connection: null # If the server logs errors about not having enough database pool diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index d73f00840e0..0b23066e75a 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -64,12 +64,22 @@ mapping: type: str required: false desc: | - By default, Galaxy uses a SQLite database at '/universe.sqlite'. You - may use a SQLAlchemy connection string to specify an external database - instead. + By default, Galaxy uses a SQLite database at '/universe.sqlite'. You + may use a SQLAlchemy connection string to specify an external database instead. Sample default 'sqlite:////universe.sqlite?isolation_level=IMMEDIATE' + You may specify additional options that will be passed to the SQLAlchemy + database engine by using the prefix "database_engine_option_". For some of these + options, default values are provided (e.g. see database_engine_option_pool_size, + etc.). + + The same applies to `install_database_connection`, for which you should use the + "install_database_engine_option_" prefix. + + For more options, please check SQLAlchemy's documentation at + https://docs.sqlalchemy.org/en/14/core/engines.html?highlight=create_engine#sqlalchemy.create_engine + database_engine_option_pool_size: type: int default: 5 From 7578edbd008ac2a422a68652baa48b80967a0893 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 14 Oct 2022 13:31:54 -0400 Subject: [PATCH 3/4] Fix mypy error --- test/unit/data/model/test_engine_factory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/data/model/test_engine_factory.py b/test/unit/data/model/test_engine_factory.py index 2453fe0e4e3..2d8cc3faa73 100644 --- a/test/unit/data/model/test_engine_factory.py +++ b/test/unit/data/model/test_engine_factory.py @@ -11,7 +11,7 @@ class TestSetSqliteConnectArgs: assert updated == {"connect_args": {"check_same_thread": False}} def test_engine_options_empty(self): - engine_options = {} + engine_options = {} # type: ignore[var-annotated] updated = set_sqlite_connect_args(engine_options, SQLITE_URL) assert updated == {"connect_args": {"check_same_thread": False}} From d957e143ea64bd6a7e1a07d0cfee9cfd5c0f8401 Mon Sep 17 00:00:00 2001 From: John Davis Date: Fri, 14 Oct 2022 15:27:20 -0400 Subject: [PATCH 4/4] Engine options are never null --- lib/galaxy/model/orm/engine_factory.py | 7 ++++--- test/unit/data/model/test_engine_factory.py | 5 ----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/model/orm/engine_factory.py b/lib/galaxy/model/orm/engine_factory.py index 98e177fd551..5a2b0c5be62 100644 --- a/lib/galaxy/model/orm/engine_factory.py +++ b/lib/galaxy/model/orm/engine_factory.py @@ -4,6 +4,7 @@ import os import threading import time from multiprocessing.util import register_after_fork +from typing import Dict from sqlalchemy import ( create_engine, @@ -99,6 +100,7 @@ def build_engine( except AttributeError: pass + engine_options = engine_options or {} engine_options = set_sqlite_connect_args(engine_options, url) engine = create_engine(url, **engine_options) @@ -122,14 +124,13 @@ def build_engine( return engine -def set_sqlite_connect_args(engine_options, url): +def set_sqlite_connect_args(engine_options: Dict, url: str): """ Add or update `connect_args` in `engine_options` if db is sqlite. Set check_same_thread to False for sqlite, handled by request-specific session. See https://fastapi.tiangolo.com/tutorial/sql-databases/#note """ - if url and url.startswith("sqlite://"): - engine_options = engine_options or {} + if url.startswith("sqlite://"): connect_args = engine_options.setdefault("connect_args", {}) connect_args["check_same_thread"] = False return engine_options diff --git a/test/unit/data/model/test_engine_factory.py b/test/unit/data/model/test_engine_factory.py index 2d8cc3faa73..947664865f3 100644 --- a/test/unit/data/model/test_engine_factory.py +++ b/test/unit/data/model/test_engine_factory.py @@ -5,11 +5,6 @@ NON_SQLITE_URL = "foo://foo.db" class TestSetSqliteConnectArgs: - def test_engine_options_none(self): - engine_options = None - updated = set_sqlite_connect_args(engine_options, SQLITE_URL) - assert updated == {"connect_args": {"check_same_thread": False}} - def test_engine_options_empty(self): engine_options = {} # type: ignore[var-annotated] updated = set_sqlite_connect_args(engine_options, SQLITE_URL)