Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(robot-server): Adjust SQL declarations to match reality #16799

Merged
merged 7 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions robot-server/robot_server/persistence/tables/schema_7.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +213 to +216
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way of explaining this, other than "migration bug," is that SQLite makes it very annoying to add non-nullable columns. Like, yeah, it's arguably a bug in the migration, but we'd have to go unusually far out of our way to avoid that bug, so is it really?

I'm happy to write whatever other people find more helpful.

),
sqlalchemy.Index(
"ix_run_run_id_command_id", # An arbitrary name for the index.
"run_id",
Expand Down Expand Up @@ -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,
),
)

Expand Down
32 changes: 8 additions & 24 deletions robot-server/tests/persistence/test_tables.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
Expand All @@ -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)
)
""",
"""
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand Down Expand Up @@ -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()`.

Expand Down
Loading