From 1b67a374ca2665fdac989db061534a719d5e193f Mon Sep 17 00:00:00 2001
From: Max Marrone <max@opentrons.com>
Date: Wed, 13 Nov 2024 17:24:35 -0500
Subject: [PATCH] refactor(robot-server): Adjust SQL declarations to match
 reality (#16799)

---
 .../persistence/tables/schema_7.py            | 21 +++++++++---
 robot-server/tests/persistence/test_tables.py | 32 +++++--------------
 2 files changed, 25 insertions(+), 28 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..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, index=True),
+    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",
@@ -251,10 +258,16 @@ 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,
         ),
-        index=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 9c069157144..642d2506e93 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)
     )
@@ -121,23 +121,16 @@
     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)
     """,
     """
-    CREATE INDEX ix_run_command_command_intent ON run_command (command_intent)
-    """,
-    """
     CREATE TABLE data_files (
         id VARCHAR NOT NULL,
         name VARCHAR NOT NULL,
         file_hash VARCHAR NOT NULL,
         created_at DATETIME NOT NULL,
-        source VARCHAR(9) NOT NULL,
-        PRIMARY KEY (id),
-        CONSTRAINT datafilesourcesqlenum CHECK (source IN ('uploaded', 'generated'))
+        source VARCHAR(9),
+        PRIMARY KEY (id)
     )
     """,
     """
@@ -542,7 +535,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 +544,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(
@@ -598,18 +594,6 @@ def record_statement(
     assert set(normalized_actual) == set(normalized_expected)
 
 
-# FIXME(mm, 2024-11-12): https://opentrons.atlassian.net/browse/EXEC-827
-#
-# 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
-# - 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)
 def test_migrated_db_matches_db_created_from_metadata(tmp_path: Path) -> None:
     """Test that the output of migration matches `metadata.create_all()`.