From 72da7eed7bdbfaf82327b79f15060f4a54560c4a Mon Sep 17 00:00:00 2001 From: Perry Mosbacher Date: Thu, 3 Sep 2026 11:06:44 -0400 Subject: [PATCH] fix(companion): make bulk contact import match stored advert types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Import repeater contacts" wrote nothing whenever a contact-type filter was supplied, and reported success having imported zero rows. `adverts.contact_type` holds the *display* name written through `handler_helpers.discovery.NODE_TYPE_NAMES` — "Chat Node", "Repeater", "Room Server", "Sensor". The import API accepts MeshCore's names — "companion", "repeater", "room_server", "sensor", validated in `companion_endpoints.import_repeater_contacts`. `companion_import_repeater_contacts` compared them directly: query += f" AND contact_type IN ({placeholders})" so "repeater" never matched "Repeater", "room_server" never matched "Room Server", and "companion" never matched "Chat Node" at all. On a live repeater with 285 adverts, every one of the four permitted values selected 0 rows, as did all four together; only an unfiltered import worked. A second defect had the same cause. The adv_type lookup normalised the stored name (`lower()`, spaces to underscores) but mapped it with the API-name table, where "chat_node" is absent — so chat contacts imported as adv_type 0 rather than 1, even on an unfiltered import. That is silently wrong rather than empty, and needs a manual `UPDATE companion_contacts SET adv_type=1 WHERE adv_type=0` to repair. Both sites derived the mapping independently, which is how they drifted. They now share one `_ADVERT_TYPE_BY_STORED` table keyed on the normalised stored form: the filter reverse-maps the requested API names onto the stored keys and compares `LOWER(REPLACE(TRIM(contact_type), ' ', '_'))`, and the adv_type comes from the same table. Filtering and limiting stay in SQL. An unrecognised type still selects nothing rather than silently widening the query. Tested against the real stored forms: 6 of the 8 new tests fail on current dev (each of the four type filters, all four combined, and the adv_type mapping) and all 8 pass with this change. Co-Authored-By: Claude Opus 5 --- repeater/data_acquisition/sqlite_handler.py | 52 ++++++- ...test_companion_import_repeater_contacts.py | 129 ++++++++++++++++++ 2 files changed, 174 insertions(+), 7 deletions(-) create mode 100644 tests/test_companion_import_repeater_contacts.py diff --git a/repeater/data_acquisition/sqlite_handler.py b/repeater/data_acquisition/sqlite_handler.py index a7cd822..3d54ff3 100644 --- a/repeater/data_acquisition/sqlite_handler.py +++ b/repeater/data_acquisition/sqlite_handler.py @@ -3831,6 +3831,30 @@ class SQLiteHandler: logger.error(f"Failed to upsert companion contact: {e}") return False + # ``adverts.contact_type`` stores the *display* name written via + # handler_helpers.discovery.NODE_TYPE_NAMES ("Chat Node", "Repeater", + # "Room Server", "Sensor"). The import API speaks MeshCore's names + # ("companion", "repeater", "room_server", "sensor"). Normalising the stored + # form once, here, keeps the SQL filter and the adv_type assignment from + # drifting apart -- they were previously derived independently, so the + # filter compared display names against API names and matched nothing. + _ADVERT_TYPE_BY_STORED = { + "chat_node": 1, + "chat": 1, + "client": 1, + "companion": 1, + "repeater": 2, + "room_server": 3, + "room": 3, + "sensor": 4, + } + _ADVERT_TYPE_BY_API = {"companion": 1, "repeater": 2, "room_server": 3, "sensor": 4} + + @staticmethod + def _normalise_advert_type(raw) -> str: + """Fold a stored contact_type to its lookup key ("Chat Node" -> "chat_node").""" + return (raw or "").lower().replace(" ", "_").strip() + def companion_import_repeater_contacts( self, companion_hash: str, @@ -3844,7 +3868,6 @@ class SQLiteHandler: imported first. Optional hours filters to adverts seen within the last N hours; optional limit caps how many contacts are imported. """ - type_map = {"companion": 1, "repeater": 2, "room_server": 3, "sensor": 4} try: with self._connect() as conn: conn.row_factory = sqlite3.Row @@ -3854,9 +3877,24 @@ class SQLiteHandler: ) params: list = [] if contact_types: - placeholders = ",".join("?" * len(contact_types)) - query += f" AND contact_type IN ({placeholders})" - params.extend(contact_types) + wanted = { + self._ADVERT_TYPE_BY_API[t] + for t in contact_types + if t in self._ADVERT_TYPE_BY_API + } + stored = sorted( + key + for key, adv in self._ADVERT_TYPE_BY_STORED.items() + if adv in wanted + ) + if not stored: + return 0 + placeholders = ",".join("?" * len(stored)) + query += ( + " AND LOWER(REPLACE(TRIM(contact_type), ' ', '_')) " + f"IN ({placeholders})" + ) + params.extend(stored) if hours is not None: cutoff = time.time() - (hours * 3600) query += " AND last_seen >= ?" @@ -3871,9 +3909,9 @@ class SQLiteHandler: now = time.time() contact_rows = [] for row in rows: - raw_type = row["contact_type"] or "" - normalized_type = raw_type.lower().replace(" ", "_").strip() - adv_type = type_map.get(normalized_type, 0) + adv_type = self._ADVERT_TYPE_BY_STORED.get( + self._normalise_advert_type(row["contact_type"]), 0 + ) contact_rows.append( ( companion_hash, diff --git a/tests/test_companion_import_repeater_contacts.py b/tests/test_companion_import_repeater_contacts.py new file mode 100644 index 0000000..85d1e54 --- /dev/null +++ b/tests/test_companion_import_repeater_contacts.py @@ -0,0 +1,129 @@ +"""Regression tests for bulk 'import repeater contacts'. + +``adverts.contact_type`` stores the *display* name written through +``handler_helpers.discovery.NODE_TYPE_NAMES`` -- "Chat Node", "Repeater", +"Room Server", "Sensor". The import API accepts MeshCore's names -- "companion", +"repeater", "room_server", "sensor" (validated in +``companion_endpoints.import_repeater_contacts``). + +``companion_import_repeater_contacts`` compared the two directly: + + query += f" AND contact_type IN ({placeholders})" # API names + # vs stored display names + +so every filtered import matched zero rows and reported success having written +nothing. Separately the adv_type lookup normalised "Chat Node" to "chat_node", +which was absent from its map, so chat contacts imported as adv_type 0 rather +than 1 even on an unfiltered import. + +Both derived the mapping independently; they now share one table. +""" +import os +import shutil +import sqlite3 +import sys +import tempfile +import time +from pathlib import Path + +import pytest + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from repeater.data_acquisition.sqlite_handler import SQLiteHandler # noqa: E402 + +# Exactly the forms the repeater writes. +SEEDS = [ + ("aa" * 32, "RepeaterOne", "Repeater", 2), + ("bb" * 32, "ChatOne", "Chat Node", 1), + ("cc" * 32, "RoomOne", "Room Server", 3), + ("dd" * 32, "SensorOne", "Sensor", 4), +] + + +@pytest.fixture +def handler(): + d = tempfile.mkdtemp() + h = SQLiteHandler(Path(d)) + con = sqlite3.connect(os.path.join(d, "repeater.db")) + now = time.time() + for pk, name, ctype, _ in SEEDS: + con.execute( + "INSERT INTO adverts" + " (timestamp, pubkey, node_name, is_repeater, contact_type, latitude," + " longitude, first_seen, last_seen, advert_count, is_new_neighbor, zero_hop)" + " VALUES (?, ?, ?, ?, ?, 0, 0, ?, ?, 1, 0, 0)", + (now, pk, name, 1 if ctype == "Repeater" else 0, ctype, now, now), + ) + con.commit() + con.close() + yield h, d + shutil.rmtree(d, ignore_errors=True) + + +def _imported(d, companion="0x01"): + con = sqlite3.connect(os.path.join(d, "repeater.db")) + try: + return { + (r[0].hex() if isinstance(r[0], (bytes, bytearray)) else r[0]): r[1] + for r in con.execute( + "select pubkey, adv_type from companion_contacts where companion_hash=?", + (companion,), + ) + } + finally: + con.close() + + +def test_unfiltered_import_takes_everything(handler): + h, d = handler + n = h.companion_import_repeater_contacts("0x01") + assert n == len(SEEDS) + assert len(_imported(d)) == len(SEEDS) + + +@pytest.mark.parametrize( + "api_name,expected_pubkey", + [ + ("repeater", "aa" * 32), + ("companion", "bb" * 32), + ("room_server", "cc" * 32), + ("sensor", "dd" * 32), + ], +) +def test_each_contact_type_filter_matches_its_stored_display_name( + handler, api_name, expected_pubkey +): + """The bug: every one of these returned 0.""" + h, d = handler + n = h.companion_import_repeater_contacts("0x01", contact_types=[api_name]) + assert n == 1, "contact_types=[%r] imported nothing" % api_name + assert set(_imported(d)) == {expected_pubkey} + + +def test_all_four_filters_together(handler): + h, d = handler + n = h.companion_import_repeater_contacts( + "0x01", contact_types=["companion", "repeater", "room_server", "sensor"] + ) + assert n == len(SEEDS) + + +def test_adv_type_is_correct_for_every_stored_name(handler): + """'Chat Node' must import as adv_type 1, not 0.""" + h, d = handler + h.companion_import_repeater_contacts("0x01") + got = _imported(d) + for pk, _name, stored, expected in SEEDS: + assert got[pk] == expected, "%r imported as adv_type %s, expected %s" % ( + stored, + got[pk], + expected, + ) + assert 0 not in got.values(), "some contact imported with adv_type 0 (unset)" + + +def test_unknown_contact_type_still_imports_nothing(handler): + """A type the API would reject must not silently widen the query.""" + h, _ = handler + assert h.companion_import_repeater_contacts("0x01", contact_types=["nonsense"]) == 0