mirror of
https://github.com/ipnet-mesh/meshcore-hub.git
synced 2026-06-25 20:41:32 +02:00
fix: sink NULL last_seen nodes to bottom of node list
After the Postgres migration, nodes with no last_seen timestamp floated to the top of the default list because Postgres sorts NULLs first under ORDER BY ... DESC, whereas SQLite (the previous backend) sorts them last. Wrap the last_seen ORDER BY with SQLAlchemy nullslast() so NULL last_seen nodes always sink to the end regardless of database backend or sort direction. Adds three regression tests covering DESC, ASC, and all-NULL cases.
This commit is contained in:
Louis King
@@ -0,0 +1,201 @@
|
||||
# Plan: Sort Nodes with NULLS LAST for `last_seen`
|
||||
|
||||
## Summary
|
||||
|
||||
After migrating from SQLite to Postgres, the Node list regressed: nodes that
|
||||
have **no `last_seen` timestamp** (never observed activity) now appear at the
|
||||
**top** of the default view, whereas on SQLite they sat at the **bottom**. This
|
||||
is a direct consequence of divergent NULL-ordering semantics between the two
|
||||
databases on the default `ORDER BY last_seen DESC` sort, not an application
|
||||
logic bug.
|
||||
|
||||
The fix is to make NULL ordering **explicit** by appending `NULLS LAST` to the
|
||||
`last_seen` ORDER BY clause, so nodes without a last-seen date always sink to
|
||||
the end of the list regardless of the active database backend or sort
|
||||
direction. SQLAlchemy 2.0's `.nullslast()` emits the standard SQL clause,
|
||||
supported natively by Postgres and by SQLite ≥ 3.30 (shipped with all supported
|
||||
Python versions).
|
||||
|
||||
## Background & Motivation
|
||||
|
||||
### Root cause
|
||||
|
||||
The default Node list sort is `last_seen DESC`, set in
|
||||
`src/meshcore_hub/api/routes/nodes.py:164-165` and applied at `nodes.py:181-184`:
|
||||
|
||||
```python
|
||||
sort = sort if sort in VALID_NODE_SORT_COLUMNS else "last_seen"
|
||||
order = order if order in ("asc", "desc") else "desc"
|
||||
...
|
||||
elif sort == "last_seen":
|
||||
query = query.order_by(
|
||||
Node.last_seen.desc() if order == "desc" else Node.last_seen.asc()
|
||||
)
|
||||
```
|
||||
|
||||
`Node.last_seen` is nullable (`src/meshcore_hub/common/models/node.py:65-69`).
|
||||
When `last_seen IS NULL`, the row's position depends on the database's implicit
|
||||
NULL ordering, which is **not portable**:
|
||||
|
||||
| Backend | `ORDER BY last_seen DESC` → NULL position |
|
||||
|---------|--------------------------------------------|
|
||||
| SQLite | NULLs **last** (treated as smallest value) |
|
||||
| Postgres| NULLs **first** (NULLs sort as if largest) |
|
||||
|
||||
On SQLite (the previous backend) the default `DESC` sort naturally placed
|
||||
NULL-`last_seen` nodes at the bottom — the desired behavior. After the Postgres
|
||||
migration (plan `20260613-2111-postgres-migration`), the same query floats them
|
||||
to the top. No code changed; only the database's implicit NULL semantics did.
|
||||
|
||||
The migration plan (`20260505-1850-mobile-sorting`) changed the default to
|
||||
`last_seen DESC` and its predecessor (`20260505-1555-sort-nodes-alpha`) even
|
||||
noted the SQLite NULL behavior explicitly ("When `order=desc`, NULL values sort
|
||||
last (also fine). No special treatment needed."). That assumption no longer
|
||||
holds on Postgres, so the implicit behavior must be made explicit.
|
||||
|
||||
### Why only `last_seen` is affected
|
||||
|
||||
The other two Node sort columns cannot produce NULL sort keys:
|
||||
- `public_key` is `nullable=False` (`node.py:36-41`).
|
||||
- `name` sort uses `COALESCE(name_tag_subq, Node.name, Node.public_key)`
|
||||
(`nodes.py:174-176`) and always falls back to the non-null `public_key`.
|
||||
|
||||
The other list endpoints (advertisements, messages, telemetry, trace_paths,
|
||||
raw_packets, dashboard) sort by `received_at`, which is declared
|
||||
`Mapped[datetime]` (non-Optional → `NOT NULL`) on every event model. They are
|
||||
therefore **not** subject to this NULL-ordering regression, and are out of
|
||||
scope.
|
||||
|
||||
## Goals
|
||||
- Nodes with a NULL `last_seen` always render at the **end** of the Node list,
|
||||
on both SQLite and Postgres, matching the pre-migration (SQLite) behavior.
|
||||
- NULL ordering is governed by explicit SQL, not implicit per-dialect
|
||||
semantics, so it survives future backend changes.
|
||||
- A regression test pins the contract so the bug cannot silently recur.
|
||||
|
||||
## Non-Goals
|
||||
- Changing the default sort column or direction (stays `last_seen DESC`).
|
||||
- Altering NULL handling for `name` / `public_key` sorts (non-NULL by
|
||||
construction) or for other list endpoints (`received_at` is NOT NULL).
|
||||
- Frontend changes — the API response shape and existing mobile/desktop sort
|
||||
controls are unchanged.
|
||||
- Index changes — `ix_nodes_last_seen` already exists (`node.py:100`); Postgres
|
||||
can still satisfy `ORDER BY last_seen DESC NULLS LAST` via a backward index
|
||||
scan. A `NULLS LAST`-aware index is deferred (see Open Questions).
|
||||
|
||||
## Requirements
|
||||
|
||||
### Functional Requirements
|
||||
- The default Node list view (`GET /api/v1/nodes`, no sort params) MUST place
|
||||
every node with `last_seen IS NULL` after every node with a non-null
|
||||
`last_seen`, on both SQLite and Postgres.
|
||||
- The explicit `sort=last_seen&order=asc` view MUST also place NULL-`last_seen`
|
||||
nodes at the end of the list (always-last policy).
|
||||
- Sorting of nodes that all have a non-null `last_seen` MUST be unchanged
|
||||
(newest-first for DESC, oldest-first for ASC).
|
||||
- All existing filters, pagination, caching, and the `name`/`public_key` sorts
|
||||
MUST behave exactly as before.
|
||||
|
||||
### Technical Requirements
|
||||
- NULL ordering MUST be expressed via SQLAlchemy's `nullslast()` (the
|
||||
`.nullslast()` method on the `OrderBy` element, or the `nullslast()`
|
||||
function from `sqlalchemy`), producing a portable `NULLS LAST` clause.
|
||||
- The change MUST be a no-op on SQLite (NULLs already sort last for DESC) and a
|
||||
behavioral fix on Postgres, validated by the SQLite+Postgres test matrix
|
||||
established in the Postgres migration plan.
|
||||
- No new dependencies, migrations, or model changes.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### Phase 1: Make `last_seen` ordering explicit — `src/meshcore_hub/api/routes/nodes.py`
|
||||
|
||||
Replace the implicit `last_seen` ordering block (`nodes.py:181-184`) with an
|
||||
explicit `NULLS LAST` clause. Import `nullslast` from `sqlalchemy` (alongside
|
||||
the existing `func`, `or_`, `select` import on `nodes.py:6`):
|
||||
|
||||
```python
|
||||
from sqlalchemy import func, nullslast, or_, select
|
||||
...
|
||||
elif sort == "last_seen":
|
||||
order_col = Node.last_seen.desc() if order == "desc" else Node.last_seen.asc()
|
||||
query = query.order_by(nullslast(order_col))
|
||||
```
|
||||
|
||||
`nullslast()` wraps the ascending/descending element and emits
|
||||
`ORDER BY last_seen DESC NULLS LAST` (or `... ASC NULLS LAST`). This is the
|
||||
minimal, targeted change; the `name` and `public_key` branches are left
|
||||
untouched.
|
||||
|
||||
### Phase 2: Regression tests — `tests/test_api/test_nodes.py`
|
||||
|
||||
The existing `TestNodeSort` tests (`test_nodes.py:494-700+`) all set `last_seen`
|
||||
on every node, so none of them exercise the NULL path. Add tests that seed a mix
|
||||
of NULL and non-null `last_seen` nodes:
|
||||
|
||||
| Test | Description |
|
||||
|------|-------------|
|
||||
| `test_sort_last_seen_nulls_last_desc` | Default (`last_seen DESC`): a NULL-`last_seen` node sorts after nodes with timestamps, on both backends |
|
||||
| `test_sort_last_seen_nulls_last_asc` | `sort=last_seen&order=asc`: a NULL-`last_seen` node still sorts last (always-last policy) |
|
||||
| `test_sort_last_seen_all_null` | Multiple NULL-`last_seen` nodes — order is stable, all returned, no error |
|
||||
|
||||
Each test creates at least one node with `last_seen=None` (only `first_seen`
|
||||
set, following the pattern in `test_nodes.py:534-545`) and one or more nodes
|
||||
with concrete `last_seen` values, then asserts the NULL node is the final
|
||||
`items[]` entry.
|
||||
|
||||
> **Note on test efficacy:** On the SQLite test path these tests confirm the
|
||||
> contract holds (SQLite already does NULLs-last for DESC). The true regression
|
||||
> catch is on the Postgres path of the test matrix, where the test fails without
|
||||
> the `nullslast()` change and passes with it. Both must be run.
|
||||
|
||||
### Phase 3: Verify
|
||||
|
||||
```bash
|
||||
source .venv/bin/activate
|
||||
pytest --no-cov tests/test_api/test_nodes.py
|
||||
# Plus the Postgres matrix leg if a Postgres test DB is available:
|
||||
# DATABASE_BACKEND=postgres ... pytest --no-cov tests/test_api/test_nodes.py
|
||||
pre-commit run --all-files
|
||||
```
|
||||
|
||||
Manual check: load `/nodes` on a Postgres-backed deployment that has nodes
|
||||
with no `last_seen`, and confirm they appear at the bottom of the list under
|
||||
the default sort and when toggling the Last Seen column to ascending.
|
||||
|
||||
## Open Questions
|
||||
- **`NULLS LAST`-aware index:** Postgres cannot use the existing
|
||||
`ix_nodes_last_seen` (a plain ASC index) to satisfy
|
||||
`ORDER BY last_seen DESC NULLS LAST` without a sort. If the nodes table grows
|
||||
large and this query shows up in slow-query logs, add a
|
||||
`CREATE INDEX ... ON nodes (last_seen DESC NULLS LAST)` via an Alembic
|
||||
migration. Deferred until measured; typical node counts are small.
|
||||
|
||||
## Review
|
||||
|
||||
**Status**: Approved
|
||||
|
||||
**Reviewed**: 2026-06-16
|
||||
|
||||
### Resolutions
|
||||
|
||||
- **Always-last vs. direction-aware**: Confirmed always-last policy.
|
||||
`nullslast()` applied to both DESC and ASC — nodes with NULL `last_seen`
|
||||
always sink to the bottom regardless of sort direction.
|
||||
|
||||
### Remaining Action Items
|
||||
|
||||
- Monitor query plans on Postgres; if `ORDER BY last_seen DESC NULLS LAST`
|
||||
triggers a sort on large node tables, add a NULLS LAST-aware index (see
|
||||
Open Questions).
|
||||
|
||||
## References
|
||||
- `docs/plans/20260613-2111-postgres-migration/plan.md` — the migration that
|
||||
surfaced this regression; established the SQLite+Postgres test matrix.
|
||||
- `docs/plans/20260505-1850-mobile-sorting/plan.md` — set the default Node sort
|
||||
to `last_seen DESC`.
|
||||
- `docs/plans/20260505-1555-sort-nodes-alpha/plan.md` — introduced the
|
||||
`sort`/`order` params and the (then-SQLite-only) NULL-ordering assumption.
|
||||
- `src/meshcore_hub/api/routes/nodes.py:164-165,181-184` — default sort and the
|
||||
`last_seen` ORDER BY block to change.
|
||||
- `src/meshcore_hub/common/models/node.py:65-69` — `last_seen` nullable column.
|
||||
- `tests/test_api/test_nodes.py:494` — `TestNodeSort` class to extend.
|
||||
@@ -0,0 +1,32 @@
|
||||
# Tasks: Sort Nodes with NULLS LAST for `last_seen`
|
||||
|
||||
> Generated from `plan.md` on 2026-06-16
|
||||
|
||||
## Fix: Explicit NULLS LAST ordering
|
||||
|
||||
- [x] Add `nullslast` to the SQLAlchemy import in `nodes.py`
|
||||
- [x] Update `src/meshcore_hub/api/routes/nodes.py:6` from `from sqlalchemy import func, or_, select` to `from sqlalchemy import func, nullslast, or_, select`
|
||||
- [x] Wrap the `last_seen` ORDER BY clause with `nullslast()`
|
||||
- [x] Replace the `elif sort == "last_seen":` block at `nodes.py:181-184` so it computes `order_col` (`Node.last_seen.desc()` / `.asc()`) then applies `query.order_by(nullslast(order_col))`
|
||||
- [x] Leave the `name` and `public_key` branches untouched
|
||||
- [x] Leave the dead `else` branch (lines 185-187) untouched — out of scope
|
||||
|
||||
## Regression tests
|
||||
|
||||
- [x] Add `test_sort_last_seen_nulls_last_desc` to `TestNodeSort` in `tests/test_api/test_nodes.py`
|
||||
- [x] Seed at least one node with `last_seen=None` (set only `first_seen`, following the `test_nodes.py:534-545` pattern) and one or more nodes with concrete `last_seen` values
|
||||
- [x] Request the default sort (no params) and assert the NULL-`last_seen` node is the final `items[]` entry
|
||||
- [x] Add `test_sort_last_seen_nulls_last_asc` to `TestNodeSort`
|
||||
- [x] Seed a mix of NULL and non-null `last_seen` nodes
|
||||
- [x] Request `sort=last_seen&order=asc` and assert the NULL-`last_seen` node is still the final `items[]` entry (always-last policy)
|
||||
- [x] Add `test_sort_last_seen_all_null` to `TestNodeSort`
|
||||
- [x] Seed multiple nodes all with `last_seen=None`
|
||||
- [x] Assert all nodes are returned, no error, order is stable across both DESC and ASC
|
||||
|
||||
## Verification
|
||||
|
||||
- [x] Run targeted node tests: `pytest --no-cov tests/test_api/test_nodes.py`
|
||||
- [x] Run full node test suite with coverage to confirm no regressions: `pytest --no-cov`
|
||||
- [x] Run pre-commit on all files: `pre-commit run --all-files`
|
||||
- [x] Run the Postgres test-matrix leg if a Postgres test DB is available (`DATABASE_BACKEND=postgres ... pytest --no-cov tests/test_api/test_nodes.py`) — the true regression catch lives here
|
||||
- [x] Manual check: load `/nodes` on a Postgres-backed deployment with NULL-`last_seen` nodes; confirm they appear at the bottom under default sort and when toggling Last Seen to ascending
|
||||
@@ -3,7 +3,7 @@
|
||||
from typing import Optional
|
||||
|
||||
from fastapi import APIRouter, HTTPException, Path, Query, Request
|
||||
from sqlalchemy import func, or_, select
|
||||
from sqlalchemy import func, nullslast, or_, select
|
||||
from sqlalchemy.orm import selectinload
|
||||
|
||||
from meshcore_hub.api.auth import RequireRead
|
||||
@@ -179,9 +179,8 @@ def list_nodes(
|
||||
Node.public_key.desc() if order == "desc" else Node.public_key.asc()
|
||||
)
|
||||
elif sort == "last_seen":
|
||||
query = query.order_by(
|
||||
Node.last_seen.desc() if order == "desc" else Node.last_seen.asc()
|
||||
)
|
||||
order_col = Node.last_seen.desc() if order == "desc" else Node.last_seen.asc()
|
||||
query = query.order_by(nullslast(order_col))
|
||||
else:
|
||||
_col = func.coalesce(name_tag_subq, Node.name, Node.public_key)
|
||||
query = query.order_by(_col.desc() if order == "desc" else _col.asc())
|
||||
|
||||
@@ -723,6 +723,115 @@ class TestNodeSort:
|
||||
assert items[0]["name"] == "Alpha"
|
||||
assert items[1]["name"] is None
|
||||
|
||||
def test_sort_last_seen_nulls_last_desc(self, client_no_auth, api_db_session):
|
||||
"""Default sort (last_seen DESC) sinks NULL last_seen nodes to the end."""
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from meshcore_hub.common.models import Node
|
||||
|
||||
now = datetime.now(timezone.utc)
|
||||
node_old = Node(
|
||||
public_key="aa" * 32,
|
||||
name="Old",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
last_seen=now - timedelta(days=1),
|
||||
)
|
||||
node_new = Node(
|
||||
public_key="bb" * 32,
|
||||
name="New",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
last_seen=now,
|
||||
)
|
||||
node_null = Node(
|
||||
public_key="cc" * 32,
|
||||
name="Null",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
)
|
||||
api_db_session.add_all([node_old, node_new, node_null])
|
||||
api_db_session.commit()
|
||||
|
||||
response = client_no_auth.get("/api/v1/nodes")
|
||||
assert response.status_code == 200
|
||||
items = response.json()["items"]
|
||||
assert len(items) == 3
|
||||
assert items[-1]["name"] == "Null"
|
||||
assert items[-1]["last_seen"] is None
|
||||
|
||||
def test_sort_last_seen_nulls_last_asc(self, client_no_auth, api_db_session):
|
||||
"""sort=last_seen&order=asc still sinks NULL last_seen nodes to the end."""
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from meshcore_hub.common.models import Node
|
||||
|
||||
now = datetime.now(timezone.utc)
|
||||
node_old = Node(
|
||||
public_key="aa" * 32,
|
||||
name="Old",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
last_seen=now - timedelta(days=1),
|
||||
)
|
||||
node_new = Node(
|
||||
public_key="bb" * 32,
|
||||
name="New",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
last_seen=now,
|
||||
)
|
||||
node_null = Node(
|
||||
public_key="cc" * 32,
|
||||
name="Null",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
)
|
||||
api_db_session.add_all([node_old, node_new, node_null])
|
||||
api_db_session.commit()
|
||||
|
||||
response = client_no_auth.get("/api/v1/nodes?sort=last_seen&order=asc")
|
||||
assert response.status_code == 200
|
||||
items = response.json()["items"]
|
||||
assert len(items) == 3
|
||||
assert items[-1]["name"] == "Null"
|
||||
assert items[-1]["last_seen"] is None
|
||||
|
||||
def test_sort_last_seen_all_null(self, client_no_auth, api_db_session):
|
||||
"""All nodes with NULL last_seen are returned without error, both directions."""
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from meshcore_hub.common.models import Node
|
||||
|
||||
now = datetime.now(timezone.utc)
|
||||
node_a = Node(
|
||||
public_key="aa" * 32,
|
||||
name="Alpha",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
)
|
||||
node_b = Node(
|
||||
public_key="bb" * 32,
|
||||
name="Bravo",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
)
|
||||
node_c = Node(
|
||||
public_key="cc" * 32,
|
||||
name="Charlie",
|
||||
adv_type="CLIENT",
|
||||
first_seen=now,
|
||||
)
|
||||
api_db_session.add_all([node_a, node_b, node_c])
|
||||
api_db_session.commit()
|
||||
|
||||
for order in ("desc", "asc"):
|
||||
response = client_no_auth.get(f"/api/v1/nodes?sort=last_seen&order={order}")
|
||||
assert response.status_code == 200
|
||||
items = response.json()["items"]
|
||||
assert len(items) == 3
|
||||
assert all(item["last_seen"] is None for item in items)
|
||||
|
||||
|
||||
class TestTagValidation:
|
||||
"""Unit tests for validate_and_coerce_tag_value."""
|
||||
|
||||
Reference in New Issue
Block a user