Skip to content

fix(search): preserve compound requested fields - #1325

Open
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1284-list-valued-fields
Open

fix(search): preserve compound requested fields#1325
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1284-list-valued-fields

Conversation

@Enferlain

Copy link
Copy Markdown

What does this PR do?

Fixes #1284.

Requested extra fields in search_graph were serialized only for scalar JSON values. Stored arrays and objects fell through to an empty cell in table output and were silently misrepresented.

This change centralizes requested-field serialization: table output emits compact JSON text for compound values, while JSON output copies the original JSON value so its type is preserved. Regression coverage verifies both output formats.

Verification

  • make -f Makefile.cbm test-focused TEST_SUITES='cypher extraction registry pipeline mcp index_resilience'
  • Result: 884 passed, 2 skipped

Checklist

  • Every commit is signed off (git commit -s) and follows the Contributor License Agreement
  • Full test suite passes locally (make -f Makefile.cbm test) — focused sanitized suites passed
  • Lint passes (make -f Makefile.cbm lint-ci) — not run locally
  • New behavior is covered by regression tests that would fail without this fix

Signed-off-by: imi <hoshinoimi@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Reviewed this properly, and the short version is: it is ready, and we would like to merge it as shown — may we?

It is still marked draft, so I am asking rather than just merging. If you were planning further changes, say so and we will wait. If draft is simply left over, mark it ready (or reply here) and it goes in.

What I verified, since it is worth knowing the review was real:

The bug reproduces on today's main. All three emitters type-switch on scalars only, so a node property whose stored value is a JSON array or object — base_classes, decorators — silently falls through: the text and TOON tables render an empty "-" cell, and format:"json" puts null in the row. That is #1284's exact symptom.

What I liked is that you did not patch one emitter. You found all three duplicated scalar-only switches — table, skeleton tree, and JSON — and centralised the cell logic into sg_toon_property_cell, so the two text paths can no longer drift apart. They had already drifted, incidentally: their int/real ordering differed. That is the fix built the way we would have wanted it built.

The type-preservation split is the right call too: compact JSON where the output has to stay inside one table cell, and yyjson_val_mut_copy where the consumer is getting real JSON — rather than the stringified-JSON-inside-JSON compromise that would have been the easy way out.

I also checked the text path stays safe: compact JSON contains ", which forces needs_quotes()append_quoted(), so quotes and backslashes are escaped, control bytes are JSON-escaped, and invalid UTF-8 is sanitised. The cell remains one parseable, valid-UTF-8 column, consistent with the recent tree-format UTF-8 hardening.

Both regression tests bind. Reverting the fix turns the text assert red (HandlerBase/Audited versus the old "-") and the JSON assert red (["HandlerBase","Audited"] versus the old null).

No defects found. Two entirely optional notes:

  • ASSERT_NOT_NULL(strstr(inner, "base_classes")) passes without the fix, since the column header alone satisfies it — harmless next to the value asserts that do the real work.
  • Worth a CHANGELOG sentence eventually: format:"json" rows can now contain nested arrays/objects where they previously held only scalars or null. Anyone consuming rows positionally will want to know.

Thank you for the minimal public repro in #1284 as well — it made confirming both the symptom and the fix scope trivial, and that is a real kindness to whoever reviews.

@Enferlain
Enferlain marked this pull request as ready for review July 31, 2026 18:12
@Enferlain
Enferlain requested a review from DeusData as a code owner July 31, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

search_graph returns null for list-valued requested fields despite stored graph properties

2 participants