diff --git a/robosystems/graph_api/core/ladybug/__init__.py b/robosystems/graph_api/core/ladybug/__init__.py index b98a18564..5a5ec771c 100644 --- a/robosystems/graph_api/core/ladybug/__init__.py +++ b/robosystems/graph_api/core/ladybug/__init__.py @@ -4,6 +4,7 @@ - ``LadybugConnectionPool`` — shared, thread-safe connections per database - ``LadybugDatabaseManager`` — database lifecycle and the blue-green swap - ``LadybugService`` — the query, health and metrics surface the routers use +- ``result_rows`` — the one way to read rows out of a ``QueryResult`` """ from .config import get_database_memory_config @@ -14,6 +15,7 @@ get_connection_pool, initialize_connection_pool, ) +from .results import result_rows from .service import ( LadybugService, get_ladybug_service, @@ -38,5 +40,6 @@ "get_ladybug_service", "init_ladybug_service", "initialize_connection_pool", + "result_rows", "validate_cypher_query", ] diff --git a/robosystems/graph_api/core/ladybug/results.py b/robosystems/graph_api/core/ladybug/results.py new file mode 100644 index 000000000..7e0adb8a6 --- /dev/null +++ b/robosystems/graph_api/core/ladybug/results.py @@ -0,0 +1,34 @@ +"""Reading rows out of a LadybugDB ``QueryResult``. + +One helper, because the alternative has already cost us a bug. Callers used +to write:: + + rows = result.get_as_list() if hasattr(result, "get_as_list") else list(result) + +``QueryResult`` has no ``get_as_list`` — it exposes ``has_next``/``get_next``, +``get_all`` and ``rows_as_dict`` — so that guard is always false and every +caller silently took the fallback. It reads as though either branch might +fire, which is what makes it dangerous: the vector-search HNSW reader wrote +the same guard *without* the ``else``, and answered HTTP 200 with zero rows +for every query until the branch was removed (PR #1368). Nothing raised, +because a missing method behind ``hasattr`` is not an error. + +Use this instead of hand-rolling the loop. For rows keyed by column name, +the row-to-dict readers in ``service.py`` and ``engine.py`` do that job — +this one stays positional, and returns rows exactly as the engine yields +them so callers can keep their own shape handling. +""" + +from typing import Any + + +def result_rows(result: Any) -> list[Any]: + """Return every row of ``result`` as a list, in engine order. + + Rows come back as the engine yields them — no normalization — so a caller + that distinguishes tuple rows from dict rows can still do so. + """ + rows: list[Any] = [] + while result.has_next(): + rows.append(result.get_next()) + return rows diff --git a/robosystems/graph_api/routers/databases/tables/materialize.py b/robosystems/graph_api/routers/databases/tables/materialize.py index 72ca9a0ed..dc794f02b 100644 --- a/robosystems/graph_api/routers/databases/tables/materialize.py +++ b/robosystems/graph_api/routers/databases/tables/materialize.py @@ -17,6 +17,7 @@ from robosystems.config import env from robosystems.graph_api.core.duckdb import quote_identifier from robosystems.graph_api.core.ladybug import get_ladybug_service +from robosystems.graph_api.core.ladybug.results import result_rows from robosystems.graph_api.models.fork import ( ForkFromParentRequest, ForkFromParentResponse, @@ -150,7 +151,7 @@ def _get_target_columns( try: with ladybug_service.db_manager.connection_pool.get_connection(graph_id) as conn: result = conn.execute(f"CALL TABLE_INFO('{table_name}') RETURN *") - rows = result.get_as_list() if hasattr(result, "get_as_list") else list(result) + rows = result_rows(result) columns = [] for row in rows: # TABLE_INFO returns: [index, name, type, default, isPrimaryKey] diff --git a/robosystems/graph_api/routers/databases/vector_search.py b/robosystems/graph_api/routers/databases/vector_search.py index f44b0000c..805c50f61 100644 --- a/robosystems/graph_api/routers/databases/vector_search.py +++ b/robosystems/graph_api/routers/databases/vector_search.py @@ -42,6 +42,7 @@ from fastapi import APIRouter, HTTPException, Query, status from pydantic import BaseModel, Field, field_validator, model_validator +from robosystems.graph_api.core.ladybug.results import result_rows from robosystems.logger import logger router = APIRouter(prefix="/databases", tags=["Vector Index"]) @@ -258,7 +259,7 @@ def _build_hnsw_index( try: result = conn.execute(f"MATCH (n:{table_name}) RETURN COUNT(n)") - rows = result.get_as_list() if hasattr(result, "get_as_list") else list(result) + rows = result_rows(result) first = list(rows[0]) if rows else [] row_count = int(first[0]) if first else 0 except Exception: @@ -311,11 +312,7 @@ async def vector_info( # are always explicit. try: info_result = conn.execute(f"CALL TABLE_INFO('{table_name}') RETURN *") - info_rows = ( - info_result.get_as_list() - if hasattr(info_result, "get_as_list") - else list(info_result) - ) + info_rows = result_rows(info_result) col_names = set() for row in info_rows: r = list(row) if not isinstance(row, (list, tuple)) else row @@ -327,7 +324,7 @@ async def vector_info( return None result = conn.execute(f"MATCH (n:{table_name}) RETURN COUNT(n)") - rows = result.get_as_list() if hasattr(result, "get_as_list") else list(result) + rows = result_rows(result) first = list(rows[0]) if rows else [] row_count = int(first[0]) if first else 0 diff --git a/tests/graph_api/conftest.py b/tests/graph_api/conftest.py index 159ac001d..31c952264 100644 --- a/tests/graph_api/conftest.py +++ b/tests/graph_api/conftest.py @@ -50,3 +50,27 @@ async def sleep(_seconds): return None monkeypatch.setattr(task_sse, "asyncio", _NoSleepAsyncio()) + + +class FakeQueryResult: + """A LadybugDB ``QueryResult`` with the surface the engine actually has. + + Use this instead of ``MagicMock`` wherever code reads rows off a result. + A mock satisfies ``hasattr`` for every name and returns a truthy object + from every call, which is how a reader that branched on the non-existent + ``get_as_list`` passed its tests while returning zero rows in production + (PR #1368). ladybug 0.18.1 offers ``get_all``, ``rows_as_dict``, + ``get_as_arrow`` and the cursor pair below -- and no ``get_as_list``. + """ + + def __init__(self, rows=()): + self._rows = list(rows) + self._i = 0 + + def has_next(self): + return self._i < len(self._rows) + + def get_next(self): + row = self._rows[self._i] + self._i += 1 + return row diff --git a/tests/graph_api/core/ladybug/test_results.py b/tests/graph_api/core/ladybug/test_results.py new file mode 100644 index 000000000..41ebaa473 --- /dev/null +++ b/tests/graph_api/core/ladybug/test_results.py @@ -0,0 +1,86 @@ +"""Tests for ``result_rows``, the single reader for a LadybugDB QueryResult. + +Deliberately no ``MagicMock`` here. A mock answers ``hasattr`` for every +attribute, and that is precisely how the defect this helper replaces stayed +invisible: the old callers branched on ``hasattr(result, "get_as_list")``, +the tests mocked ``get_as_list`` into existence, and the branch that ran in +production — the one that returned nothing — was never the branch under +test. These fakes expose only what ``ladybug.QueryResult`` actually has. +""" + +import pytest + +from robosystems.graph_api.core.ladybug.results import result_rows + + +class FakeQueryResult: + """The engine's cursor surface: ``has_next`` / ``get_next``, nothing else. + + Mirrors ladybug 0.18.1, which offers ``get_all``, ``rows_as_dict``, + ``get_as_arrow`` and this cursor pair — and no ``get_as_list``. + """ + + def __init__(self, rows): + self._rows = list(rows) + self._i = 0 + + def has_next(self): + return self._i < len(self._rows) + + def get_next(self): + row = self._rows[self._i] + self._i += 1 + return row + + +@pytest.mark.unit +class TestResultRows: + def test_reads_every_row_in_order(self): + result = FakeQueryResult([["a", 1], ["b", 2], ["c", 3]]) + + assert result_rows(result) == [["a", 1], ["b", 2], ["c", 3]] + + def test_empty_result_is_an_empty_list(self): + assert result_rows(FakeQueryResult([])) == [] + + def test_exhausts_the_cursor(self): + """A partial read would silently truncate a caller's row count.""" + result = FakeQueryResult([[1], [2], [3]]) + + result_rows(result) + + assert not result.has_next() + + def test_returns_rows_unnormalized(self): + """Callers distinguish tuple rows from dict rows (TABLE_INFO does), so + the helper must not coerce either into the other.""" + rows = [("tuple", 1), {"dict": 2}, ["list", 3]] + + assert result_rows(FakeQueryResult(rows)) == rows + + +@pytest.mark.unit +class TestDoesNotDependOnGetAsList: + """The regression. ``get_as_list`` does not exist on the engine's result; + reading through it — or branching on it — yields nothing at all.""" + + def test_reads_a_result_that_has_no_get_as_list(self): + result = FakeQueryResult([["only-row"]]) + assert not hasattr(result, "get_as_list") + + assert result_rows(result) == [["only-row"]] + + def test_never_reaches_for_get_as_list(self): + """Locks the guard out: touching that name at all is the old bug.""" + + class TrapResult(FakeQueryResult): + def __getattr__(self, name): + if name == "get_as_list": + raise AssertionError( + "result_rows must not branch on get_as_list — the engine has no " + "such method, so the branch never fires and the fallback is what " + "actually runs" + ) + raise AttributeError(name) + + assert result_rows(TrapResult([["row"]])) == [["row"]] diff --git a/tests/graph_api/routers/databases/tables/test_materialize_helpers.py b/tests/graph_api/routers/databases/tables/test_materialize_helpers.py index 753594e3d..46feb6cc4 100644 --- a/tests/graph_api/routers/databases/tables/test_materialize_helpers.py +++ b/tests/graph_api/routers/databases/tables/test_materialize_helpers.py @@ -23,6 +23,7 @@ _lbug_type_to_duck, checkpoint_with_retry, ) +from tests.graph_api.conftest import FakeQueryResult MODULE = "robosystems.graph_api.routers.databases.tables.materialize" @@ -258,9 +259,7 @@ def test_nullified_target_columns_keep_their_slot(self): class TestGetTargetColumns: def _service_returning(self, rows): conn = MagicMock() - result = MagicMock() - result.get_as_list.return_value = rows - conn.execute.return_value = result + conn.execute.return_value = FakeQueryResult(rows) service = MagicMock() service.db_manager.connection_pool.get_connection.return_value.__enter__.return_value = conn return service diff --git a/tests/graph_api/routers/databases/test_vector_hnsw.py b/tests/graph_api/routers/databases/test_vector_hnsw.py index d9485523f..853c30bea 100644 --- a/tests/graph_api/routers/databases/test_vector_hnsw.py +++ b/tests/graph_api/routers/databases/test_vector_hnsw.py @@ -21,6 +21,7 @@ _require_writer, _validate_identifier, ) +from tests.graph_api.conftest import FakeQueryResult MODULE = "robosystems.graph_api.routers.databases.vector_search" @@ -84,9 +85,7 @@ def test_blocks_replica_with_501(self, monkeypatch): class TestBuildHnswIndex: def test_builds_and_reports_row_count(self): conn = MagicMock() - result = MagicMock() - result.get_as_list.return_value = [[1500]] - conn.execute.return_value = result + conn.execute.return_value = FakeQueryResult([[1500]]) service = _service_with(conn) with patch(f"{MODULE}._get_ladybug_service", return_value=service): @@ -102,7 +101,7 @@ def test_checkpoints_after_building(self): """Without the CHECKPOINT the index lives only in the WAL and is lost on the next engine restart.""" conn = MagicMock() - conn.execute.return_value = MagicMock(get_as_list=MagicMock(return_value=[[0]])) + conn.execute.return_value = FakeQueryResult([[0]]) service = _service_with(conn) with patch(f"{MODULE}._get_ladybug_service", return_value=service): @@ -127,7 +126,7 @@ def test_row_count_failure_degrades_to_zero(self): def execute(sql): if sql.startswith("MATCH"): raise RuntimeError("count failed") - return MagicMock() + return FakeQueryResult() conn.execute.side_effect = execute service = _service_with(conn)