From 05313951c4f2e4c95e3256f9690a0802ce363296 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 12:50:00 -0500 Subject: [PATCH 1/7] Further normalize SQL formatting. --- robot-server/tests/persistence/test_tables.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 9c069157144..7b9a50b8572 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -542,7 +542,7 @@ def _normalize_statement(statement: str) -> str: - """Fix up the formatting of a SQL statement for easier comparison.""" + """Fix up the internal formatting of a single SQL statement for easier comparison.""" lines = statement.splitlines() # Remove whitespace at the beginning and end of each line. @@ -551,7 +551,10 @@ def _normalize_statement(statement: str) -> str: # Filter out blank lines. lines = [line for line in lines if line != ""] - return "\n".join(lines) + # Normalize line breaks to spaces. When we ask SQLite for its schema, it appears + # inconsistent in whether it uses spaces or line breaks to separate tokens. + # That may have to do with whether `ALTER TABLE` has been used on the table. + return " ".join(lines) @pytest.mark.parametrize( From 93716976461687ae38bfec3e8ba7167af60fb055 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 11:28:52 -0500 Subject: [PATCH 2/7] Reconcile data_files.source index. --- robot-server/robot_server/persistence/tables/schema_7.py | 1 - robot-server/tests/persistence/test_tables.py | 4 ---- 2 files changed, 5 deletions(-) diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index 1690298007f..331cdc1888a 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -253,7 +253,6 @@ class DataFileSourceSQLEnum(enum.Enum): validate_strings=True, create_constraint=True, ), - index=True, nullable=False, ), ) diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 7b9a50b8572..06f3e88c97b 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -121,9 +121,6 @@ CREATE UNIQUE INDEX ix_run_run_id_index_in_run ON run_command (run_id, index_in_run) """, """ - CREATE INDEX ix_data_files_source ON data_files (source) - """, - """ CREATE INDEX ix_protocol_protocol_kind ON protocol (protocol_kind) """, """ @@ -605,7 +602,6 @@ def record_statement( # # There are at least these mismatches: # -# - `ix_data_files_source` is present in metadata, but not emitted by the migration path # - `ix_run_command_command_intent` is present in metadata, but not emitted by the migration path # - `data_files.source` is nullable as emitted by the migration path, but not as declared in metadata # - `command.command_intent` is nullable as emitted by the migration path, but not as declared in metadata From 9d59e5821e11598f7425b4c4812d07febdd8f4be Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 11:30:06 -0500 Subject: [PATCH 3/7] Reconcile run_command.command_intent index. --- robot-server/robot_server/persistence/tables/schema_7.py | 2 +- robot-server/tests/persistence/test_tables.py | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index 331cdc1888a..02cafc2dfc6 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -207,7 +207,7 @@ class DataFileSourceSQLEnum(enum.Enum): sqlalchemy.Column("index_in_run", sqlalchemy.Integer, nullable=False), sqlalchemy.Column("command_id", sqlalchemy.String, nullable=False), sqlalchemy.Column("command", sqlalchemy.String, nullable=False), - sqlalchemy.Column("command_intent", sqlalchemy.String, nullable=False, index=True), + sqlalchemy.Column("command_intent", sqlalchemy.String, nullable=False), sqlalchemy.Index( "ix_run_run_id_command_id", # An arbitrary name for the index. "run_id", diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 06f3e88c97b..1af7641988c 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -124,9 +124,6 @@ CREATE INDEX ix_protocol_protocol_kind ON protocol (protocol_kind) """, """ - CREATE INDEX ix_run_command_command_intent ON run_command (command_intent) - """, - """ CREATE TABLE data_files ( id VARCHAR NOT NULL, name VARCHAR NOT NULL, @@ -602,7 +599,6 @@ def record_statement( # # There are at least these mismatches: # -# - `ix_run_command_command_intent` is present in metadata, but not emitted by the migration path # - `data_files.source` is nullable as emitted by the migration path, but not as declared in metadata # - `command.command_intent` is nullable as emitted by the migration path, but not as declared in metadata # - constraint `datafilesourcesqlenum` is present in metadata, but not not emitted by the migration path From fdcbb24acb7d7574b874e3a01af85aae572b691e Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 12:03:50 -0500 Subject: [PATCH 4/7] Reconcile data_files.source nullability. --- robot-server/robot_server/persistence/tables/schema_7.py | 5 ++++- robot-server/tests/persistence/test_tables.py | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index 02cafc2dfc6..a2524bf35d3 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -253,7 +253,10 @@ class DataFileSourceSQLEnum(enum.Enum): validate_strings=True, create_constraint=True, ), - nullable=False, + # nullable=True to match the underlying SQL, which is nullable because of a bug + # in the migration that introduced this column. This is not intended to ever be + # null in practice. + nullable=True, ), ) diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 1af7641988c..285733a4ee7 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -129,7 +129,7 @@ name VARCHAR NOT NULL, file_hash VARCHAR NOT NULL, created_at DATETIME NOT NULL, - source VARCHAR(9) NOT NULL, + source VARCHAR(9), PRIMARY KEY (id), CONSTRAINT datafilesourcesqlenum CHECK (source IN ('uploaded', 'generated')) ) @@ -599,7 +599,6 @@ def record_statement( # # There are at least these mismatches: # -# - `data_files.source` is nullable as emitted by the migration path, but not as declared in metadata # - `command.command_intent` is nullable as emitted by the migration path, but not as declared in metadata # - constraint `datafilesourcesqlenum` is present in metadata, but not not emitted by the migration path # From d5ad93860767b6a05c8d1018850a4569c2d791c7 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 12:10:47 -0500 Subject: [PATCH 5/7] Reconcile data_files.source constraint. --- robot-server/robot_server/persistence/tables/schema_7.py | 6 +++++- robot-server/tests/persistence/test_tables.py | 4 +--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index a2524bf35d3..73d6b0cea97 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -251,7 +251,11 @@ class DataFileSourceSQLEnum(enum.Enum): DataFileSourceSQLEnum, values_callable=lambda obj: [e.value for e in obj], validate_strings=True, - create_constraint=True, + # create_constraint=False to match the underlying SQL, which omits + # the constraint because of a bug in the migration that introduced this + # column. This is not intended to ever have values other than those in + # DataFileSourceSQLEnum. + create_constraint=False, ), # nullable=True to match the underlying SQL, which is nullable because of a bug # in the migration that introduced this column. This is not intended to ever be diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 285733a4ee7..80a4357dba4 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -130,8 +130,7 @@ file_hash VARCHAR NOT NULL, created_at DATETIME NOT NULL, source VARCHAR(9), - PRIMARY KEY (id), - CONSTRAINT datafilesourcesqlenum CHECK (source IN ('uploaded', 'generated')) + PRIMARY KEY (id) ) """, """ @@ -600,7 +599,6 @@ def record_statement( # There are at least these mismatches: # # - `command.command_intent` is nullable as emitted by the migration path, but not as declared in metadata -# - constraint `datafilesourcesqlenum` is present in metadata, but not not emitted by the migration path # # Remove this xfail mark when the mismatches are resolved. @pytest.mark.xfail(strict=True) From 942b102462525e78b4ddb903db02062b16d2e56b Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 12:23:45 -0500 Subject: [PATCH 6/7] Reconcile command_intent nullability. --- robot-server/robot_server/persistence/tables/schema_7.py | 9 ++++++++- robot-server/tests/persistence/test_tables.py | 7 +------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index 73d6b0cea97..9f0c2879533 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -207,7 +207,14 @@ class DataFileSourceSQLEnum(enum.Enum): sqlalchemy.Column("index_in_run", sqlalchemy.Integer, nullable=False), sqlalchemy.Column("command_id", sqlalchemy.String, nullable=False), sqlalchemy.Column("command", sqlalchemy.String, nullable=False), - sqlalchemy.Column("command_intent", sqlalchemy.String, nullable=False), + sqlalchemy.Column( + "command_intent", + sqlalchemy.String, + # nullable=True to match the underlying SQL, which is nullable because of a bug + # in the migration that introduced this column. This is not intended to ever be + # null in practice. + nullable=True, + ), sqlalchemy.Index( "ix_run_run_id_command_id", # An arbitrary name for the index. "run_id", diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 80a4357dba4..ee3b58a856a 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -109,7 +109,7 @@ index_in_run INTEGER NOT NULL, command_id VARCHAR NOT NULL, command VARCHAR NOT NULL, - command_intent VARCHAR NOT NULL, + command_intent VARCHAR, PRIMARY KEY (row_id), FOREIGN KEY(run_id) REFERENCES run (id) ) @@ -595,11 +595,6 @@ def record_statement( # FIXME(mm, 2024-11-12): https://opentrons.atlassian.net/browse/EXEC-827 -# -# There are at least these mismatches: -# -# - `command.command_intent` is nullable as emitted by the migration path, but not as declared in metadata -# # Remove this xfail mark when the mismatches are resolved. @pytest.mark.xfail(strict=True) def test_migrated_db_matches_db_created_from_metadata(tmp_path: Path) -> None: From 438bfd2327d79fc882b83980b304d9a2039f6178 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 13 Nov 2024 12:56:53 -0500 Subject: [PATCH 7/7] Remove xfail. --- robot-server/tests/persistence/test_tables.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index ee3b58a856a..642d2506e93 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -594,9 +594,6 @@ def record_statement( assert set(normalized_actual) == set(normalized_expected) -# FIXME(mm, 2024-11-12): https://opentrons.atlassian.net/browse/EXEC-827 -# Remove this xfail mark when the mismatches are resolved. -@pytest.mark.xfail(strict=True) def test_migrated_db_matches_db_created_from_metadata(tmp_path: Path) -> None: """Test that the output of migration matches `metadata.create_all()`.