From b54bf35518595aa886948e2c36f6949017c78f36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 28 Jun 2024 12:31:26 +0200 Subject: [PATCH 01/22] Support using a table in any database supported by SQLAlchemy as mapping for interactive tools Replace the class `InteractiveToolSqlite` in lib/galaxy/managers/interactivetool.py with a new class `InteractiveToolPropagatorSQLAlchemy`. The new class implements a SQLAlchemy "propagator" for `InteractiveToolManager` (on the same file). This propagator writes the mappings to the table named after the value of `DATABASE_TABLE_NAME`, in the database specified by the SQLAlchemy database url passed to its constructor. Change the constructor of `InteractiveToolManager` so that it uses `InteractiveToolPropagatorSQLAlchemy`. Change the method `_process_config` of `galaxy.config.GalaxyAppConfiguration` so that it converts the value of `interactivetools_map` to a SQLAlchemy database url if it is a path. Update documentation to reflect these changes. --- config/galaxy.yml.interactivetools | 2 + doc/source/admin/galaxy_options.rst | 9 +- .../admin/special_topics/interactivetools.rst | 2 + lib/galaxy/config/__init__.py | 9 +- lib/galaxy/config/sample/galaxy.yml.sample | 4 +- lib/galaxy/config/schemas/config_schema.yml | 6 +- lib/galaxy/managers/interactivetool.py | 201 +++++++++--------- 7 files changed, 128 insertions(+), 105 deletions(-) diff --git a/config/galaxy.yml.interactivetools b/config/galaxy.yml.interactivetools index add807ec9e5d..7504ab5b5002 100644 --- a/config/galaxy.yml.interactivetools +++ b/config/galaxy.yml.interactivetools @@ -12,6 +12,8 @@ gravity: galaxy: interactivetools_enable: true + # either a path to a SQLite database file or a SQLAlchemy database URL, in both cases the table "gxitproxy" on + # the target database is used interactivetools_map: database/interactivetools_map.sqlite # outputs_to_working_directory will provide you with a better level of isolation. It is highly recommended to set diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index cdf82ab688fd..8d77718b2f81 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2128,9 +2128,12 @@ ~~~~~~~~~~~~~~~~~~~~~~~~ :Description: - Map for interactivetool proxy. - The value of this option will be resolved with respect to - . + Map for interactivetool proxy. It may be either a path to a SQLite + database or a SQLAlchemy database URL (see + https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls). + In either case, mappings will be written to the table "gxitproxy" within the + database. If it is a path, the value of this option will be resolved with + respect to . :Default: ``interactivetools_map.sqlite`` :Type: str diff --git a/doc/source/admin/special_topics/interactivetools.rst b/doc/source/admin/special_topics/interactivetools.rst index 698c2d1d3d73..73fd0c00731c 100644 --- a/doc/source/admin/special_topics/interactivetools.rst +++ b/doc/source/admin/special_topics/interactivetools.rst @@ -80,6 +80,8 @@ Set these values in ``galaxy.yml``: galaxy: interactivetools_enable: true + # either a path to a SQLite database file or a SQLAlchemy database URL, in both cases the table "gxitproxy" on the + # target database is used interactivetools_map: database/interactivetools_map.sqlite # outputs_to_working_directory will provide you with a better level of isolation. It is highly recommended to set diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 554018472dc9..78b13f32d49e 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -1103,10 +1103,15 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.proxy_session_map = self.dynamic_proxy_session_map self.manage_dynamic_proxy = self.dynamic_proxy_manage # Set to false if being launched externally - # InteractiveTools propagator mapping file - self.interactivetools_map = self._in_root_dir( + # InteractiveTools propagator mapping + interactivetools_map = urlparse( kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) ) + self.interactivetools_map = ( + interactivetools_map.geturl() + if interactivetools_map.scheme + else "sqlite:///" + self._in_root_dir(interactivetools_map.geturl()) + ) # Compliance/Policy variables self.redact_username_during_deletion = False diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index bb4c6fc32d4f..80ca62bdcc1d 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -189,8 +189,8 @@ gravity: # Public-facing port of the proxy # port: 4002 - # Routes file to monitor. - # Should be set to the same path as ``interactivetools_map`` in the ``galaxy:`` section. This is ignored if + # Database to monitor. + # Should be set to the same value as ``interactivetools_map`` in the ``galaxy:`` section. This is ignored if # ``interactivetools_map is set``. # sessions: database/interactivetools_map.sqlite diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index e18dc2aa28e8..b27179d8bb5c 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -1518,9 +1518,11 @@ mapping: interactivetools_map: type: str default: interactivetools_map.sqlite - path_resolves_to: data_dir + path_resolves_to: data_dir # does not apply to SQLAlchemy database URLs desc: | - Map for interactivetool proxy. + Map for interactivetool proxy. It may be either a path to a SQLite database or a SQLALchemy database URL (see + https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls). In either case, mappings will be written + to the table "gxitproxy" within the database. interactivetools_prefix: type: str diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 8cec876ba561..9ff533829b3c 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -1,14 +1,15 @@ import json import logging -import sqlite3 from urllib.parse import ( urlsplit, urlunsplit, ) from sqlalchemy import ( + create_engine, or_, select, + text, ) from galaxy import exceptions @@ -18,94 +19,108 @@ ) from galaxy.model.base import transaction from galaxy.security.idencoding import IdAsLowercaseAlphanumEncodingHelper -from galaxy.util.filelock import FileLock log = logging.getLogger(__name__) DATABASE_TABLE_NAME = "gxitproxy" -class InteractiveToolSqlite: - def __init__(self, sqlite_filename, encode_id): - self.sqlite_filename = sqlite_filename - self.encode_id = encode_id +class InteractiveToolPropagatorSQLAlchemy: + """ + Propagator for InteractiveToolManager implemented using SQLAlchemy. + """ + + def __init__(self, database_url, encode_id): + """ + Constructor that sets up the propagator using a SQLAlchemy database URL. + + :param database_url: SQLAlchemy database URL, read more on the SQLAlchemy documentation + https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls. + :param encode_id: A helper class that can encode ids as lowercase alphanumeric strings and vice versa. + """ + self._engine = create_engine(database_url) + self._encode_id = encode_id def get(self, key, key_type): - with FileLock(self.sqlite_filename): - conn = sqlite3.connect(self.sqlite_filename) - try: - c = conn.cursor() - select = f"""SELECT token, host, port, info - FROM {DATABASE_TABLE_NAME} - WHERE key=? and key_type=?""" - c.execute( - select, - ( - key, - key_type, - ), - ) - try: - token, host, port, info = c.fetchone() - except TypeError: - log.warning("get(): invalid key: %s key_type %s", key, key_type) - return None - return dict(key=key, key_type=key_type, token=token, host=host, port=port, info=info) - finally: - conn.close() + with self._engine.connect() as conn: + query = text( + f""" + SELECT token, host, port, info + FROM {DATABASE_TABLE_NAME} WHERE key=:key AND key_type=:key_type + """ + ) + parameters = dict( + key=key, + key_type=key_type, + ) + result = conn.execute(query, parameters).fetchone() + return ( + None + if result is None + else dict( + key=key, + key_type=key_type, + token=result[0], + host=result[1], + port=result[2], + info=result[3], + ) + ) def save(self, key, key_type, token, host, port, info=None): """ - Writeout a key, key_type, token, value store that is can be used for coordinating - with external resources. + Write out a key, key_type, token, value store that is can be used for coordinating with external resources. """ assert key, ValueError("A non-zero length key is required.") assert key_type, ValueError("A non-zero length key_type is required.") assert token, ValueError("A non-zero length token is required.") - with FileLock(self.sqlite_filename): - conn = sqlite3.connect(self.sqlite_filename) - try: - c = conn.cursor() - try: - # Create table - c.execute( - f"""CREATE TABLE {DATABASE_TABLE_NAME} - (key text, - key_type text, - token text, - host text, - port integer, - info text, - PRIMARY KEY (key, key_type) - )""" - ) - except Exception: - pass - delete = f"""DELETE FROM {DATABASE_TABLE_NAME} WHERE key=? and key_type=?""" - c.execute( - delete, - ( - key, - key_type, - ), - ) - insert = f"""INSERT INTO {DATABASE_TABLE_NAME} - (key, key_type, token, host, port, info) - VALUES (?, ?, ?, ?, ?, ?)""" - c.execute( - insert, - ( - key, - key_type, - token, - host, - port, - info, - ), - ) - conn.commit() - finally: - conn.close() + with self._engine.connect() as conn: + # create gx-it-proxy table if not exists + query = text( + f""" + CREATE TABLE IF NOT EXISTS {DATABASE_TABLE_NAME} ( + key TEXT, + key_type TEXT, + token TEXT, + host TEXT, + port INTEGER, + info TEXT, + PRIMARY KEY (key, key_type) + ) + """ + ) + conn.execute(query) + + # delete existing data with same key + query = text( + f""" + DELETE FROM {DATABASE_TABLE_NAME} WHERE key=:key AND key_type=:key_type + """ + ) + parameters = dict( + key=key, + key_type=key_type, + ) + conn.execute(query, parameters) + + # save data + query = text( + f""" + INSERT INTO {DATABASE_TABLE_NAME} (key, key_type, token, host, port, info) + VALUES (:key, :key_type, :token, :host, :port, :info) + """ + ) + parameters = dict( + key=key, + key_type=key_type, + token=token, + host=host, + port=port, + info=info, + ) + conn.execute(query, parameters) + + conn.commit() def remove(self, **kwd): """ @@ -113,30 +128,24 @@ def remove(self, **kwd): with external resources. Remove entries that match all provided key=values """ assert kwd, ValueError("You must provide some values to key upon") - delete = f"DELETE FROM {DATABASE_TABLE_NAME} WHERE" - value_list = [] - for i, (key, value) in enumerate(kwd.items()): - if i != 0: - delete += " and" - delete += f" {key}=?" - value_list.append(value) - with FileLock(self.sqlite_filename): - conn = sqlite3.connect(self.sqlite_filename) - try: - c = conn.cursor() - try: - # Delete entry - c.execute(delete, tuple(value_list)) - except Exception as e: - log.debug("Error removing entry (%s): %s", delete, e) - conn.commit() - finally: - conn.close() + with self._engine.connect() as conn: + query_template = f"DELETE FROM {DATABASE_TABLE_NAME} WHERE {{conditions}}" + conditions = ( + "{key}=:{value}".format( + key=key, + value=f"value_{i}", + ) + for i, key in enumerate(kwd) + ) + query = text(query_template.format(conditions=" AND ".join(conditions))) + parameters = {f"value_{i}": value for i, value in enumerate(kwd.values())} + conn.execute(query, parameters) + conn.commit() def save_entry_point(self, entry_point): """Convenience method to easily save an entry_point.""" return self.save( - self.encode_id(entry_point.id), + self._encode_id(entry_point.id), entry_point.__class__.__name__.lower(), entry_point.token, entry_point.host, @@ -151,7 +160,7 @@ def save_entry_point(self, entry_point): def remove_entry_point(self, entry_point): """Convenience method to easily remove an entry_point.""" - return self.remove(key=self.encode_id(entry_point.id), key_type=entry_point.__class__.__name__.lower()) + return self.remove(key=self._encode_id(entry_point.id), key_type=entry_point.__class__.__name__.lower()) class InteractiveToolManager: @@ -166,7 +175,7 @@ def __init__(self, app): self.sa_session = app.model.context self.job_manager = app.job_manager self.encoder = IdAsLowercaseAlphanumEncodingHelper(app.security) - self.propagator = InteractiveToolSqlite(app.config.interactivetools_map, self.encoder.encode_id) + self.propagator = InteractiveToolPropagatorSQLAlchemy(app.config.interactivetools_map, self.encoder.encode_id) def create_entry_points(self, job, tool, entry_points=None, flush=True): entry_points = entry_points or tool.ports From bf6b6edd71ac00960be238f4dd300695d8d98e27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Thu, 4 Jul 2024 10:39:15 +0200 Subject: [PATCH 02/22] Fix bug when using a mapping for interactive tools from a SQLAlchemy database url --- lib/galaxy/config/__init__.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 78b13f32d49e..7648bec93e35 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -1104,13 +1104,11 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.manage_dynamic_proxy = self.dynamic_proxy_manage # Set to false if being launched externally # InteractiveTools propagator mapping - interactivetools_map = urlparse( - kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) - ) + interactivetools_map = kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) self.interactivetools_map = ( - interactivetools_map.geturl() - if interactivetools_map.scheme - else "sqlite:///" + self._in_root_dir(interactivetools_map.geturl()) + interactivetools_map + if urlparse(interactivetools_map).scheme + else "sqlite:///" + self._in_root_dir(interactivetools_map) ) # Compliance/Policy variables From d85e1b5b65f26a25011a9285175d044dfa098007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Wed, 17 Jul 2024 15:46:32 +0200 Subject: [PATCH 03/22] Replace raw SQL queries with SQLAlchemy Core SQL expressions --- lib/galaxy/managers/interactivetool.py | 103 +++++++++---------------- 1 file changed, 36 insertions(+), 67 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 9ff533829b3c..8cffa36c5690 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -6,10 +6,17 @@ ) from sqlalchemy import ( + Column, create_engine, + delete, + insert, + Integer, + MetaData, or_, select, - text, + String, + Table, + Text, ) from galaxy import exceptions @@ -22,7 +29,16 @@ log = logging.getLogger(__name__) -DATABASE_TABLE_NAME = "gxitproxy" +gxitproxy = Table( + "gxitproxy", + MetaData(), + Column("key", String(16), primary_key=True), + Column("key_type", Text(), primary_key=True), + Column("token", String(32)), + Column("host", Text()), + Column("port", Integer()), + Column("info", Text()), +) class InteractiveToolPropagatorSQLAlchemy: @@ -39,33 +55,20 @@ def __init__(self, database_url, encode_id): :param encode_id: A helper class that can encode ids as lowercase alphanumeric strings and vice versa. """ self._engine = create_engine(database_url) + gxitproxy.create(self._engine, checkfirst=True) self._encode_id = encode_id def get(self, key, key_type): + columns = ("token", "host", "port", "info") + with self._engine.connect() as conn: - query = text( - f""" - SELECT token, host, port, info - FROM {DATABASE_TABLE_NAME} WHERE key=:key AND key_type=:key_type - """ - ) - parameters = dict( - key=key, - key_type=key_type, + query = select(gxitproxy.c[columns]).where( + gxitproxy.c["key"] == key, + gxitproxy.c["key_type"] == key_type, ) - result = conn.execute(query, parameters).fetchone() - return ( - None - if result is None - else dict( - key=key, - key_type=key_type, - token=result[0], - host=result[1], - port=result[2], - info=result[3], - ) - ) + result = conn.execute(query).fetchone() + + return dict(key=key, key_type=key_type, **dict(zip(columns, result))) if result else None def save(self, key, key_type, token, host, port, info=None): """ @@ -75,42 +78,15 @@ def save(self, key, key_type, token, host, port, info=None): assert key_type, ValueError("A non-zero length key_type is required.") assert token, ValueError("A non-zero length token is required.") with self._engine.connect() as conn: - # create gx-it-proxy table if not exists - query = text( - f""" - CREATE TABLE IF NOT EXISTS {DATABASE_TABLE_NAME} ( - key TEXT, - key_type TEXT, - token TEXT, - host TEXT, - port INTEGER, - info TEXT, - PRIMARY KEY (key, key_type) - ) - """ - ) - conn.execute(query) - # delete existing data with same key - query = text( - f""" - DELETE FROM {DATABASE_TABLE_NAME} WHERE key=:key AND key_type=:key_type - """ - ) - parameters = dict( - key=key, - key_type=key_type, + query_delete = delete(gxitproxy).where( + gxitproxy.c["key"] == key, + gxitproxy.c["key_type"] == key_type, ) - conn.execute(query, parameters) + conn.execute(query_delete) # save data - query = text( - f""" - INSERT INTO {DATABASE_TABLE_NAME} (key, key_type, token, host, port, info) - VALUES (:key, :key_type, :token, :host, :port, :info) - """ - ) - parameters = dict( + query_insert = insert(gxitproxy).values( key=key, key_type=key_type, token=token, @@ -118,7 +94,7 @@ def save(self, key, key_type, token, host, port, info=None): port=port, info=info, ) - conn.execute(query, parameters) + conn.execute(query_insert) conn.commit() @@ -129,17 +105,10 @@ def remove(self, **kwd): """ assert kwd, ValueError("You must provide some values to key upon") with self._engine.connect() as conn: - query_template = f"DELETE FROM {DATABASE_TABLE_NAME} WHERE {{conditions}}" - conditions = ( - "{key}=:{value}".format( - key=key, - value=f"value_{i}", - ) - for i, key in enumerate(kwd) + query = delete(gxitproxy).where( + *(gxitproxy.c[key] == value for key, value in kwd.items()), ) - query = text(query_template.format(conditions=" AND ".join(conditions))) - parameters = {f"value_{i}": value for i, value in enumerate(kwd.values())} - conn.execute(query, parameters) + conn.execute(query) conn.commit() def save_entry_point(self, entry_point): From ce6d0f73c51c61b55d6fa4b41f9ba7a8827ce13a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Thu, 18 Jul 2024 09:51:47 +0200 Subject: [PATCH 04/22] Configure SQLAlchemy interactive tool maps via new `interactivetools_map_sqlalchemy` setting Define a new configuration property `interactivetools_map_sqlalchemy` to store interactivetool proxy mappings on a RDBMS supported by SQLAlchemy. If set, it takes precedence over `interactivetools_map`. When unset, a SQLite mapping file located at `interactivetools_map` is used (as usual). --- config/galaxy.yml.interactivetools | 2 -- doc/source/admin/galaxy_options.rst | 29 +++++++++++++++---- .../admin/special_topics/interactivetools.rst | 2 -- lib/galaxy/config/__init__.py | 10 +++---- lib/galaxy/config/sample/galaxy.yml.sample | 20 +++++++++++-- lib/galaxy/config/schemas/config_schema.yml | 19 +++++++++--- lib/galaxy/managers/interactivetool.py | 5 +++- 7 files changed, 64 insertions(+), 23 deletions(-) diff --git a/config/galaxy.yml.interactivetools b/config/galaxy.yml.interactivetools index 7504ab5b5002..add807ec9e5d 100644 --- a/config/galaxy.yml.interactivetools +++ b/config/galaxy.yml.interactivetools @@ -12,8 +12,6 @@ gravity: galaxy: interactivetools_enable: true - # either a path to a SQLite database file or a SQLAlchemy database URL, in both cases the table "gxitproxy" on - # the target database is used interactivetools_map: database/interactivetools_map.sqlite # outputs_to_working_directory will provide you with a better level of isolation. It is highly recommended to set diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index 8d77718b2f81..785e3b83aeaf 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2128,16 +2128,33 @@ ~~~~~~~~~~~~~~~~~~~~~~~~ :Description: - Map for interactivetool proxy. It may be either a path to a SQLite - database or a SQLAlchemy database URL (see - https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls). - In either case, mappings will be written to the table "gxitproxy" within the - database. If it is a path, the value of this option will be resolved with - respect to . + Map for the interactivetool proxy. Mappings are stored on a SQLite + database file located on this path. As an alternative, you may + also store them on any other RDBMS supported by SQLAlchemy using + the option ``interactivetools_map_sqlalchemy``. + The value of this option will be resolved with respect to + . :Default: ``interactivetools_map.sqlite`` :Type: str +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +``interactivetools_map_sqlalchemy`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +:Description: + Use a database supported by SQLAlchemy as map for the + interactivetool proxy. When this option is set, the value of + ``interactivetools_map`` is ignored. The value of this option must + be a `SQLAlchemy database URL + `_. + Mappings are written to the table "gxitproxy" within the database. + This value cannot match ``database_connection`` nor + ``install_database_connection``. +:Default: ``None`` +:Type: str + + ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``interactivetools_prefix`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/doc/source/admin/special_topics/interactivetools.rst b/doc/source/admin/special_topics/interactivetools.rst index da75caec2095..6d032c02d05c 100644 --- a/doc/source/admin/special_topics/interactivetools.rst +++ b/doc/source/admin/special_topics/interactivetools.rst @@ -80,8 +80,6 @@ Set these values in ``galaxy.yml``: galaxy: interactivetools_enable: true - # either a path to a SQLite database file or a SQLAlchemy database URL, in both cases the table "gxitproxy" on the - # target database is used interactivetools_map: database/interactivetools_map.sqlite # outputs_to_working_directory will provide you with a better level of isolation. It is highly recommended to set diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 7648bec93e35..a8dd2470f8f7 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -33,6 +33,7 @@ from urllib.parse import urlparse import yaml +from sqlalchemy.engine import make_url as parse_sqlalchemy_url from galaxy.config.schema import AppSchema from galaxy.exceptions import ConfigurationError @@ -1104,11 +1105,8 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.manage_dynamic_proxy = self.dynamic_proxy_manage # Set to false if being launched externally # InteractiveTools propagator mapping - interactivetools_map = kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) - self.interactivetools_map = ( - interactivetools_map - if urlparse(interactivetools_map).scheme - else "sqlite:///" + self._in_root_dir(interactivetools_map) + self.interactivetools_map = "sqlite:///" + self._in_root_dir( + kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) ) # Compliance/Policy variables @@ -1230,6 +1228,8 @@ def try_parsing(value, name): try_parsing(self.database_connection, "database_connection") try_parsing(self.install_database_connection, "install_database_connection") + if self.interactivetools_map_sqlalchemy is not None: + try_parsing(self.interactivetools_map_sqlalchemy, "interactivetools_map_sqlalchemy") try_parsing(self.amqp_internal_connection, "amqp_internal_connection") def _configure_dataset_storage(self): diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index 80ca62bdcc1d..9212cb4aa463 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -190,8 +190,9 @@ gravity: # port: 4002 # Database to monitor. - # Should be set to the same value as ``interactivetools_map`` in the ``galaxy:`` section. This is ignored if - # ``interactivetools_map is set``. + # Should be set to the same value as ``interactivetools_map`` (or ``interactivetools_map_sqlalchemy``) in the + # ``galaxy:`` section. This is ignored if either ``interactivetools_map`` or ``interactivetools_map_sqlalchemy`` are + # set. # sessions: database/interactivetools_map.sqlite # Include verbose messages in gx-it-proxy @@ -1357,11 +1358,24 @@ galaxy: # subdomain. Defaults to "/". #interactivetools_base_path: / - # Map for interactivetool proxy. + # Map for the interactivetool proxy. Mappings are stored on a SQLite + # database file located on this path. As an alternative, you may also + # store them on any other RDBMS supported by SQLAlchemy using the + # option ``interactivetools_map_sqlalchemy``. # The value of this option will be resolved with respect to # . #interactivetools_map: interactivetools_map.sqlite + # Use a database supported by SQLAlchemy as map for the + # interactivetool proxy. When this option is set, the value of + # ``interactivetools_map`` is ignored. The value of this option must + # be a `SQLAlchemy database URL + # `_. + # Mappings are written to the table "gxitproxy" within the database. + # This value cannot match ``database_connection`` nor + # ``install_database_connection``. + #interactivetools_map_sqlalchemy: null + # Prefix to use in the formation of the subdomain or path for # interactive tools #interactivetools_prefix: interactivetool diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index a43e592e4b10..9d629fc2fda7 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -1518,11 +1518,22 @@ mapping: interactivetools_map: type: str default: interactivetools_map.sqlite - path_resolves_to: data_dir # does not apply to SQLAlchemy database URLs + path_resolves_to: data_dir + desc: | + Map for the interactivetool proxy. Mappings are stored on a SQLite database file + located on this path. As an alternative, you may also store them on any other RDBMS + supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``. + + interactivetools_map_sqlalchemy: + type: str + required: false desc: | - Map for interactivetool proxy. It may be either a path to a SQLite database or a SQLALchemy database URL (see - https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls). In either case, mappings will be written - to the table "gxitproxy" within the database. + Use a database supported by SQLAlchemy as map for the interactivetool proxy. + When this option is set, the value of ``interactivetools_map`` is ignored. The + value of this option must be a + `SQLAlchemy database URL `_. + Mappings are written to the table "gxitproxy" within the database. This value cannot match + ``database_connection`` nor ``install_database_connection``. interactivetools_prefix: type: str diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 8cffa36c5690..5417b4fcaa2c 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -144,7 +144,10 @@ def __init__(self, app): self.sa_session = app.model.context self.job_manager = app.job_manager self.encoder = IdAsLowercaseAlphanumEncodingHelper(app.security) - self.propagator = InteractiveToolPropagatorSQLAlchemy(app.config.interactivetools_map, self.encoder.encode_id) + self.propagator = InteractiveToolPropagatorSQLAlchemy( + app.config.interactivetools_map_sqlalchemy or app.config.interactivetools_map, + self.encoder.encode_id, + ) def create_entry_points(self, job, tool, entry_points=None, flush=True): entry_points = entry_points or tool.ports From 647fd4c060db36e90a7f930e7cd11311953a3895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Thu, 18 Jul 2024 10:30:13 +0200 Subject: [PATCH 05/22] Revert changes to description for setting `sessions` in galaxy.yml.sample This text is managed by Gravity --- lib/galaxy/config/sample/galaxy.yml.sample | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index 9212cb4aa463..ca8933333c4a 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -189,10 +189,9 @@ gravity: # Public-facing port of the proxy # port: 4002 - # Database to monitor. - # Should be set to the same value as ``interactivetools_map`` (or ``interactivetools_map_sqlalchemy``) in the - # ``galaxy:`` section. This is ignored if either ``interactivetools_map`` or ``interactivetools_map_sqlalchemy`` are - # set. + # Routes file to monitor. + # Should be set to the same path as ``interactivetools_map`` in the ``galaxy:`` section. This is ignored if + # ``interactivetools_map is set. # sessions: database/interactivetools_map.sqlite # Include verbose messages in gx-it-proxy From 678bea108597244c7906f9a2740f00a870814bd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Thu, 18 Jul 2024 11:05:55 +0200 Subject: [PATCH 06/22] Forbid setting `interactivetools_map_sqlalchemy` from matching `database_connection` or `install_database_connection` --- lib/galaxy/config/__init__.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index a8dd2470f8f7..0aea97c31fc3 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -1108,6 +1108,32 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.interactivetools_map = "sqlite:///" + self._in_root_dir( kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) ) + if self.interactivetools_map_sqlalchemy: + # ensure the database URL for the SQLAlchemy map does not match that of a Galaxy DB + urls = { + setting: parse_sqlalchemy_url(value) + for setting, value in ( + ("interactivetools_map_sqlalchemy", self.interactivetools_map_sqlalchemy), + ("database_connection", self.database_connection), + ("install_database_connection", self.install_database_connection), + ) + if value is not None + } + + def is_in_conflict(url1, url2): + return all((url1.host == url2.host, url1.port == url2.port, url1.database == url2.database)) + + conflicting_settings = { + setting + for setting, url in tuple(urls.items())[1:] # exclude "interactivetools_map_sqlalchemy" + if is_in_conflict(url, list(urls.values())[0]) # compare with "interactivetools_map_sqlalchemy" + } + + if conflicting_settings: + raise ConfigurationError( + f"Option `{tuple(urls)[0]}` cannot take the same value as: %s" + % ", ".join(f"`{setting}`" for setting in conflicting_settings) + ) # Compliance/Policy variables self.redact_username_during_deletion = False From dddf5ba9f698920b933dffaa7b7e4aa27791ea89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Thu, 18 Jul 2024 15:04:02 +0200 Subject: [PATCH 07/22] Add note in interactive tools docs page informing about `interactivetools_map_sqlalchemy` setting Refer to SQLite docs page describing situations where a client/server RDBMS works better and provide an example. Link to the `interactivetools_map_sqlalchemy` on the Galaxy configuration page. --- .../admin/special_topics/interactivetools.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/doc/source/admin/special_topics/interactivetools.rst b/doc/source/admin/special_topics/interactivetools.rst index 6d032c02d05c..e621edc205ac 100644 --- a/doc/source/admin/special_topics/interactivetools.rst +++ b/doc/source/admin/special_topics/interactivetools.rst @@ -102,6 +102,19 @@ The ``gx-it-proxy`` config relates to an important service in the InteractiveToo proxy. ``gx-it-proxy`` runs as a separate process listening at port 4002 (by default). HTTP requests are decoded based on the URL and headers, then somewhat massaged, and finally forwarded to the correct entry point port of the target InteractiveTool. +.. note:: + + Entry point mappings used by the proxy are stored on a SQLite database file located at ``interactivetools_map``. In + `some situations `_, + SQLite may not be the best choice. A common case is a high-availability production setup, meaning that multiple + copies of Galaxy are running on different servers behind a load balancer. + + For these situations, there exists an optional |configuration option interactivetools_map_sqlalchemy|_ that allows + using any database supported by SQLAlchemy (it overrides ``interactivetools_map``). + +.. |configuration option interactivetools_map_sqlalchemy| replace:: configuration option ``interactivetools_map_sqlalchemy`` +.. _configuration option interactivetools_map_sqlalchemy: ../config.html#interactivetools-map-sqlalchemy + .. note:: A previous config option ``interactivetools_shorten_url`` was removed in commit `#73100de `_ From 6be9b60cea2c903fc0082e253ed785167a5fb60a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Mon, 22 Jul 2024 17:16:18 +0200 Subject: [PATCH 08/22] workaround `mypy` error on statement using `sqlalchemy.select` ``` lib/galaxy/managers/interactivetool.py:65: error: No overload variant of "select" matches argument type "ReadOnlyColumnCollection[str, Column[Any]]" ``` --- lib/galaxy/managers/interactivetool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 5417b4fcaa2c..21a79d9421b6 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -62,7 +62,7 @@ def get(self, key, key_type): columns = ("token", "host", "port", "info") with self._engine.connect() as conn: - query = select(gxitproxy.c[columns]).where( + query = select(*gxitproxy.c[columns]).where( gxitproxy.c["key"] == key, gxitproxy.c["key_type"] == key_type, ) From e7dbe42f07aa786c6d65ec56754f083e6671f8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Tue, 23 Jul 2024 09:05:49 +0200 Subject: [PATCH 09/22] Create `gxitproxy` database table when using interactive tools for the first time --- lib/galaxy/managers/interactivetool.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 21a79d9421b6..43c0e410edab 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -55,7 +55,6 @@ def __init__(self, database_url, encode_id): :param encode_id: A helper class that can encode ids as lowercase alphanumeric strings and vice versa. """ self._engine = create_engine(database_url) - gxitproxy.create(self._engine, checkfirst=True) self._encode_id = encode_id def get(self, key, key_type): @@ -78,6 +77,9 @@ def save(self, key, key_type, token, host, port, info=None): assert key_type, ValueError("A non-zero length key_type is required.") assert token, ValueError("A non-zero length token is required.") with self._engine.connect() as conn: + # create database table if not exists + gxitproxy.create(conn, checkfirst=True) + # delete existing data with same key query_delete = delete(gxitproxy).where( gxitproxy.c["key"] == key, From b6ad695d0377f28e0eb492a95d260d1e2be127b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 2 Aug 2024 15:30:25 +0200 Subject: [PATCH 10/22] Add `sqlalchemy` to config package requirements `lib.galaxy.config.GalaxyAppConfiguration._process_config` requires `sqlalchemy.engine.make_url` to forbid `interactivetools_map_sqlalchemy` from matching `database_connection` or `install_database_connection` (678bea1) --- packages/config/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/config/setup.cfg b/packages/config/setup.cfg index 5249de2ee6ab..91aaf36d6a5b 100644 --- a/packages/config/setup.cfg +++ b/packages/config/setup.cfg @@ -37,6 +37,7 @@ install_requires = jinja2 pykwalify PyYAML + sqlalchemy~=2.0 packages = find: python_requires = >=3.8 From 5e8ca551ce3ee14e358138c0fde264a8d9dac5af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 2 Aug 2024 15:46:27 +0200 Subject: [PATCH 11/22] Test that Galaxy raises `ConfigurationError` when setting `interactivetools_map_sqlalchemy` matches `database_connection` or `install_database_connection` (678bea1) --- test/unit/config/test_config_values.py | 38 ++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/unit/config/test_config_values.py b/test/unit/config/test_config_values.py index 2f00824839d0..668ad766ae8b 100644 --- a/test/unit/config/test_config_values.py +++ b/test/unit/config/test_config_values.py @@ -67,6 +67,44 @@ def test_error_if_database_connection_contains_brackets(bracket): config.GalaxyAppConfiguration(override_tempdir=False, amqp_internal_connection=uri) +def test_error_if_interactivetools_map_sqlalchemy_matches_other_database_connections(): + """ + The setting `interactivetools_map_sqlalchemy` allows storing the session map in a + database supported by SQLAlchemy. This database must be different from the Galaxy database + and the tool shed database. + + Motivation for this constraint: + https://github.com/galaxyproject/galaxy/pull/18481#issuecomment-2218493956 + """ + database_connection = "dbscheme://user:password@host/db" + install_database_connection = "dbscheme://user:password@host/install_db" + settings = dict( + override_tempdir=False, + database_connection=database_connection, + install_database_connection=install_database_connection, + ) + + with pytest.raises(ConfigurationError): + # interactivetools_map_sqlalchemy matches database_connection + config.GalaxyAppConfiguration( + **settings, + interactivetools_map_sqlalchemy=database_connection, + ) + + with pytest.raises(ConfigurationError): + # interactivetools_map_sqlalchemy matches install_database_connection + config.GalaxyAppConfiguration( + **settings, + interactivetools_map_sqlalchemy=install_database_connection, + ) + + # interactivetools_map_sqlalchemy differs from database_connection, install_database_connection + config.GalaxyAppConfiguration( + **settings, + interactivetools_map_sqlalchemy="dbscheme://user:password@host/gxitproxy", + ) + + class TestIsFetchWithCeleryEnabled: def test_disabled_if_celery_disabled(self, appconfig): appconfig.enable_celery_tasks = False From 26fd210ca1874c3527aa30a57a0d93c7c2bbe3c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 09:34:54 +0200 Subject: [PATCH 12/22] Set `interactivetools_map` to `None` when `interactivetools_map_sqlalchemy` is defined Given that `interactivetools_map` is ignored if `interactivetools_map_sqlalchemy` is set, first check `interactivetools_map_sqlalchemy`. If it's not set, then build the sqlite url; but if it is set, then set `interactivetools_map` to `None` for clarity (and to avoid potential bugs in the future) Co-authored-by: John Davis --- lib/galaxy/config/__init__.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 0aea97c31fc3..2a27a19140aa 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -1105,10 +1105,13 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.manage_dynamic_proxy = self.dynamic_proxy_manage # Set to false if being launched externally # InteractiveTools propagator mapping - self.interactivetools_map = "sqlite:///" + self._in_root_dir( - kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) - ) - if self.interactivetools_map_sqlalchemy: + if self.interactivetools_map_sqlalchemy is None: + self.interactivetools_map = "sqlite:///" + self._in_root_dir( + kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) + ) + else: + self.interactivetools_map = None # overridden by `self.interactivetools_map_sqlalchemy` + # ensure the database URL for the SQLAlchemy map does not match that of a Galaxy DB urls = { setting: parse_sqlalchemy_url(value) From ffa8ea4f4da6a0ac3afba22fee504ff67c5a4678 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 09:42:30 +0200 Subject: [PATCH 13/22] Fix typos in descriptions of `interactivetools_map` and `interactivetools_map_sqlalchemy` Authored-by: John Davis --- doc/source/admin/galaxy_options.rst | 4 ++-- lib/galaxy/config/sample/galaxy.yml.sample | 4 ++-- lib/galaxy/config/schemas/config_schema.yml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index b9c9b951d752..660816c73ff2 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2112,9 +2112,9 @@ ~~~~~~~~~~~~~~~~~~~~~~~~ :Description: - Map for the interactivetool proxy. Mappings are stored on a SQLite + Map for the interactivetool proxy. Mappings are stored in a SQLite database file located on this path. As an alternative, you may - also store them on any other RDBMS supported by SQLAlchemy using + also store them in any other RDBMS supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``. The value of this option will be resolved with respect to . diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index e34c698043d7..922e39521c2a 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -1348,9 +1348,9 @@ galaxy: # subdomain. Defaults to "/". #interactivetools_base_path: / - # Map for the interactivetool proxy. Mappings are stored on a SQLite + # Map for the interactivetool proxy. Mappings are stored in a SQLite # database file located on this path. As an alternative, you may also - # store them on any other RDBMS supported by SQLAlchemy using the + # store them in any other RDBMS supported by SQLAlchemy using the # option ``interactivetools_map_sqlalchemy``. # The value of this option will be resolved with respect to # . diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index 7695bf111a64..ccb4e798bf1b 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -1520,8 +1520,8 @@ mapping: default: interactivetools_map.sqlite path_resolves_to: data_dir desc: | - Map for the interactivetool proxy. Mappings are stored on a SQLite database file - located on this path. As an alternative, you may also store them on any other RDBMS + Map for the interactivetool proxy. Mappings are stored in a SQLite database file + located on this path. As an alternative, you may also store them in any other RDBMS supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``. interactivetools_map_sqlalchemy: From 018e5417c23517fddc190008c45ac3c384ce4315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 09:50:47 +0200 Subject: [PATCH 14/22] Mention that `interactivetools_map_sqlalchemy` overrides `interactivetools_map` in the latter's description Co-authored-by: John Davis --- doc/source/admin/galaxy_options.rst | 3 ++- lib/galaxy/config/sample/galaxy.yml.sample | 3 ++- lib/galaxy/config/schemas/config_schema.yml | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index 660816c73ff2..f232622ece02 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2115,7 +2115,8 @@ Map for the interactivetool proxy. Mappings are stored in a SQLite database file located on this path. As an alternative, you may also store them in any other RDBMS supported by SQLAlchemy using - the option ``interactivetools_map_sqlalchemy``. + the option ``interactivetools_map_sqlalchemy``, which overrides + this one. The value of this option will be resolved with respect to . :Default: ``interactivetools_map.sqlite`` diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index 922e39521c2a..faeee1c70515 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -1351,7 +1351,8 @@ galaxy: # Map for the interactivetool proxy. Mappings are stored in a SQLite # database file located on this path. As an alternative, you may also # store them in any other RDBMS supported by SQLAlchemy using the - # option ``interactivetools_map_sqlalchemy``. + # option ``interactivetools_map_sqlalchemy``, which overrides this + # one. # The value of this option will be resolved with respect to # . #interactivetools_map: interactivetools_map.sqlite diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index ccb4e798bf1b..2d48d7991b94 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -1522,7 +1522,8 @@ mapping: desc: | Map for the interactivetool proxy. Mappings are stored in a SQLite database file located on this path. As an alternative, you may also store them in any other RDBMS - supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``. + supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``, which + overrides this one. interactivetools_map_sqlalchemy: type: str From e040750ac14740237c4878629f888efe75c2c1a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 10:10:42 +0200 Subject: [PATCH 15/22] Refactor variable name `query` to `stmt` for calls to `select()`, `insert()` or `delete()` --- lib/galaxy/managers/interactivetool.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 43c0e410edab..f4345637a4fe 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -61,11 +61,11 @@ def get(self, key, key_type): columns = ("token", "host", "port", "info") with self._engine.connect() as conn: - query = select(*gxitproxy.c[columns]).where( + stmt = select(*gxitproxy.c[columns]).where( gxitproxy.c["key"] == key, gxitproxy.c["key_type"] == key_type, ) - result = conn.execute(query).fetchone() + result = conn.execute(stmt).fetchone() return dict(key=key, key_type=key_type, **dict(zip(columns, result))) if result else None @@ -81,14 +81,14 @@ def save(self, key, key_type, token, host, port, info=None): gxitproxy.create(conn, checkfirst=True) # delete existing data with same key - query_delete = delete(gxitproxy).where( + stmt_delete = delete(gxitproxy).where( gxitproxy.c["key"] == key, gxitproxy.c["key_type"] == key_type, ) - conn.execute(query_delete) + conn.execute(stmt_delete) # save data - query_insert = insert(gxitproxy).values( + stmt_insert = insert(gxitproxy).values( key=key, key_type=key_type, token=token, @@ -96,7 +96,7 @@ def save(self, key, key_type, token, host, port, info=None): port=port, info=info, ) - conn.execute(query_insert) + conn.execute(stmt_insert) conn.commit() @@ -107,10 +107,10 @@ def remove(self, **kwd): """ assert kwd, ValueError("You must provide some values to key upon") with self._engine.connect() as conn: - query = delete(gxitproxy).where( + stmt = delete(gxitproxy).where( *(gxitproxy.c[key] == value for key, value in kwd.items()), ) - conn.execute(query) + conn.execute(stmt) conn.commit() def save_entry_point(self, entry_point): From 830501672c7d4205c789990d6e88517e83a70da0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 10:30:32 +0200 Subject: [PATCH 16/22] Access individual table columns via `__getattr__` rather than `__getitem__` Match @jdavcs' coding style. --- lib/galaxy/managers/interactivetool.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index f4345637a4fe..c9d01c0d9331 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -62,8 +62,8 @@ def get(self, key, key_type): with self._engine.connect() as conn: stmt = select(*gxitproxy.c[columns]).where( - gxitproxy.c["key"] == key, - gxitproxy.c["key_type"] == key_type, + gxitproxy.c.key == key, + gxitproxy.c.key_type == key_type, ) result = conn.execute(stmt).fetchone() @@ -82,8 +82,8 @@ def save(self, key, key_type, token, host, port, info=None): # delete existing data with same key stmt_delete = delete(gxitproxy).where( - gxitproxy.c["key"] == key, - gxitproxy.c["key_type"] == key_type, + gxitproxy.c.key == key, + gxitproxy.c.key_type == key_type, ) conn.execute(stmt_delete) From 6fc0d462cef51a392a164a852b80fed8de192ee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 11:24:53 +0200 Subject: [PATCH 17/22] Restore original error handling for `InteractiveToolPropagatorSQLAlchemy.get()` Emit a warning when `get()` returns no results. --- lib/galaxy/managers/interactivetool.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index c9d01c0d9331..588397bf54b9 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -58,16 +58,18 @@ def __init__(self, database_url, encode_id): self._encode_id = encode_id def get(self, key, key_type): - columns = ("token", "host", "port", "info") - with self._engine.connect() as conn: - stmt = select(*gxitproxy.c[columns]).where( + stmt = select(*gxitproxy.c["token", "host", "port", "info"]).where( gxitproxy.c.key == key, gxitproxy.c.key_type == key_type, ) - result = conn.execute(stmt).fetchone() + cursor_result = conn.execute(stmt) - return dict(key=key, key_type=key_type, **dict(zip(columns, result))) if result else None + try: + return dict(key=key, key_type=key_type, **dict(zip(cursor_result.keys(), cursor_result.fetchone()))) + except TypeError: # when `cursor_result.fetchone() is None` (no results) + log.warning("get(): invalid key: %s key_type %s", key, key_type) + return None def save(self, key, key_type, token, host, port, info=None): """ From 4e6688cf1113b088017263a982a2f243b88bb64a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Fri, 9 Aug 2024 14:18:16 +0200 Subject: [PATCH 18/22] Workaround to have original error handling for `get()` and pass mypy checks --- lib/galaxy/managers/interactivetool.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 588397bf54b9..11cf5bbd8588 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -64,13 +64,14 @@ def get(self, key, key_type): gxitproxy.c.key_type == key_type, ) cursor_result = conn.execute(stmt) + result = cursor_result.fetchone() - try: - return dict(key=key, key_type=key_type, **dict(zip(cursor_result.keys(), cursor_result.fetchone()))) - except TypeError: # when `cursor_result.fetchone() is None` (no results) + if result is None: log.warning("get(): invalid key: %s key_type %s", key, key_type) return None + return dict(key=key, key_type=key_type, **dict(zip(cursor_result.keys(), result))) + def save(self, key, key_type, token, host, port, info=None): """ Write out a key, key_type, token, value store that is can be used for coordinating with external resources. From 0b6ad177c54c8f14c2f48fae16fbeb422c463280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Tue, 3 Sep 2024 13:45:24 +0200 Subject: [PATCH 19/22] Remove dead code `InteractiveToolPropagatorSQLAlchemy.get()` --- lib/galaxy/managers/interactivetool.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index 11cf5bbd8588..b25ac16a67b6 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -57,21 +57,6 @@ def __init__(self, database_url, encode_id): self._engine = create_engine(database_url) self._encode_id = encode_id - def get(self, key, key_type): - with self._engine.connect() as conn: - stmt = select(*gxitproxy.c["token", "host", "port", "info"]).where( - gxitproxy.c.key == key, - gxitproxy.c.key_type == key_type, - ) - cursor_result = conn.execute(stmt) - result = cursor_result.fetchone() - - if result is None: - log.warning("get(): invalid key: %s key_type %s", key, key_type) - return None - - return dict(key=key, key_type=key_type, **dict(zip(cursor_result.keys(), result))) - def save(self, key, key_type, token, host, port, info=None): """ Write out a key, key_type, token, value store that is can be used for coordinating with external resources. From 9ee6636f4a8b389169139e91938f68aea47e234a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Tue, 3 Sep 2024 14:17:12 +0200 Subject: [PATCH 20/22] Rename `interactivetools_map_sqlalchemy` to `interactivetoolsproxy_map` --- doc/source/admin/galaxy_options.rst | 10 +++++----- .../admin/special_topics/interactivetools.rst | 8 ++++---- lib/galaxy/config/__init__.py | 16 ++++++++-------- lib/galaxy/config/sample/galaxy.yml.sample | 5 ++--- lib/galaxy/config/schemas/config_schema.yml | 4 ++-- lib/galaxy/managers/interactivetool.py | 2 +- test/unit/config/test_config_values.py | 16 ++++++++-------- 7 files changed, 30 insertions(+), 31 deletions(-) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index f232622ece02..438665063e4a 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2115,17 +2115,17 @@ Map for the interactivetool proxy. Mappings are stored in a SQLite database file located on this path. As an alternative, you may also store them in any other RDBMS supported by SQLAlchemy using - the option ``interactivetools_map_sqlalchemy``, which overrides - this one. + the option ``interactivetoolsproxy_map``, which overrides this + one. The value of this option will be resolved with respect to . :Default: ``interactivetools_map.sqlite`` :Type: str -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -``interactivetools_map_sqlalchemy`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +``interactivetoolsproxy_map`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ :Description: Use a database supported by SQLAlchemy as map for the diff --git a/doc/source/admin/special_topics/interactivetools.rst b/doc/source/admin/special_topics/interactivetools.rst index e621edc205ac..f66d895e8725 100644 --- a/doc/source/admin/special_topics/interactivetools.rst +++ b/doc/source/admin/special_topics/interactivetools.rst @@ -109,11 +109,11 @@ the URL and headers, then somewhat massaged, and finally forwarded to the correc SQLite may not be the best choice. A common case is a high-availability production setup, meaning that multiple copies of Galaxy are running on different servers behind a load balancer. - For these situations, there exists an optional |configuration option interactivetools_map_sqlalchemy|_ that allows - using any database supported by SQLAlchemy (it overrides ``interactivetools_map``). + For these situations, there exists an optional |configuration option interactivetoolsproxy_map|_ that allows using + any database supported by SQLAlchemy (it overrides ``interactivetools_map``). -.. |configuration option interactivetools_map_sqlalchemy| replace:: configuration option ``interactivetools_map_sqlalchemy`` -.. _configuration option interactivetools_map_sqlalchemy: ../config.html#interactivetools-map-sqlalchemy +.. |configuration option interactivetoolsproxy_map| replace:: configuration option ``interactivetoolsproxy_map`` +.. _configuration option interactivetoolsproxy_map: ../config.html#interactivetoolsproxy-map .. note:: diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 2a27a19140aa..eba4306839ae 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -1104,19 +1104,19 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: self.proxy_session_map = self.dynamic_proxy_session_map self.manage_dynamic_proxy = self.dynamic_proxy_manage # Set to false if being launched externally - # InteractiveTools propagator mapping - if self.interactivetools_map_sqlalchemy is None: + # Interactive tools proxy mapping + if self.interactivetoolsproxy_map is None: self.interactivetools_map = "sqlite:///" + self._in_root_dir( kwargs.get("interactivetools_map", self._in_data_dir("interactivetools_map.sqlite")) ) else: - self.interactivetools_map = None # overridden by `self.interactivetools_map_sqlalchemy` + self.interactivetools_map = None # overridden by `self.interactivetoolsproxy_map` # ensure the database URL for the SQLAlchemy map does not match that of a Galaxy DB urls = { setting: parse_sqlalchemy_url(value) for setting, value in ( - ("interactivetools_map_sqlalchemy", self.interactivetools_map_sqlalchemy), + ("interactivetoolsproxy_map", self.interactivetoolsproxy_map), ("database_connection", self.database_connection), ("install_database_connection", self.install_database_connection), ) @@ -1128,8 +1128,8 @@ def is_in_conflict(url1, url2): conflicting_settings = { setting - for setting, url in tuple(urls.items())[1:] # exclude "interactivetools_map_sqlalchemy" - if is_in_conflict(url, list(urls.values())[0]) # compare with "interactivetools_map_sqlalchemy" + for setting, url in tuple(urls.items())[1:] # exclude "interactivetoolsproxy_map" + if is_in_conflict(url, list(urls.values())[0]) # compare with "interactivetoolsproxy_map" } if conflicting_settings: @@ -1257,8 +1257,8 @@ def try_parsing(value, name): try_parsing(self.database_connection, "database_connection") try_parsing(self.install_database_connection, "install_database_connection") - if self.interactivetools_map_sqlalchemy is not None: - try_parsing(self.interactivetools_map_sqlalchemy, "interactivetools_map_sqlalchemy") + if self.interactivetoolsproxy_map is not None: + try_parsing(self.interactivetoolsproxy_map, "interactivetoolsproxy_map") try_parsing(self.amqp_internal_connection, "amqp_internal_connection") def _configure_dataset_storage(self): diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index faeee1c70515..c6bbba50e7c9 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -1351,8 +1351,7 @@ galaxy: # Map for the interactivetool proxy. Mappings are stored in a SQLite # database file located on this path. As an alternative, you may also # store them in any other RDBMS supported by SQLAlchemy using the - # option ``interactivetools_map_sqlalchemy``, which overrides this - # one. + # option ``interactivetoolsproxy_map``, which overrides this one. # The value of this option will be resolved with respect to # . #interactivetools_map: interactivetools_map.sqlite @@ -1365,7 +1364,7 @@ galaxy: # Mappings are written to the table "gxitproxy" within the database. # This value cannot match ``database_connection`` nor # ``install_database_connection``. - #interactivetools_map_sqlalchemy: null + #interactivetoolsproxy_map: null # Prefix to use in the formation of the subdomain or path for # interactive tools diff --git a/lib/galaxy/config/schemas/config_schema.yml b/lib/galaxy/config/schemas/config_schema.yml index 2d48d7991b94..063d7f947bbd 100644 --- a/lib/galaxy/config/schemas/config_schema.yml +++ b/lib/galaxy/config/schemas/config_schema.yml @@ -1522,10 +1522,10 @@ mapping: desc: | Map for the interactivetool proxy. Mappings are stored in a SQLite database file located on this path. As an alternative, you may also store them in any other RDBMS - supported by SQLAlchemy using the option ``interactivetools_map_sqlalchemy``, which + supported by SQLAlchemy using the option ``interactivetoolsproxy_map``, which overrides this one. - interactivetools_map_sqlalchemy: + interactivetoolsproxy_map: type: str required: false desc: | diff --git a/lib/galaxy/managers/interactivetool.py b/lib/galaxy/managers/interactivetool.py index b25ac16a67b6..f96fac35d96b 100644 --- a/lib/galaxy/managers/interactivetool.py +++ b/lib/galaxy/managers/interactivetool.py @@ -135,7 +135,7 @@ def __init__(self, app): self.job_manager = app.job_manager self.encoder = IdAsLowercaseAlphanumEncodingHelper(app.security) self.propagator = InteractiveToolPropagatorSQLAlchemy( - app.config.interactivetools_map_sqlalchemy or app.config.interactivetools_map, + app.config.interactivetoolsproxy_map or app.config.interactivetools_map, self.encoder.encode_id, ) diff --git a/test/unit/config/test_config_values.py b/test/unit/config/test_config_values.py index 668ad766ae8b..753fbf7b1f2d 100644 --- a/test/unit/config/test_config_values.py +++ b/test/unit/config/test_config_values.py @@ -67,9 +67,9 @@ def test_error_if_database_connection_contains_brackets(bracket): config.GalaxyAppConfiguration(override_tempdir=False, amqp_internal_connection=uri) -def test_error_if_interactivetools_map_sqlalchemy_matches_other_database_connections(): +def test_error_if_interactivetoolsproxy_map_matches_other_database_connections(): """ - The setting `interactivetools_map_sqlalchemy` allows storing the session map in a + The setting `interactivetoolsproxy_map` allows storing the session map in a database supported by SQLAlchemy. This database must be different from the Galaxy database and the tool shed database. @@ -85,23 +85,23 @@ def test_error_if_interactivetools_map_sqlalchemy_matches_other_database_connect ) with pytest.raises(ConfigurationError): - # interactivetools_map_sqlalchemy matches database_connection + # interactivetoolsproxy_map matches database_connection config.GalaxyAppConfiguration( **settings, - interactivetools_map_sqlalchemy=database_connection, + interactivetoolsproxy_map=database_connection, ) with pytest.raises(ConfigurationError): - # interactivetools_map_sqlalchemy matches install_database_connection + # interactivetoolsproxy_map matches install_database_connection config.GalaxyAppConfiguration( **settings, - interactivetools_map_sqlalchemy=install_database_connection, + interactivetoolsproxy_map=install_database_connection, ) - # interactivetools_map_sqlalchemy differs from database_connection, install_database_connection + # interactivetoolsproxy_map differs from database_connection, install_database_connection config.GalaxyAppConfiguration( **settings, - interactivetools_map_sqlalchemy="dbscheme://user:password@host/gxitproxy", + interactivetoolsproxy_map="dbscheme://user:password@host/gxitproxy", ) From 5c0b5d609c0a9e7e8c4d5cfd9673bda30fe208ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Tue, 3 Sep 2024 15:07:47 +0200 Subject: [PATCH 21/22] Use `urlparse` to compare `interactivetoolsproxy_map` with `database_connection` and `install_database_connection` Using `urlparse` rather than `sqlalchemy.engine.make_url` decreases the precision of the comparison, but removes the need to declare `sqlalchemy` as a dependency of the config package. This is a tradeoff between convenience (preventing mistakes from the user) and complexity (number of dependencies of the config package). --- lib/galaxy/config/__init__.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index eba4306839ae..bb81a5390f44 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -33,7 +33,6 @@ from urllib.parse import urlparse import yaml -from sqlalchemy.engine import make_url as parse_sqlalchemy_url from galaxy.config.schema import AppSchema from galaxy.exceptions import ConfigurationError @@ -1114,7 +1113,7 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: # ensure the database URL for the SQLAlchemy map does not match that of a Galaxy DB urls = { - setting: parse_sqlalchemy_url(value) + setting: urlparse(value) for setting, value in ( ("interactivetoolsproxy_map", self.interactivetoolsproxy_map), ("database_connection", self.database_connection), @@ -1124,7 +1123,14 @@ def _process_config(self, kwargs: Dict[str, Any]) -> None: } def is_in_conflict(url1, url2): - return all((url1.host == url2.host, url1.port == url2.port, url1.database == url2.database)) + return all( + ( + url1.scheme == url2.scheme, + url1.hostname == url2.hostname, + url1.port == url2.port, + url1.path == url2.path, + ) + ) conflicting_settings = { setting From 7510372ce2c72e762f1b6de6ed899b004b2db02e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Tue, 3 Sep 2024 15:09:17 +0200 Subject: [PATCH 22/22] Revert "Add `sqlalchemy` to config package requirements" This reverts commit b6ad695d0377f28e0eb492a95d260d1e2be127b7. --- packages/config/setup.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/config/setup.cfg b/packages/config/setup.cfg index 91aaf36d6a5b..5249de2ee6ab 100644 --- a/packages/config/setup.cfg +++ b/packages/config/setup.cfg @@ -37,7 +37,6 @@ install_requires = jinja2 pykwalify PyYAML - sqlalchemy~=2.0 packages = find: python_requires = >=3.8