BE: FREEBOX dual stack IP fix #1804

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:

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,
                     )

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:

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 ---

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"