From 021d28ebebcbb4a9d4f23158afb42fe62d5e15db Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 30 Sep 2024 12:54:31 -0400 Subject: [PATCH] feat(robot-server): Implement storage for "enable/disable error recovery" setting (#16333) --- .../persistence/_migrations/v6_to_v7.py | 1 + .../persistence/file_and_directory_names.py | 2 +- .../persistence/persistence_directory.py | 3 ++ .../persistence/tables/__init__.py | 4 ++ .../persistence/tables/schema_7.py | 28 +++++++++++ .../runs/error_recovery_setting_store.py | 46 +++++++++++++++++++ .../http_api/persistence/test_reset.py | 18 ++------ robot-server/tests/persistence/test_tables.py | 15 +++++- .../runs/test_error_recovery_setting_store.py | 29 ++++++++++++ 9 files changed, 129 insertions(+), 17 deletions(-) create mode 100644 robot-server/robot_server/runs/error_recovery_setting_store.py create mode 100644 robot-server/tests/runs/test_error_recovery_setting_store.py 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 0faee6736e7..faae646e5b7 100644 --- a/robot-server/robot_server/persistence/_migrations/v6_to_v7.py +++ b/robot-server/robot_server/persistence/_migrations/v6_to_v7.py @@ -3,6 +3,7 @@ Summary of changes from schema 6: - Adds a new command_intent to store the commands intent in the commands table +- Adds the `boolean_setting` table. """ import json diff --git a/robot-server/robot_server/persistence/file_and_directory_names.py b/robot-server/robot_server/persistence/file_and_directory_names.py index 781217f6418..7074dd6db2f 100644 --- a/robot-server/robot_server/persistence/file_and_directory_names.py +++ b/robot-server/robot_server/persistence/file_and_directory_names.py @@ -8,7 +8,7 @@ from typing import Final -LATEST_VERSION_DIRECTORY: Final = "7" +LATEST_VERSION_DIRECTORY: Final = "7.1" DECK_CONFIGURATION_FILE: Final = "deck_configuration.json" PROTOCOLS_DIRECTORY: Final = "protocols" diff --git a/robot-server/robot_server/persistence/persistence_directory.py b/robot-server/robot_server/persistence/persistence_directory.py index c4a9675025a..2dd46711697 100644 --- a/robot-server/robot_server/persistence/persistence_directory.py +++ b/robot-server/robot_server/persistence/persistence_directory.py @@ -52,6 +52,9 @@ async def prepare_active_subdirectory(prepared_root: Path) -> Path: v3_to_v4.Migration3to4(subdirectory="4"), v4_to_v5.Migration4to5(subdirectory="5"), v5_to_v6.Migration5to6(subdirectory="6"), + # Subdirectory "7" was previously used on our edge branch for an in-dev + # schema that was never released to the public. It may be present on + # internal robots. v6_to_v7.Migration6to7(subdirectory=LATEST_VERSION_DIRECTORY), ], temp_file_prefix="temp-", diff --git a/robot-server/robot_server/persistence/tables/__init__.py b/robot-server/robot_server/persistence/tables/__init__.py index 097383a0612..006f5356d76 100644 --- a/robot-server/robot_server/persistence/tables/__init__.py +++ b/robot-server/robot_server/persistence/tables/__init__.py @@ -12,8 +12,10 @@ action_table, run_csv_rtp_table, data_files_table, + boolean_setting_table, PrimitiveParamSQLEnum, ProtocolKindSQLEnum, + BooleanSettingKey, ) @@ -28,6 +30,8 @@ "action_table", "run_csv_rtp_table", "data_files_table", + "boolean_setting_table", "PrimitiveParamSQLEnum", "ProtocolKindSQLEnum", + "BooleanSettingKey", ] diff --git a/robot-server/robot_server/persistence/tables/schema_7.py b/robot-server/robot_server/persistence/tables/schema_7.py index b08a447505d..790113ab8e3 100644 --- a/robot-server/robot_server/persistence/tables/schema_7.py +++ b/robot-server/robot_server/persistence/tables/schema_7.py @@ -102,6 +102,7 @@ class ProtocolKindSQLEnum(enum.Enum): PrimitiveParamSQLEnum, values_callable=lambda obj: [e.value for e in obj], create_constraint=True, + # todo(mm, 2024-09-24): Can we add validate_strings=True here? ), nullable=False, ), @@ -263,3 +264,30 @@ class ProtocolKindSQLEnum(enum.Enum): nullable=True, ), ) + + +class BooleanSettingKey(enum.Enum): + """Keys for boolean settings.""" + + DISABLE_ERROR_RECOVERY = "disable_error_recovery" + + +boolean_setting_table = sqlalchemy.Table( + "boolean_setting", + metadata, + sqlalchemy.Column( + "key", + sqlalchemy.Enum( + BooleanSettingKey, + values_callable=lambda obj: [e.value for e in obj], + validate_strings=True, + create_constraint=True, + ), + primary_key=True, + ), + sqlalchemy.Column( + "value", + sqlalchemy.Boolean, + nullable=False, + ), +) diff --git a/robot-server/robot_server/runs/error_recovery_setting_store.py b/robot-server/robot_server/runs/error_recovery_setting_store.py new file mode 100644 index 00000000000..2a892c26eeb --- /dev/null +++ b/robot-server/robot_server/runs/error_recovery_setting_store.py @@ -0,0 +1,46 @@ +# noqa: D100 + + +import sqlalchemy + +from robot_server.persistence.tables import boolean_setting_table, BooleanSettingKey + + +class ErrorRecoverySettingStore: + """Persistently stores settings related to error recovery.""" + + def __init__(self, sql_engine: sqlalchemy.engine.Engine) -> None: + self._sql_engine = sql_engine + + def get_is_disabled(self) -> bool | None: + """Get the value of the "error recovery enabled" setting. + + `None` is the default, i.e. it's never been explicitly set one way or the other. + """ + with self._sql_engine.begin() as transaction: + return transaction.execute( + sqlalchemy.select(boolean_setting_table.c.value).where( + boolean_setting_table.c.key + == BooleanSettingKey.DISABLE_ERROR_RECOVERY + ) + ).scalar_one_or_none() + + def set_is_disabled(self, is_disabled: bool | None) -> None: + """Set the value of the "error recovery enabled" setting. + + `None` means revert to the default. + """ + with self._sql_engine.begin() as transaction: + transaction.execute( + sqlalchemy.delete(boolean_setting_table).where( + boolean_setting_table.c.key + == BooleanSettingKey.DISABLE_ERROR_RECOVERY + ) + ) + if is_disabled is not None: + transaction.execute( + sqlalchemy.insert(boolean_setting_table).values( + key=BooleanSettingKey.DISABLE_ERROR_RECOVERY, + value=is_disabled, + ) + ) diff --git a/robot-server/tests/integration/http_api/persistence/test_reset.py b/robot-server/tests/integration/http_api/persistence/test_reset.py index ffff3aed08e..2d0bf00cb00 100644 --- a/robot-server/tests/integration/http_api/persistence/test_reset.py +++ b/robot-server/tests/integration/http_api/persistence/test_reset.py @@ -30,23 +30,13 @@ def _get_corrupt_persistence_dir() -> Path: async def _assert_reset_was_successful( robot_client: RobotClient, persistence_directory: Path ) -> None: - # It should have no protocols. + # We really want to check that the server's persistence directory has been wiped + # clean, but testing that directly would rely on internal implementation details + # of file layout and tend to be brittle. + # As an approximation, just check that there are no protocols or runs left. assert (await robot_client.get_protocols()).json()["data"] == [] - - # It should have no runs. assert (await robot_client.get_runs()).json()["data"] == [] - # There should be no files except for robot_server.db - # and an empty protocols/ directory. - all_files_and_directories = set(persistence_directory.glob("**/*")) - expected_files_and_directories = { - persistence_directory / "robot_server.db", - persistence_directory / "7", - persistence_directory / "7" / "protocols", - persistence_directory / "7" / "robot_server.db", - } - assert all_files_and_directories == expected_files_and_directories - async def _wait_until_initialization_failed(robot_client: RobotClient) -> None: """Wait until the server returns an "initialization failed" health status.""" diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 757cdd9a570..bb3a5ece78e 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -142,8 +142,20 @@ FOREIGN KEY(file_id) REFERENCES data_files (id) ) """, + """ + CREATE TABLE boolean_setting ( + "key" VARCHAR(22) NOT NULL, + value BOOLEAN NOT NULL, + PRIMARY KEY ("key"), + CONSTRAINT booleansettingkey CHECK ("key" IN ('disable_error_recovery')) + ) + """, ] + +EXPECTED_STATEMENTS_V7 = EXPECTED_STATEMENTS_LATEST + + EXPECTED_STATEMENTS_V6 = [ """ CREATE TABLE protocol ( @@ -257,8 +269,6 @@ ] -EXPECTED_STATEMENTS_V7 = EXPECTED_STATEMENTS_LATEST - EXPECTED_STATEMENTS_V5 = [ """ CREATE TABLE protocol ( @@ -334,6 +344,7 @@ """, ] + EXPECTED_STATEMENTS_V4 = [ """ CREATE TABLE protocol ( diff --git a/robot-server/tests/runs/test_error_recovery_setting_store.py b/robot-server/tests/runs/test_error_recovery_setting_store.py new file mode 100644 index 00000000000..fae8bb76705 --- /dev/null +++ b/robot-server/tests/runs/test_error_recovery_setting_store.py @@ -0,0 +1,29 @@ +"""Tests for error_recovery_setting_store.""" + + +from robot_server.runs.error_recovery_setting_store import ErrorRecoverySettingStore + +import pytest +import sqlalchemy + + +@pytest.fixture +def subject( + sql_engine: sqlalchemy.engine.Engine, +) -> ErrorRecoverySettingStore: + """Return a test subject.""" + return ErrorRecoverySettingStore(sql_engine=sql_engine) + + +def test_error_recovery_setting_store(subject: ErrorRecoverySettingStore) -> None: + """Test `ErrorRecoverySettingStore`.""" + assert subject.get_is_disabled() is None + + subject.set_is_disabled(is_disabled=False) + assert subject.get_is_disabled() is False + + subject.set_is_disabled(is_disabled=True) + assert subject.get_is_disabled() is True + + subject.set_is_disabled(is_disabled=None) + assert subject.get_is_disabled() is None