Fix minimum schema version to run event_id_post_migration (#139014)

* Fix minimum version to run event_id_post_migration

The table rebuild to fix the foreign key constraint was added
in #120779 but the
schema version was not bumped so we need to make sure
any database that was created with schema 43 or older
still has the migration run as otherwise they will not
be able to purge the database with SQLite since each
delete in the events table will due a full table scan
of the states table to look for a foreign key that is
not there

fixes #138818

* Apply suggestions from code review

* Update homeassistant/components/recorder/migration.py

* Update homeassistant/components/recorder/migration.py

* Update homeassistant/components/recorder/const.py

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* update tests, add more cover

* update tests, add more cover

* Update tests/components/recorder/test_migration_run_time_migrations_remember.py

Author: bdraco

homeassistant/components/recorder/const.py

@@ -50,6 +50,11 @@
 LAST_REPORTED_SCHEMA_VERSION = 43
 
 LEGACY_STATES_EVENT_ID_INDEX_SCHEMA_VERSION = 28
+LEGACY_STATES_EVENT_FOREIGN_KEYS_FIXED_SCHEMA_VERSION = 43
+# https://github.com/home-assistant/core/pull/120779
+# fixed the foreign keys in the states table but it did
+# not bump the schema version which means only databases
+# created with schema 44 and later do not need the rebuild.
 
 INTEGRATION_PLATFORM_COMPILE_STATISTICS = "compile_statistics"
 INTEGRATION_PLATFORM_LIST_STATISTIC_IDS = "list_statistic_ids"

homeassistant/components/recorder/migration.py

@@ -52,6 +52,7 @@
 from .const import (
     CONTEXT_ID_AS_BINARY_SCHEMA_VERSION,
     EVENT_TYPE_IDS_SCHEMA_VERSION,
+    LEGACY_STATES_EVENT_FOREIGN_KEYS_FIXED_SCHEMA_VERSION,
     LEGACY_STATES_EVENT_ID_INDEX_SCHEMA_VERSION,
     STATES_META_SCHEMA_VERSION,
     SupportedDialect,
@@ -2490,9 +2491,10 @@ def needs_migrate(self, instance: Recorder, session: Session) -> bool:
         if self.initial_schema_version > self.max_initial_schema_version:
             _LOGGER.debug(
                 "Data migration '%s' not needed, database created with version %s "
-                "after migrator was added",
+                "after migrator was added in version %s",
                 self.migration_id,
                 self.initial_schema_version,
+                self.max_initial_schema_version,
             )
             return False
         if self.start_schema_version < self.required_schema_version:
@@ -2868,7 +2870,14 @@ class EventIDPostMigration(BaseRunTimeMigration):
     """Migration to remove old event_id index from states."""
 
     migration_id = "event_id_post_migration"
-    max_initial_schema_version = LEGACY_STATES_EVENT_ID_INDEX_SCHEMA_VERSION - 1
+    # Note we don't subtract 1 from the max_initial_schema_version
+    # in this case because we need to run this migration on databases
+    # version >= 43 because the schema was not bumped when the table
+    # rebuild was added in
+    # https://github.com/home-assistant/core/pull/120779
+    # which means its only safe to assume version 44 and later
+    # do not need the table rebuild
+    max_initial_schema_version = LEGACY_STATES_EVENT_FOREIGN_KEYS_FIXED_SCHEMA_VERSION
     task = MigrationTask
     migration_version = 2
 

tests/components/recorder/test_migration_from_schema_32.py

@@ -225,6 +225,7 @@ def _insert_events():
         patch.object(recorder, "db_schema", old_db_schema),
         patch.object(migration, "SCHEMA_VERSION", old_db_schema.SCHEMA_VERSION),
         patch.object(migration.EventsContextIDMigration, "migrate_data"),
+        patch.object(migration.EventIDPostMigration, "migrate_data"),
         patch(CREATE_ENGINE_TARGET, new=_create_engine_test),
     ):
         async with (
@@ -282,6 +283,7 @@ def _fetch_migrated_events():
             patch(
                 "sqlalchemy.schema.Index.create", autospec=True, wraps=Index.create
             ) as wrapped_idx_create,
+            patch.object(migration.EventIDPostMigration, "migrate_data"),
         ):
             async with async_test_recorder(
                 hass, wait_recorder=False, wait_recorder_setup=False
@@ -588,6 +590,7 @@ def _insert_states():
         patch.object(recorder, "db_schema", old_db_schema),
         patch.object(migration, "SCHEMA_VERSION", old_db_schema.SCHEMA_VERSION),
         patch.object(migration.StatesContextIDMigration, "migrate_data"),
+        patch.object(migration.EventIDPostMigration, "migrate_data"),
         patch(CREATE_ENGINE_TARGET, new=_create_engine_test),
     ):
         async with (
@@ -640,6 +643,7 @@ def _fetch_migrated_states():
             patch(
                 "sqlalchemy.schema.Index.create", autospec=True, wraps=Index.create
             ) as wrapped_idx_create,
+            patch.object(migration.EventIDPostMigration, "migrate_data"),
         ):
             async with async_test_recorder(
                 hass, wait_recorder=False, wait_recorder_setup=False
@@ -1127,6 +1131,7 @@ def _insert_events():
         patch.object(migration, "SCHEMA_VERSION", old_db_schema.SCHEMA_VERSION),
         patch.object(migration.EntityIDMigration, "migrate_data"),
         patch.object(migration.EntityIDPostMigration, "migrate_data"),
+        patch.object(migration.EventIDPostMigration, "migrate_data"),
         patch(CREATE_ENGINE_TARGET, new=_create_engine_test),
     ):
         async with (
@@ -1158,9 +1163,12 @@ def _fetch_migrated_states():
             return {state.state: state.entity_id for state in states}
 
     # Run again with new schema, let migration run
-    with patch(
-        "sqlalchemy.schema.Index.create", autospec=True, wraps=Index.create
-    ) as wrapped_idx_create:
+    with (
+        patch(
+            "sqlalchemy.schema.Index.create", autospec=True, wraps=Index.create
+        ) as wrapped_idx_create,
+        patch.object(migration.EventIDPostMigration, "migrate_data"),
+    ):
         async with (
             async_test_home_assistant() as hass,
             async_test_recorder(hass) as instance,
@@ -1169,7 +1177,6 @@ def _fetch_migrated_states():
 
             await hass.async_block_till_done()
             await async_wait_recording_done(hass)
-            await async_wait_recording_done(hass)
 
             states_by_state = await instance.async_add_executor_job(
                 _fetch_migrated_states

tests/components/recorder/test_migration_run_time_migrations_remember.py

@@ -115,7 +115,7 @@ def _create_engine_test(*args, **kwargs):
                 "event_context_id_as_binary": (0, 1),
                 "event_type_id_migration": (2, 1),
                 "entity_id_migration": (2, 1),
-                "event_id_post_migration": (0, 0),
+                "event_id_post_migration": (1, 1),
                 "entity_id_post_migration": (0, 1),
             },
             [
@@ -131,7 +131,7 @@ def _create_engine_test(*args, **kwargs):
                 "event_context_id_as_binary": (0, 0),
                 "event_type_id_migration": (2, 1),
                 "entity_id_migration": (2, 1),
-                "event_id_post_migration": (0, 0),
+                "event_id_post_migration": (1, 1),
                 "entity_id_post_migration": (0, 1),
             },
             ["ix_states_entity_id_last_updated_ts"],
@@ -143,13 +143,43 @@ def _create_engine_test(*args, **kwargs):
                 "event_context_id_as_binary": (0, 0),
                 "event_type_id_migration": (0, 0),
                 "entity_id_migration": (2, 1),
-                "event_id_post_migration": (0, 0),
+                "event_id_post_migration": (1, 1),
                 "entity_id_post_migration": (0, 1),
             },
             ["ix_states_entity_id_last_updated_ts"],
         ),
         (
             38,
+            {
+                "state_context_id_as_binary": (0, 0),
+                "event_context_id_as_binary": (0, 0),
+                "event_type_id_migration": (0, 0),
+                "entity_id_migration": (0, 0),
+                "event_id_post_migration": (1, 1),
+                "entity_id_post_migration": (0, 0),
+            },
+            [],
+        ),
+        (
+            43,
+            {
+                "state_context_id_as_binary": (0, 0),
+                "event_context_id_as_binary": (0, 0),
+                "event_type_id_migration": (0, 0),
+                "entity_id_migration": (0, 0),
+                # Schema was not bumped when the SQLite
+                # table rebuild was implemented so we need
+                # run event_id_post_migration up until
+                # schema 44 since its the first one we can
+                # be sure has the foreign key constraint was removed
+                # via https://github.com/home-assistant/core/pull/120779
+                "event_id_post_migration": (1, 1),
+                "entity_id_post_migration": (0, 0),
+            },
+            [],
+        ),
+        (
+            44,
             {
                 "state_context_id_as_binary": (0, 0),
                 "event_context_id_as_binary": (0, 0),
@@ -266,8 +296,14 @@ def patch_migrate(
     # the expected number of times.
     for migrator, mock in migrator_mocks.items():
         needs_migrate_calls, migrate_data_calls = expected_migrator_calls[migrator]
-        assert len(mock["needs_migrate"].mock_calls) == needs_migrate_calls
-        assert len(mock["migrate_data"].mock_calls) == migrate_data_calls
+        assert len(mock["needs_migrate"].mock_calls) == needs_migrate_calls, (
+            f"Expected {migrator} needs_migrate to be called {needs_migrate_calls} times,"
+            f" got {len(mock['needs_migrate'].mock_calls)}"
+        )
+        assert len(mock["migrate_data"].mock_calls) == migrate_data_calls, (
+            f"Expected {migrator} migrate_data to be called {migrate_data_calls} times, "
+            f"got {len(mock['migrate_data'].mock_calls)}"
+        )
 
 
 @pytest.mark.parametrize("enable_migrate_state_context_ids", [True])