From 23cdce03e819aafa6a1a46fb39d7cc6e1d3ee5ed Mon Sep 17 00:00:00 2001 From: agessaman Date: Tue, 21 Apr 2026 17:04:51 -0700 Subject: [PATCH] feat(db_migrations, message_handler, repeater_manager): enhance purging log functionality - Added a new migration to include a 'details' column in the 'purging_log' table for improved logging of actions. - Refactored the `log_purging_action` method in `RepeaterManager` to handle both new and legacy schemas, ensuring compatibility with the updated database structure. - Updated `MessageHandler` to utilize the new logging method, replacing direct database updates with a unified logging approach. - Added tests to verify the presence of the new column and the correct functionality of the logging mechanism across different scenarios. --- modules/db_migrations.py | 7 ++++ modules/message_handler.py | 15 +++---- modules/repeater_manager.py | 74 +++++++++++++++++++++++++--------- tests/test_db_migrations.py | 37 +++++++++++++++++ tests/test_message_handler.py | 3 +- tests/test_repeater_manager.py | 24 +++++++++++ 6 files changed, 132 insertions(+), 28 deletions(-) diff --git a/modules/db_migrations.py b/modules/db_migrations.py index 7ef9427..b04b57f 100644 --- a/modules/db_migrations.py +++ b/modules/db_migrations.py @@ -484,6 +484,12 @@ def _m0011_repeater_and_graph_indexes(cursor: sqlite3.Cursor) -> None: ) +def _m0012_purging_log_details_column(cursor: sqlite3.Cursor) -> None: + """Add details column for newer purging log entries.""" + if _table_exists(cursor, "purging_log"): + _add_column(cursor, "purging_log", "details", "TEXT") + + # --------------------------------------------------------------------------- # Migration registry — append new entries here, never remove or reorder. # --------------------------------------------------------------------------- @@ -502,6 +508,7 @@ MIGRATIONS: list[MigrationEntry] = [ (9, "optional repeater/graph indexes", _m0009_repeater_optional_indexes), (10, "create repeater/graph tables", _m0010_create_repeater_and_graph_tables), (11, "repeater/graph indexes", _m0011_repeater_and_graph_indexes), + (12, "purging_log: add details column", _m0012_purging_log_details_column), ] diff --git a/modules/message_handler.py b/modules/message_handler.py index 86c5c72..5564af4 100644 --- a/modules/message_handler.py +++ b/modules/message_handler.py @@ -3346,12 +3346,9 @@ class MessageHandler: await self.bot.repeater_manager.check_and_auto_purge() - self.bot.repeater_manager.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ( - 'new_contact_discovered', - f'New contact discovered: {contact_name} (key: {public_key[:16]}...)', - ), + self.bot.repeater_manager.log_purging_action( + "new_contact_discovered", + f"New contact discovered: {contact_name} (key: {public_key[:16]}...)", ) return @@ -3408,9 +3405,9 @@ class MessageHandler: # Log the new contact discovery if hasattr(self.bot, 'repeater_manager'): - self.bot.repeater_manager.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('new_contact_discovered', f'New contact discovered: {contact_name} (key: {public_key[:16]}...)') + self.bot.repeater_manager.log_purging_action( + "new_contact_discovered", + f"New contact discovered: {contact_name} (key: {public_key[:16]}...)", ) except Exception as e: diff --git a/modules/repeater_manager.py b/modules/repeater_manager.py index 2d1410d..a4bd27c 100644 --- a/modules/repeater_manager.py +++ b/modules/repeater_manager.py @@ -88,6 +88,7 @@ class RepeaterManager: self._auto_purge_lock = asyncio.Lock() self._auto_purge_in_progress = False self._purge_inflight_keys = set() + self._purging_log_has_details: Optional[bool] = None def _start_purge_attempt(self, public_key: str, contact_type: str) -> bool: """Mark a purge as in-flight; return False when another attempt is already active.""" @@ -103,6 +104,43 @@ class RepeaterManager: """Clear in-flight marker for a purge attempt.""" self._purge_inflight_keys.discard(public_key) + def _refresh_purging_log_details_capability(self) -> bool: + """Return True if purging_log.details exists.""" + if self._purging_log_has_details is not None: + return self._purging_log_has_details + try: + with self.db_manager.connection() as conn: + rows = conn.execute("PRAGMA table_info(purging_log)").fetchall() + self._purging_log_has_details = any(row[1] == "details" for row in rows) + except Exception as e: + self.logger.debug(f"Unable to inspect purging_log schema: {e}") + self._purging_log_has_details = False + return self._purging_log_has_details + + def log_purging_action(self, action: str, details: str) -> None: + """Insert purging log rows across both new and legacy schemas.""" + try: + if self._refresh_purging_log_details_capability(): + self.db_manager.execute_update( + "INSERT INTO purging_log (action, details) VALUES (?, ?)", + (action, details), + ) + return + self.db_manager.execute_update( + "INSERT INTO purging_log (action, public_key, name, reason) VALUES (?, ?, ?, ?)", + (action, "", action, details), + ) + except Exception as e: + err = str(e) + if "no column named details" in err: + self._purging_log_has_details = False + self.db_manager.execute_update( + "INSERT INTO purging_log (action, public_key, name, reason) VALUES (?, ?, ?, ?)", + (action, "", action, details), + ) + return + self.logger.error(f"Failed to write purging log action '{action}': {e}") + def _init_repeater_tables(self): """Ensure repeater-specific tables exist (created by migrations).""" try: @@ -2538,9 +2576,9 @@ class RepeaterManager: self.logger.error(f"Failed to discover companion contacts: {e}") # Step 3: Log the post-purge management action - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('post_purge_management', 'Enabled manual contact addition and initiated companion contact discovery') + self.log_purging_action( + "post_purge_management", + "Enabled manual contact addition and initiated companion contact discovery", ) self.logger.info("Post-purge contact management completed") @@ -2682,9 +2720,9 @@ class RepeaterManager: # Log the management action if actions_taken: - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('contact_management', f'Contact list management: {"; ".join(actions_taken)}') + self.log_purging_action( + "contact_management", + f'Contact list management: {"; ".join(actions_taken)}', ) return { @@ -2725,9 +2763,9 @@ class RepeaterManager: self.logger.info(f"✅ Successfully removed stale contact: {contact_name}") # Log the removal - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('stale_contact_removal', f'Removed stale contact: {contact_name} (last seen {contact["days_stale"]} days ago)') + self.log_purging_action( + "stale_contact_removal", + f'Removed stale contact: {contact_name} (last seen {contact["days_stale"]} days ago)', ) else: error_code = result.payload.get('error_code', 'unknown') if hasattr(result, 'payload') else 'unknown' @@ -3026,9 +3064,9 @@ class RepeaterManager: # Log the addition if successful if contact_addition_successful: - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('contact_addition', f'Added discovered contact: {contact_name} - {reason}') + self.log_purging_action( + "contact_addition", + f"Added discovered contact: {contact_name} - {reason}", ) self.logger.info(f"Successfully added contact '{contact_name}': {reason}") return True @@ -3056,9 +3094,9 @@ class RepeaterManager: self.logger.debug(f"Manual contact addition toggle result: {result}") # Log the action - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('manual_add_toggle', f'{"Enabled" if enabled else "Disabled"} manual contact addition - {reason}') + self.log_purging_action( + "manual_add_toggle", + f'{"Enabled" if enabled else "Disabled"} manual contact addition - {reason}', ) return True @@ -3086,9 +3124,9 @@ class RepeaterManager: self.logger.debug(f"Discovery result: {result}") # Log the action - self.db_manager.execute_update( - 'INSERT INTO purging_log (action, details) VALUES (?, ?)', - ('companion_discovery', f'Manual companion contact discovery - {reason}') + self.log_purging_action( + "companion_discovery", + f"Manual companion contact discovery - {reason}", ) return True diff --git a/tests/test_db_migrations.py b/tests/test_db_migrations.py index ef43dcb..70ee962 100644 --- a/tests/test_db_migrations.py +++ b/tests/test_db_migrations.py @@ -286,3 +286,40 @@ class TestSchema: (idx,), ) assert cur.fetchone() is not None + + def test_purging_log_has_details_column_after_full_migration(self, runner, conn): + runner.run() + cursor = conn.cursor() + assert _column_exists(cursor, "purging_log", "details") is True + + def test_migration_adds_purging_log_details_for_legacy_schema(self, conn, logger): + conn.execute(""" + CREATE TABLE IF NOT EXISTS schema_version ( + version INTEGER NOT NULL, + description TEXT, + applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP + ) + """) + for version, description, _ in MIGRATIONS: + if version <= 11: + conn.execute( + "INSERT INTO schema_version (version, description) VALUES (?, ?)", + (version, description), + ) + conn.execute(""" + CREATE TABLE IF NOT EXISTS purging_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + timestamp TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + action TEXT NOT NULL, + public_key TEXT NOT NULL, + name TEXT NOT NULL, + reason TEXT + ) + """) + conn.commit() + + runner = MigrationRunner(conn, logger) + runner.run() + + cursor = conn.cursor() + assert _column_exists(cursor, "purging_log", "details") is True diff --git a/tests/test_message_handler.py b/tests/test_message_handler.py index 9ecee5f..161422e 100644 --- a/tests/test_message_handler.py +++ b/tests/test_message_handler.py @@ -1910,6 +1910,7 @@ def new_contact_env(mock_logger): ) rm.manage_contact_list = AsyncMock(return_value={"success": True}) rm.add_companion_from_contact_data = AsyncMock(return_value=True) + rm.log_purging_action = Mock() rm.db_manager = Mock() rm.db_manager.execute_update = Mock() rm._is_repeater_device = Mock(return_value=False) @@ -1933,7 +1934,7 @@ class TestHandleNewContactAutoManage: rm.track_contact_advertisement.assert_awaited_once() mesh.commands.add_contact.assert_not_called() rm.add_companion_from_contact_data.assert_not_called() - rm.db_manager.execute_update.assert_called() + rm.log_purging_action.assert_called_once() async def test_device_mode_no_bot_add_contact(self, new_contact_env): bot, handler, rm, mesh = new_contact_env diff --git a/tests/test_repeater_manager.py b/tests/test_repeater_manager.py index 7f37c34..5310680 100644 --- a/tests/test_repeater_manager.py +++ b/tests/test_repeater_manager.py @@ -90,6 +90,30 @@ class TestDetermineContactRole: assert rm._determine_contact_role({}) == "companion" +class TestPurgingLogCompatibility: + def test_log_purging_action_uses_details_when_available(self, rm): + rm._purging_log_has_details = True + rm.db_manager.execute_update = Mock() + + rm.log_purging_action("contact_management", "managed contacts") + + rm.db_manager.execute_update.assert_called_once_with( + "INSERT INTO purging_log (action, details) VALUES (?, ?)", + ("contact_management", "managed contacts"), + ) + + def test_log_purging_action_falls_back_to_legacy_columns(self, rm): + rm._purging_log_has_details = False + rm.db_manager.execute_update = Mock() + + rm.log_purging_action("contact_management", "managed contacts") + + rm.db_manager.execute_update.assert_called_once_with( + "INSERT INTO purging_log (action, public_key, name, reason) VALUES (?, ?, ?, ?)", + ("contact_management", "", "contact_management", "managed contacts"), + ) + + # --------------------------------------------------------------------------- # _determine_device_type # ---------------------------------------------------------------------------