refactor(graph): one reader for LadybugDB QueryResult rows

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",
 ]

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

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]

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
 

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

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

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

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)