diff --git a/server/api_server/graphql_endpoint.py b/server/api_server/graphql_endpoint.py index a6aa786c..61e65135 100755 --- a/server/api_server/graphql_endpoint.py +++ b/server/api_server/graphql_endpoint.py @@ -45,6 +45,20 @@ folder = apiPath # string, which mixed_type_sort_key sorts numerically on its own. _IP_SORT_FIELDS = {"devLastIP", "devPrimaryIPv4", "devPrimaryIPv6"} + +def _ip_sort_key(value): + """Sort key for an IP-address field: (0, long_value) for a real address, + (1, 0) for empty/unparseable - mirrors mixed_type_sort_key's bucketing + (valid values sort before the empty bucket in ascending order, and Python's + sorted(reverse=True) flips both together) instead of relying on + format_ip_long's -1 sentinel, which put blanks *before* real addresses in + ascending order - the opposite of every other column's convention.""" + long_value = format_ip_long(value) if value else -1 + if long_value < 0: + return (1, 0) + return (0, long_value) + + # In-memory cache for lang strings _langstrings_cache = {} # caches lists per file (core JSON or plugin) _langstrings_cache_mtime = {} # tracks last modified times @@ -403,7 +417,7 @@ class Query(ObjectType): # Numeric sort so e.g. 192.168.1.15 sorts before 192.168.1.105 devices_data = sorted( devices_data, - key=lambda x: format_ip_long(x.get(field) or ""), + key=lambda x: _ip_sort_key(x.get(field)), reverse=(sort_option.order.lower() == "desc"), ) else: diff --git a/server/plugins/freebox/freebox.py b/server/plugins/freebox/freebox.py index 89bd9fbd..ab60a60a 100755 --- a/server/plugins/freebox/freebox.py +++ b/server/plugins/freebox/freebox.py @@ -170,7 +170,11 @@ def main(): watched1=host.get("primary_name", "(unknown)"), watched2=host.get("vendor_name", "(unknown)"), watched3=map_device_type(host.get("host_type", "")), - watched4=datetime.fromtimestamp(ip.get("last_time_reachable", 0), tz=dt_timezone.utc).strftime(DATETIME_PATTERN), + # .get(..., 0) alone isn't enough: the Freebox API can return this + # key present but explicitly null, and dict.get()'s default only + # applies when the key is absent, not when its value is None - + # `or 0` catches both, avoiding a TypeError from fromtimestamp(None). + watched4=datetime.fromtimestamp(ip.get("last_time_reachable") or 0, tz=dt_timezone.utc).strftime(DATETIME_PATTERN), extra="", foreignKey=mac, ) diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 02429d9b..4e6bec71 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -372,14 +372,22 @@ def update_ipv4_ipv6(db): ) AS family_rn FROM CurrentScan WHERE scanLastIP NOT IN ({NULL_EQUIVALENTS_SQL}) + AND scanMac NOT IN ({NULL_EQUIVALENTS_SQL}) AND scanPresence = 1 ) - SELECT scanMac, family, scanLastIP FROM ranked WHERE family_rn = 1 + SELECT scanMac, family, scanLastIP FROM ranked ORDER BY scanMac, family, family_rn """).fetchall() per_mac = {} for row in rows: mac, family, ip = row["scanMac"], row["family"], row["scanLastIP"] + if family in per_mac.get(mac, {}): + # Already have a valid candidate for this (mac, family) from a more- + # recent row (rows arrive ordered by family_rn) - a malformed *newer* + # scanLastIP (passes the SQL-side ':' family heuristic but fails real + # IP parsing below) must not block an older, valid one for the same + # mac/family from being used instead. + continue try: ipaddress.ip_address(ip) # defensive re-validation of the SQL-side family classification except ValueError: diff --git a/test/api_endpoints/test_graphq_endpoints.py b/test/api_endpoints/test_graphq_endpoints.py index 82ec783b..b6a2d1da 100644 --- a/test/api_endpoints/test_graphq_endpoints.py +++ b/test/api_endpoints/test_graphq_endpoints.py @@ -121,9 +121,15 @@ def test_graphql_devices_parent_children_count_matches_recount(client, api_token @pytest.mark.parametrize("field", ["devLastIP", "devPrimaryIPv4", "devPrimaryIPv6"]) -def test_graphql_devices_sort_by_ip_is_numeric(client, api_token, field): +@pytest.mark.parametrize("order", ["ASC", "DESC"]) +def test_graphql_devices_sort_by_ip_is_numeric(client, api_token, field, order): """Sorting devices by an IP field must order numerically (e.g. 192.168.1.15 - before 192.168.1.105), not lexicographically. Regression guard for #1797. + before 192.168.1.105), not lexicographically, in both directions - and a + device with no value for that field must consistently land after every + real address in ASC and before every real address in DESC (matching + mixed_type_sort_key's convention for every other column), not wherever + format_ip_long's -1-for-empty sentinel happens to fall in raw numeric + order. Regression guard for #1797. Not seeded: reuses whatever devices already exist (see the docstring on test_graphql_devices_parent_children_count_matches_recount for why this @@ -132,7 +138,7 @@ def test_graphql_devices_sort_by_ip_is_numeric(client, api_token, field): query = { "query": f""" {{ - devices(options: {{sort: [{{field: "{field}", order: "ASC"}}]}}) {{ + devices(options: {{sort: [{{field: "{field}", order: "{order}"}}]}}) {{ devices {{ {field} }} @@ -144,9 +150,18 @@ def test_graphql_devices_sort_by_ip_is_numeric(client, api_token, field): assert resp.status_code == 200 values = [d[field] for d in resp.get_json()["data"]["devices"]["devices"]] - longs = [format_ip_long(v or "") for v in values] - assert longs == sorted(longs) + # Blank-value placement, independent of the non-blank values' numeric order: + # False (non-blank) sorts before True (blank), so a properly-grouped ASC + # result is already in sorted(is_blank) order; DESC is the reverse of that. + is_blank = [not v for v in values] + expected_blank_order = sorted(is_blank, reverse=(order == "DESC")) + assert is_blank == expected_blank_order, f"blank {field} values not grouped correctly for {order}: {values}" + + # Non-blank values must be in real numeric IP order for the requested direction. + non_blank_longs = [format_ip_long(v) for v in values if v] + expected = sorted(non_blank_longs, reverse=(order == "DESC")) + assert non_blank_longs == expected # --- SETTINGS TESTS --- diff --git a/test/scan/test_ip_format_and_locking.py b/test/scan/test_ip_format_and_locking.py index 435b0fda..d136e8e2 100644 --- a/test/scan/test_ip_format_and_locking.py +++ b/test/scan/test_ip_format_and_locking.py @@ -261,3 +261,60 @@ def test_dual_stack_presence_suppressed_row_excluded(scan_db, mock_ip_handlers): ).fetchone() assert row["devPrimaryIPv4"] == "192.168.1.70" assert row["devPrimaryIPv6"] in (None, "") + + +def test_dual_stack_blank_scan_mac_row_is_inert(scan_db, mock_ip_handlers): + """A CurrentScan row with a blank scanMac must never update any Devices + row, even if it has an otherwise-valid scanLastIP - matches the same + blank-scanMac guard create_new_devices() already applies (see the + scan-pipeline skill's Gotcha 5).""" + cur = scan_db.cursor() + cur.execute("INSERT INTO Devices (devMac, devPrimaryIPv4) VALUES (?, ?)", ("cc:cc:cc:cc:cc:07", "203.0.113.1")) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("", "192.168.1.99", "DOCKERDISC", "2025-01-01 03:00:00") + ) + scan_db.commit() + + db = Mock(sql_connection=scan_db, sql=cur) + device_handling.update_devices_data_from_scan(db) + device_handling.update_ipv4_ipv6(db) + + # The blank-scanMac row must not have created/updated any Devices row - + # in particular, it must not have overwritten the unrelated real device. + row = cur.execute( + "SELECT devPrimaryIPv4 FROM Devices WHERE devMac = ?", + ("cc:cc:cc:cc:cc:07",), + ).fetchone() + assert row["devPrimaryIPv4"] == "203.0.113.1" + + blank_mac_row = cur.execute("SELECT devMac FROM Devices WHERE devMac = ''").fetchone() + assert blank_mac_row is None + + +def test_dual_stack_malformed_newer_row_falls_back_to_valid_older_row(scan_db, mock_ip_handlers): + """When the most-recent CurrentScan row for a (mac, family) has a + malformed scanLastIP (passes the SQL-side ':' family heuristic but fails + real IP validation), an older but valid row for the same mac/family must + still be used instead of the family being dropped entirely this cycle.""" + cur = scan_db.cursor() + cur.execute("INSERT INTO Devices (devMac) VALUES (?)", ("cc:cc:cc:cc:cc:08",)) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:08", "999.999.999.999", "BUGGYPLG", "2025-01-01 05:00:00") + ) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:08", "172.16.0.5", "ARPSCAN", "2025-01-01 04:00:00") + ) + scan_db.commit() + + db = Mock(sql_connection=scan_db, sql=cur) + device_handling.update_devices_data_from_scan(db) + device_handling.update_ipv4_ipv6(db) + + row = cur.execute( + "SELECT devPrimaryIPv4 FROM Devices WHERE devMac = ?", + ("cc:cc:cc:cc:cc:08",), + ).fetchone() + assert row["devPrimaryIPv4"] == "172.16.0.5"