Skip to content

Use ObjectEmpty() instead of array-only Empty() on a JSON object - #4548

Open
Daksha1611 wants to merge 1 commit into
openvinotoolkit:mainfrom
Daksha1611:fix-rapidjson-objectempty
Open

Use ObjectEmpty() instead of array-only Empty() on a JSON object#4548
Daksha1611 wants to merge 1 commit into
openvinotoolkit:mainfrom
Daksha1611:fix-rapidjson-objectempty

Conversation

@Daksha1611

@Daksha1611 Daksha1611 commented Sep 11, 2026

Copy link
Copy Markdown

🛠 Summary

Fixes #4547.

computeDeltaImpl() builds its result with delta.SetObject(), so nestedDelta is always an object, but it was tested with Value::Empty(). In the pinned RapidJSON that accessor is array-only:

// rapidjson/document.h:1649
bool Empty() const { RAPIDJSON_ASSERT(IsArray()); return data_.a.size == 0; }
// rapidjson/document.h:1197
bool ObjectEmpty() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.size == 0; }

RAPIDJSON_ASSERT is assert() and is not overridden anywhere in the tree, so a build with assertions live (-c dbg, Makefile:118) aborts with Assertion `IsArray()' failed as soon as a streamed delta contains a nested object present in both snapshots — ordinary streaming of a tool call with nested arguments. Under NDEBUG the assert disappears and data_.a.size aliases data_.o.size, so the wrong accessor returns the right number. Release behaviour is unchanged by this PR.

The four tool parsers computing the same kind of delta already use ObjectEmpty() (hermes3, llama3, mistral, phi4), so this brings the builder in line. The other two Value::Empty() call sites in src/ are genuinely arrays and are left alone.

The five delta assertions in partial_json_builder_test.cpp made the same misuse, so the suite aborted under -c dbg before reaching the production call; they now use ObjectEmpty(). Adds computeDeltaWithUnchangedNestedMember for the branch whose emptiness result decides the output. Note that test passes both before and after in a release build, for the aliasing reason above — its value is running correctly under assertions.

I have no OVMS build container available, so this is not compiled against the full tree and CI will need to confirm the build. I did reproduce the abort and the fix against the exact pinned RapidJSON revision from WORKSPACE:102. Draft for that reason.

🧪 Checklist

  • Unit tests added.
  • The documentation updated.
  • Change follows security best practices.

computeDeltaImpl() builds its result with delta.SetObject(), so nestedDelta
is always an object. Testing it with Value::Empty() is an array-only
accessor:

  // rapidjson/document.h:1649
  bool Empty() const { RAPIDJSON_ASSERT(IsArray()); return data_.a.size == 0; }
  // rapidjson/document.h:1197
  bool ObjectEmpty() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.size == 0; }

RAPIDJSON_ASSERT is assert() and is not overridden anywhere in the tree, so a
build with assertions live (-c dbg, see Makefile:118) aborts with
"Assertion `IsArray()' failed" as soon as a streamed delta contains a nested
object present in both snapshots - ordinary streaming of a tool call whose
arguments contain a nested JSON object.

Under NDEBUG the assert disappears and data_.a.size aliases data_.o.size
(both unions open with SizeType size), so the wrong accessor returns the
right number. That is why this has gone unnoticed; release behaviour is
unchanged by this commit.

The four tool parsers that compute the same kind of delta already use
ObjectEmpty() - hermes3, llama3, mistral and phi4 - so this brings the
builder in line with them. The other two Value::Empty() call sites in the
tree are genuinely arrays and are left alone: openai_api_handler.cpp:736
operates on a GetArray() result, and openai_responses.cpp:522 is guarded by
an IsArray() check immediately above.

Tests: the five delta assertions in partial_json_builder_test.cpp made the
same misuse, so the suite itself aborted under -c dbg before reaching the
production call. Adds computeDeltaWithUnchangedNestedMember, which pins the
branch whose emptiness result actually decides the output - an unchanged
nested object must contribute no member to the delta.
@Daksha1611
Daksha1611 marked this pull request as ready for review September 12, 2026 07:09
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.

computeDeltaImpl() calls array-only Empty() on a JSON object - aborts builds with assertions enabled

1 participant