From 1e75d85fd4ca2a35dfa0081840125a2d725600e6 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 13 Jul 2022 15:40:42 -0400 Subject: [PATCH 1/4] Prevent accessing LegacyScripts.database before processing script args --- lib/galaxy/model/migrations/scripts.py | 11 +++++++++-- test/unit/data/model/migrations/test_scripts.py | 12 ++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/model/migrations/scripts.py b/lib/galaxy/model/migrations/scripts.py index b23c6af7bc9..487e0b744ce 100644 --- a/lib/galaxy/model/migrations/scripts.py +++ b/lib/galaxy/model/migrations/scripts.py @@ -120,7 +120,13 @@ class LegacyScripts: def __init__(self, argv: List[str], cwd: Optional[str] = None) -> None: self.argv = argv self.cwd = cwd or os.getcwd() - self.database = self.DEFAULT_DB_ARG + self._database = None # Do not assign default value: `None` means we don't know yet. + + @property + def database(self): + if self._database is None: + raise LegacyScriptsException("Attempt to access identifier of database before processing the script arguments") + return self._database def run(self) -> None: """ @@ -147,8 +153,9 @@ class LegacyScripts: If last argument is a valid database name, pop and assign it; otherwise assign default. """ arg = self.argv[-1] + self._database = self.DEFAULT_DB_ARG if arg in ["galaxy", "install"]: - self.database = self.argv.pop() + self._database = self.argv.pop() def rename_config_argument(self) -> None: """ diff --git a/test/unit/data/model/migrations/test_scripts.py b/test/unit/data/model/migrations/test_scripts.py index 216bc6a5efe..5fc28b73346 100644 --- a/test/unit/data/model/migrations/test_scripts.py +++ b/test/unit/data/model/migrations/test_scripts.py @@ -145,3 +145,15 @@ class TestLegacyScripts: argv = ["caller", "--alembic-config", "path-to-alembic", "downgrade"] with pytest.raises(LegacyScriptsException): LegacyScripts(argv).convert_args() + + def test_access_database_id(self): + db = "galaxy" + argv = ["caller", "--alembic-config", "path-to-alembic", "upgrade", db] + ls = LegacyScripts(argv) + ls.run() + assert ls.database == db + + def test_access_database_id_before_processing_script_args_raises_error(self): + argv = ["caller", "--alembic-config", "path-to-alembic", "upgrade"] + with pytest.raises(LegacyScriptsException): + LegacyScripts(argv).database From def882932d152a6f4d868d6e1a67e730b7550f9e Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 13 Jul 2022 16:10:58 -0400 Subject: [PATCH 2/4] Add verify_db_is_initialized function; tests --- lib/galaxy/model/migrations/scripts.py | 54 ++++++++++++++++++- .../data/model/migrations/test_scripts.py | 15 ++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/model/migrations/scripts.py b/lib/galaxy/model/migrations/scripts.py index 487e0b744ce..491f8f1ce6f 100644 --- a/lib/galaxy/model/migrations/scripts.py +++ b/lib/galaxy/model/migrations/scripts.py @@ -14,7 +14,10 @@ from alembic.script import ScriptDirectory from sqlalchemy import create_engine from sqlalchemy.engine import Engine -from galaxy.model.database_utils import is_one_database +from galaxy.model.database_utils import ( + database_exists, + is_one_database, +) from galaxy.model.migrations import ( AlembicManager, DatabaseConfig, @@ -38,6 +41,51 @@ GXY_CONFIG_PREFIX = "GALAXY_CONFIG_" TSI_CONFIG_PREFIX = "GALAXY_INSTALL_CONFIG_" +class DatabaseDoesNotExistError(Exception): + def __init__(self, db_url: str) -> None: + super().__init__( + f"""The database at {db_url} does not exist. You must + create and initialize the database before running this script. You + can do so by (a) running `create_db.sh`; or by (b) starting Galaxy, + in which case Galaxy will create and initialize the database + automatically.""" + ) + + +class DatabaseNotInitializedError(Exception): + def __init__(self, db_url: str) -> None: + super().__init__( + f"""The database at {db_url} is empty. You must + initialize the database before running this script. You can do so by + (a) running `create_db.sh`; or by (b) starting Galaxy, in which case + Galaxy will initialize the database automatically.""" + ) + + +def verify_database_is_initialized(db_url: str) -> None: + """ + Intended for use by scripts that run database migrations (manage_db.sh, + run_alembic.sh). Those scripts are meant to run on a database that has been + initialized with the appropriate metadata (e.g. galaxy or install model). + + This function will raise an error if the database does not exist or has not + been initialized*. + + *NOTE: this function cannot determine whether a database has been properly + initialized; it can only tell when a database has *not* been initialized. + """ + if not database_exists(db_url): + raise DatabaseDoesNotExistError(db_url) + + engine = create_engine(db_url) + try: + db_state = DatabaseStateCache(engine=engine) + if db_state.is_database_empty() or db_state.contains_only_kombu_tables(): + raise DatabaseNotInitializedError(db_url) + finally: + engine.dispose() + + def get_configuration(argv: List[str], cwd: str) -> Tuple[DatabaseConfig, DatabaseConfig, bool]: """ Return a 3-item-tuple with configuration values used for managing databases. @@ -125,7 +173,9 @@ class LegacyScripts: @property def database(self): if self._database is None: - raise LegacyScriptsException("Attempt to access identifier of database before processing the script arguments") + raise LegacyScriptsException( + "Attempt to access identifier of database before processing the script arguments" + ) return self._database def run(self) -> None: diff --git a/test/unit/data/model/migrations/test_scripts.py b/test/unit/data/model/migrations/test_scripts.py index 5fc28b73346..0472b7503a7 100644 --- a/test/unit/data/model/migrations/test_scripts.py +++ b/test/unit/data/model/migrations/test_scripts.py @@ -1,8 +1,13 @@ +import random + import pytest from galaxy.model.migrations.scripts import ( + DatabaseDoesNotExistError, + DatabaseNotInitializedError, LegacyScripts, LegacyScriptsException, + verify_database_is_initialized, ) @@ -157,3 +162,13 @@ class TestLegacyScripts: argv = ["caller", "--alembic-config", "path-to-alembic", "upgrade"] with pytest.raises(LegacyScriptsException): LegacyScripts(argv).database + + def test_verify_database_is_init_raises_error_if_no_database(self): + nonexistant_path = str(random.random())[2:] + db_url = f"sqlite:////{nonexistant_path}" + with pytest.raises(DatabaseDoesNotExistError): + verify_database_is_initialized(db_url) + + def test_verify_database_is_init_raises_error_if_database_not_initialized(self, sqlite_memory_url): + with pytest.raises(DatabaseNotInitializedError): + verify_database_is_initialized(sqlite_memory_url) From af81f2a01d9685a967b2ccc89eed34b637e0ccc7 Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 13 Jul 2022 16:38:11 -0400 Subject: [PATCH 3/4] Verify db initialization before running upgrade script --- lib/galaxy/model/migrations/scripts.py | 6 ++++++ scripts/manage_db_adapter.py | 3 +++ 2 files changed, 9 insertions(+) diff --git a/lib/galaxy/model/migrations/scripts.py b/lib/galaxy/model/migrations/scripts.py index 491f8f1ce6f..c25b88d0080 100644 --- a/lib/galaxy/model/migrations/scripts.py +++ b/lib/galaxy/model/migrations/scripts.py @@ -253,6 +253,12 @@ class LegacyScripts: elif self.database == "install": self.argv.append("tsi@head") + def get_db_url(self): + if self.database in ["galaxy", self.DEFAULT_DB_ARG]: + return self.gxy_url + elif self.database == "install": + return self.tsi_url + def _rename_arg(self, old_name, new_name) -> None: pos = self.argv.index(old_name) self.argv[pos] = new_name diff --git a/scripts/manage_db_adapter.py b/scripts/manage_db_adapter.py index d4be4f14127..f19974486e8 100644 --- a/scripts/manage_db_adapter.py +++ b/scripts/manage_db_adapter.py @@ -25,12 +25,15 @@ sys.path.insert(1, os.path.abspath(os.path.join(os.path.dirname(__file__), os.pa from galaxy.model.migrations.scripts import ( invoke_alembic, LegacyScripts, + verify_database_is_initialized, ) def run(): ls = LegacyScripts(sys.argv, os.getcwd()) ls.run() + db_url = ls.get_db_url() + verify_database_is_initialized(db_url) invoke_alembic() From 2d19565986a3f64a17dfe3949e50bf701412468b Mon Sep 17 00:00:00 2001 From: John Davis Date: Wed, 13 Jul 2022 16:43:19 -0400 Subject: [PATCH 4/4] Fix mypy --- lib/galaxy/model/migrations/scripts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/migrations/scripts.py b/lib/galaxy/model/migrations/scripts.py index c25b88d0080..54f0ad774e1 100644 --- a/lib/galaxy/model/migrations/scripts.py +++ b/lib/galaxy/model/migrations/scripts.py @@ -168,7 +168,7 @@ class LegacyScripts: def __init__(self, argv: List[str], cwd: Optional[str] = None) -> None: self.argv = argv self.cwd = cwd or os.getcwd() - self._database = None # Do not assign default value: `None` means we don't know yet. + self._database: Optional[str] = None # Do not assign default value: `None` means we don't know yet. @property def database(self):