refactor(graph): one reader for LadybugDB QueryResult rows - #1369
Conversation
Four call sites read rows as rows = result.get_as_list() if hasattr(result, "get_as_list") else list(result) QueryResult has no get_as_list, so the guard is always false and all four have been taking the fallback since the ladybug rename. They work. The problem is the shape: it reads as though either branch might fire, and a fifth copy written without the `else` is a silent empty result, not an error. That fifth copy existed -- the vector-search HNSW reader removed in PR #1368, which answered 200 with zero rows for every query. Replaced with result_rows(), which uses the engine's documented has_next()/get_next() cursor, matching the row-to-dict readers already in service.py and engine.py. No behavior change; the fallback each site was silently taking is now the only path. Left alone: _copy_result_rows in materialize.py guards on get_as_arrow, which QueryResult does have, and service.py/engine.py build column-mapped dicts rather than positional rows. The tests are the other half. The old ones set get_as_list on a MagicMock, so hasattr was true under test and false against the engine -- the mock manufactured the method whose absence was the bug. Swapping in a fake with the engine's real surface made those tests hang instead of pass: MagicMock returns a truthy Mock from has_next(), so the cursor never terminates. That is the same lie in the other direction, and it is why FakeQueryResult now lives in tests/graph_api/conftest.py for anything reading rows off a result. test_results.py covers order, exhaustion, unnormalized rows, and a trap fake that fails if anything reaches for get_as_list again. Verified on the local stack: GET /vector (TABLE_INFO + COUNT), POST /vector/build, and the no-such-table path all behave as before. The TABLE_INFO read is the same query and reader materialize's _get_target_columns uses. Claude-Session: https://claude.ai/code/session_01HHvoJGP23TsTZZ8njeRTWj
github-actions
Bot
added
the
change:normal
|
Change classification: normal — platform or application code changed. A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The |
|
@claude please review this PR |
|
Claude finished @jfrench9's task in 1m 6s —— View job Review:
|
1 participant
Follow-up to #1368, which removed a reader that silently returned nothing. This removes the pattern that produced it.
What
Four call sites read rows this way:
QueryResulthas noget_as_list— ladybug 0.18.1 exposesget_all,rows_as_dict,get_as_arrowand ahas_next/get_nextcursor — so the guard is always false and all four have been taking the fallback since the rename. They work today. Replaced withresult_rows()incore/ladybug/results.py, built on the documented cursor, matching the row-to-dict readers already inservice.pyandengine.py.vector_search.py:263(build, COUNT),:386(info, TABLE_INFO),:401(info, COUNT)tables/materialize.py:153(_get_target_columns, TABLE_INFO)No behavior change: the fallback every site was silently taking is now the only path.
Why, given they all work
The shape is the defect, not the four instances. It reads as though either branch might fire, so a fifth copy written without the
elselooks like a style choice rather than a bug. That fifth copy is exactly what #1368 deleted — the HNSW reader that answered200 {"results": []}for every query, with nothing logged, because a missing method behindhasattris not an error. Two files, four sites, one demonstrated failure is enough evidence to collapse it.The honest limit: a helper only prevents recurrence if the next person reaches for it. It's exported from
core.ladybugand documented as the one way in, but that's a convention, not a guarantee.Deliberately left alone
_copy_result_rowsinmaterialize.pyguards onget_as_arrow, whichQueryResultdoes have. Live guard, correct as written.service.pyandengine.pybuild column-mapped dicts with row caps for API responses. Different job, correct, and riskier to touch than to leave.The tests were half the bug
The old tests set
get_as_liston aMagicMock, sohasattrwas true under test and false against the engine — the mock manufactured the very method whose absence was the defect.Swapping in a fake with the engine's real surface didn't just flip those tests to passing; it made them hang.
MagicMock.has_next()returns a truthyMock, so the cursor never terminates. That's the same lie pointing the other way, and it's whyFakeQueryResultnow lives intests/graph_api/conftest.pyfor anything reading rows off a result.test_results.pycovers order, cursor exhaustion, unnormalized rows (TABLE_INFOcallers distinguish tuple rows from dict rows), and a trap fake that raises if anything reaches forget_as_listagain.1393 graph_api tests pass;
just test-codeclean.Verification
On the local stack, against the subgraph from #1368:
GET /vector→row_count: 8— exercises TABLE_INFO and COUNT through the new readerPOST /vector/buildbackend="hnsw"→row_count: 8200with null, not a 500The
TABLE_INFOread is the same query and the same reader_get_target_columnsuses during materialization, so that row shape is covered live rather than only under mocks.🤖 Generated with Claude Code
https://claude.ai/code/session_01HHvoJGP23TsTZZ8njeRTWj