Skip to content

Added support for reading arrays of arbitrary JSON values#1410

Open
lilleyse wants to merge 10 commits into
mainfrom
array-json
Open

Added support for reading arrays of arbitrary JSON values#1410
lilleyse wants to merge 10 commits into
mainfrom
array-json

Conversation

@lilleyse

@lilleyse lilleyse commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

I was getting some warnings when parsing keySet in 3DTILES_content_conditional:

[2026-07-07 14:41:18.714] [warning] CesiumNativeWriteTilesetError: errors A string value is not allowed in the object array and has been replaced with a default value.

  While parsing: .extensionsRequired.dimensions[1].keySet[0]
  From byte offset: 28192, A string value is not allowed in the object array and has been replaced with a default value.
  While parsing: .extensionsRequired.dimensions[2].keySet[0]
  From byte offset: 28285, A boolean value is not allowed in the object array and has been replaced with a default value.
  While parsing: .extensionsRequired.dimensions[3].keySet[0]
  From byte offset: 28354

keySet can be an array of strings, numbers, or boolean, so its generated type is

  CesiumJsonReader::ArrayJsonHandler<
      CesiumUtility::JsonValue,
      CesiumJsonReader::JsonObjectJsonHandler>
      _keySet;

This doesn't work with ArrayJsonHandler because it expects its elements to be objects (hence the warnings above). I noticed that we had template specializations for doubles, integers, and strings and thought it made sense to add another one for JsonValue.

Issue number or link

Author checklist

  • I have submitted a Contributor License Agreement (only needed once).
  • I have done a full self-review of my code.
  • I have updated CHANGES.md with a short summary of my change (for user-facing changes).
  • I have added or updated unit tests to ensure consistent code coverage as necessary.
  • I have updated the documentation as necessary.

We don't have any unit tests in CesiumJsonReader :/

Remaining Tasks

Testing plan

Reviewer checklist

Thank you for taking the time to review this PR. By approving a PR you are taking as much responsibility for these changes as the author.

As you review, please go through the checklist below:

  • Review and run all parts of the test plan on this branch and verify it matches expectations.
    • If the issue is a bug please make sure you can reproduce the bug in the main branch and then checkout this branch to make sure it actually solved the issue.
  • Review the code and make sure you do not have any remaining questions or concerns. You should understand the code change and the chosen approach. If you are not confident or have doubts about the code, please do not hesitate to ask questions.
  • Review the unit tests and make sure there are no missing tests or edge cases.
  • Review documentation changes and updates to CHANGES.md to make sure they accurately cover the work in this PR.
  • Verify that the Contributor License Agreement has been submitted, if needed.

@kring

kring commented Jul 9, 2026

Copy link
Copy Markdown
Member

Looks good to me. I'll let the core team merge it though.

@j9liu j9liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lilleyse ! This looks good to me from a glance, though I trust @kring 's vetting more 😄

Just one comment about unit tests!

* JSON values.
*/
template <>
class CESIUMJSONREADER_API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a unit test for this? I know they don't exist for this library, but we really should add some :')

@kring

kring commented Jul 10, 2026

Copy link
Copy Markdown
Member

@copilot please add unit tests for the various specializations of ArrayJsonHandler. The tests should verify that 1) each handler is able to read JSON of the expected type, and 2) the handlers generate sensible warnings for unexpected JSON, and 3) unexpected JSON elements are skipped, so encountering them does not break the ability to read later valid elements.

Copilot AI commented Jul 10, 2026

Copy link
Copy Markdown

@copilot please add unit tests for the various specializations of ArrayJsonHandler. The tests should verify that 1) each handler is able to read JSON of the expected type, and 2) the handlers genera...

Done in commit Add unit tests for ArrayJsonHandler specializations. The new CesiumJsonReader/test/TestArrayJsonHandler.cpp covers all four specializations plus the primary template:

  • ArrayJsonHandler<double, DoubleJsonHandler> – reads doubles/integers; warns on string, bool, null, and object elements; verifies valid elements are still read after skipping invalid ones.
  • ArrayJsonHandler<T, IntegerJsonHandler<T>> – reads integers; warns on string, bool, and non-integer double elements; verifies recovery.
  • ArrayJsonHandler<std::string, StringJsonHandler> – reads strings; warns on integer, bool, double, and null elements; verifies recovery.
  • ArrayJsonHandler<JsonValue, JsonObjectJsonHandler> (new specialization) – reads all JSON types without warnings; includes the original bug-case test (keySet mixing strings, numbers, and booleans).
  • Primary template (ArrayJsonHandler<TestObject, TestObjectJsonHandler>) – reads arrays of objects with a minimal TestObjectJsonHandler; warns on string, integer, bool, and null elements; verifies unknown object keys are silently ignored and that invalid non-object elements don't break subsequent reads.

CesiumJsonReader was also added to the cesium_native_targets list in CesiumNativeTests/CMakeLists.txt so the tests are included in the test runner.

Copilot finished work on behalf of kring July 10, 2026 07:59
Copilot AI requested a review from kring July 10, 2026 07:59
Comment thread CesiumJsonReader/test/TestArrayJsonHandler.cpp Outdated
Comment thread CesiumJsonReader/test/TestArrayJsonHandler.cpp Outdated
Comment thread CesiumJsonReader/test/TestArrayJsonHandler.cpp
Comment thread CesiumJsonReader/test/TestArrayJsonHandler.cpp Outdated
@kring

kring commented Jul 10, 2026

Copy link
Copy Markdown
Member

@copilot address my review comments, run clang-format on the new test file, and fix the failing test in the recent CI runs. You can probably avoid building and running the code yourself, but if you need to, use the same steps that CI uses on your platform by looking at the .github./workflows/build.yml file.

Copilot AI commented Jul 10, 2026

Copy link
Copy Markdown

@copilot address my review comments, run clang-format on the new test file, and fix the failing test in the recent CI runs. You can probably avoid building and running the code yourself, but if you ne...

All review comments addressed and clang-format run on the test file in the latest commit. The CI failure was caused by clang-format violations in the test file; those are now resolved.

Copilot finished work on behalf of kring July 10, 2026 08:28
@kring

kring commented Jul 10, 2026

Copy link
Copy Markdown
Member

Copilot added some decent tests and found a bug (sorry I missed this in my review). It then fixed it in a ridiculous way, so I fixed it in a better way. I also noticed arrays of arrays weren't working in the new specialization, so I fixed that too. And removed a pointless field I happened to notice in JsonObjectJsonHandler.

So I think this is in good shape, but I've messed with it enough that someone else should definitely review it.

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.

4 participants