From cf88696eb24c98b8fbf4eef2afcf40050f5730aa Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 20 Nov 2024 15:14:45 -0500 Subject: [PATCH 1/4] Treat SQLAlchemy deprecation warnings as errors. --- robot-server/pytest.ini | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/robot-server/pytest.ini b/robot-server/pytest.ini index 211c218295d..51ed89fd0d2 100644 --- a/robot-server/pytest.ini +++ b/robot-server/pytest.ini @@ -4,3 +4,8 @@ markers = ot3_only: Test only functions using the OT3 hardware addopts = --color=yes --strict-markers asyncio_mode = auto + +# Don't allow any new code that uses features removed in SQLAlchemy 2.0. +# We should remove this when we upgrade to SQLAlchemy 2.0. +filterwarnings = + error::sqlalchemy.exc.RemovedIn20Warning From afbe61e4449d5269e7d0cafc4461cee20349e0c5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 20 Nov 2024 15:14:49 -0500 Subject: [PATCH 2/4] Deduplicate add_column() and avoid deprecated features. - Execute on a transaction or connection, not the raw engine. - Use sqlalchemy.text(), not a raw string. --- .../persistence/_migrations/_util.py | 42 ++++++++++++++++++- .../persistence/_migrations/v3_to_v4.py | 11 +---- .../persistence/_migrations/v4_to_v5.py | 14 +------ .../persistence/_migrations/v6_to_v7.py | 12 +----- .../persistence/_migrations/v7_to_v8.py | 11 +---- 5 files changed, 44 insertions(+), 46 deletions(-) diff --git a/robot-server/robot_server/persistence/_migrations/_util.py b/robot-server/robot_server/persistence/_migrations/_util.py index b3c44a96af2..e597a29f89d 100644 --- a/robot-server/robot_server/persistence/_migrations/_util.py +++ b/robot-server/robot_server/persistence/_migrations/_util.py @@ -1,10 +1,11 @@ """Shared helpers for migrations.""" -import sqlalchemy - import shutil +import typing from pathlib import Path +import sqlalchemy + from ..database import sqlite_rowid @@ -54,3 +55,40 @@ def copytree_if_exists(src: Path, dst: Path) -> None: shutil.copytree(src=src, dst=dst) except FileNotFoundError: pass + + +def add_column( + engine: sqlalchemy.engine.Engine, + table_name: str, + column: typing.Any, +) -> None: + """Add a column to an existing SQL table, with an `ALTER TABLE` statement. + + Params: + engine: A SQLAlchemy engine to connect to the database. + table_name: The SQL name of the parent table. + column: The SQLAlchemy column object. + + Known limitations: + + - This does not currently support indexes. + - This does not currently support constraints. + - The column will always be added as nullable. Adding non-nullable columns in + SQLite requires an elaborate and sensitive dance that we do not wish to attempt. + https://www.sqlite.org/lang_altertable.html#making_other_kinds_of_table_schema_changes + + To avoid those limitations, instead of this function, consider this: + + 1. Start with an empty database, or drop or rename the current table. + 2. Use SQLAlchemy's `metadata.create_all()` to create an empty table with the new + schema, including the new column. + 3. Copy rows from the old table to the new one, populating the new column + however you please. + """ + column_type = column.type.compile(engine.dialect) + with engine.begin() as transaction: + transaction.execute( + sqlalchemy.text( + f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" + ) + ) diff --git a/robot-server/robot_server/persistence/_migrations/v3_to_v4.py b/robot-server/robot_server/persistence/_migrations/v3_to_v4.py index f5273f5f678..118f8fcc8fc 100644 --- a/robot-server/robot_server/persistence/_migrations/v3_to_v4.py +++ b/robot-server/robot_server/persistence/_migrations/v3_to_v4.py @@ -13,6 +13,7 @@ import sqlalchemy +from ._util import add_column from ..database import sql_engine_ctx from ..file_and_directory_names import DB_FILE from ..tables import schema_4 @@ -35,16 +36,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None: dest_engine = exit_stack.enter_context(sql_engine_ctx(dest_db_file)) schema_4.metadata.create_all(dest_engine) - def add_column( - engine: sqlalchemy.engine.Engine, - table_name: str, - column: Any, - ) -> None: - column_type = column.type.compile(engine.dialect) - engine.execute( - f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" - ) - add_column( dest_engine, schema_4.analysis_table.name, diff --git a/robot-server/robot_server/persistence/_migrations/v4_to_v5.py b/robot-server/robot_server/persistence/_migrations/v4_to_v5.py index 788723968b2..44b7f9a12d4 100644 --- a/robot-server/robot_server/persistence/_migrations/v4_to_v5.py +++ b/robot-server/robot_server/persistence/_migrations/v4_to_v5.py @@ -9,10 +9,8 @@ from pathlib import Path from contextlib import ExitStack import shutil -from typing import Any - -import sqlalchemy +from ._util import add_column from ..database import sql_engine_ctx from ..file_and_directory_names import DB_FILE from ..tables import schema_5 @@ -35,16 +33,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None: dest_engine = exit_stack.enter_context(sql_engine_ctx(dest_db_file)) schema_5.metadata.create_all(dest_engine) - def add_column( - engine: sqlalchemy.engine.Engine, - table_name: str, - column: Any, - ) -> None: - column_type = column.type.compile(engine.dialect) - engine.execute( - f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" - ) - add_column( dest_engine, schema_5.protocol_table.name, diff --git a/robot-server/robot_server/persistence/_migrations/v6_to_v7.py b/robot-server/robot_server/persistence/_migrations/v6_to_v7.py index ce528a12ab7..d71e4336e9c 100644 --- a/robot-server/robot_server/persistence/_migrations/v6_to_v7.py +++ b/robot-server/robot_server/persistence/_migrations/v6_to_v7.py @@ -11,10 +11,10 @@ from pathlib import Path from contextlib import ExitStack import shutil -from typing import Any import sqlalchemy +from ._util import add_column from ..database import sql_engine_ctx, sqlite_rowid from ..tables import DataFileSourceSQLEnum, schema_7 from .._folder_migrator import Migration @@ -44,16 +44,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None: dest_transaction = exit_stack.enter_context(dest_engine.begin()) - def add_column( - engine: sqlalchemy.engine.Engine, - table_name: str, - column: Any, - ) -> None: - column_type = column.type.compile(engine.dialect) - engine.execute( - f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" - ) - add_column( dest_engine, schema_7.run_command_table.name, diff --git a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py index 5299298c5f1..0473754d690 100644 --- a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py +++ b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py @@ -14,6 +14,7 @@ import sqlalchemy +from ._util import add_column from ..database import sql_engine_ctx from ..tables import schema_8 from .._folder_migrator import Migration @@ -41,16 +42,6 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None: dest_transaction = exit_stack.enter_context(dest_engine.begin()) - def add_column( - engine: sqlalchemy.engine.Engine, - table_name: str, - column: Any, - ) -> None: - column_type = column.type.compile(engine.dialect) - engine.execute( - f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" - ) - add_column( dest_engine, schema_8.run_command_table.name, From f6b61b7a607356b9478356754d8a216e26bc591a Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 20 Nov 2024 19:25:59 -0500 Subject: [PATCH 3/4] Deduplicate index declaration and avoid executing raw string. --- .../robot_server/persistence/_migrations/v7_to_v8.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py index 0473754d690..fe5322efaaf 100644 --- a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py +++ b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py @@ -64,9 +64,12 @@ def migrate(self, source_dir: Path, dest_dir: Path) -> None: def _add_missing_indexes(dest_transaction: sqlalchemy.engine.Connection) -> None: # todo(2024-11-20): Probably add the indexes missing from prior migrations here. # https://opentrons.atlassian.net/browse/EXEC-827 - dest_transaction.execute( - "CREATE UNIQUE INDEX ix_run_run_id_command_status_index_in_run ON run_command (run_id, command_status, index_in_run);" + index = next( + index + for index in schema_8.run_command_table.indexes + if index.name == "ix_run_run_id_command_status_index_in_run" ) + index.create(dest_transaction) def _migrate_command_table_with_new_command_error_col_and_command_status( From aa720f8d7b0be29ee928be8d5ad3236669f307a3 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 21 Nov 2024 11:55:59 -0500 Subject: [PATCH 4/4] We love to lint. --- robot-server/robot_server/persistence/_migrations/v3_to_v4.py | 3 --- robot-server/robot_server/persistence/_migrations/v7_to_v8.py | 1 - 2 files changed, 4 deletions(-) diff --git a/robot-server/robot_server/persistence/_migrations/v3_to_v4.py b/robot-server/robot_server/persistence/_migrations/v3_to_v4.py index 118f8fcc8fc..b69eb0e5b4f 100644 --- a/robot-server/robot_server/persistence/_migrations/v3_to_v4.py +++ b/robot-server/robot_server/persistence/_migrations/v3_to_v4.py @@ -9,9 +9,6 @@ from pathlib import Path from contextlib import ExitStack import shutil -from typing import Any - -import sqlalchemy from ._util import add_column from ..database import sql_engine_ctx diff --git a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py index fe5322efaaf..dce151c9e5f 100644 --- a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py +++ b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py @@ -10,7 +10,6 @@ from pathlib import Path from contextlib import ExitStack import shutil -from typing import Any import sqlalchemy