Skip to content

feat(java): add cycle detection and depth limit to JSON serializer - #383

Open
buongarzoni wants to merge 1 commit into
masterfrom
feat/add-json-serializer-depth-limith-and-cycle-detection
Open

buongarzoni wants to merge 1 commit into
masterfrom
feat/add-json-serializer-depth-limith-and-cycle-detection

Conversation

@buongarzoni

Copy link
Copy Markdown
Collaborator

Description of the change

JsonSerializerImpl had no depth limit and no cycle detection. User data that contains itself (e.g. map.put("self", map)) caused a StackOverflowError on the caller's thread, and very deeply nested data produced huge payloads.

Changes

  • Track, by identity, the values currently being serialized. A value that refers back to one of them is written as "<circular reference>". The same value reached twice without a cycle is still serialized both times.
  • Values nested deeper than maxDepth (default 100) are written as "<max depth exceeded>". Neither case throws.
  • New constructor JsonSerializerImpl(boolean prettyPrint, int maxDepth) sets the limit; use it with ConfigBuilder.jsonSerializer(...).

Tests

Added tests for cycles through maps, collections, arrays and JsonSerializable, a shared value that isn't a cycle, a custom maxDepth, 100,000 levels of nesting (previously a stack overflow), and rejecting a maxDepth below 1.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@buongarzoni buongarzoni self-assigned this Oct 5, 2026
@rollbar-circleci-machine

Copy link
Copy Markdown

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

LGTM. No defects confirmed on the changed lines. I did not run the tests; everything below comes from reading the code in the checkout.

What I verified

  • Cycle detection is sound. serializeNested (JsonSerializerImpl.java:207-234) handles every container type that serializeValue sends to it (:191-193).
    • It tracks object identity (:113), so a map that contains itself never has its own recursive hashCode()/equals() called.
    • Values are removed again in a finally (:231-233), so a value reached through two branches is still written twice, and the set is cleaned up if asJson() throws.
  • Depth counting matches the Javadoc (:83-85) and both depth tests (JsonSerializerImplTest.java:316-356).
    • The root is level 0 (:114). Map entries and array elements go one level deeper (:172, :306). A JsonSerializable does not add a level (:222).
    • With level >= maxDepth (:213), maxDepth = N writes exactly N nested containers.
  • The default limit is not too tight. The payload's own structure uses only the first few levels, so the default DEFAULT_MAX_DEPTH = 100 leaves plenty of room for user data.
  • Truncation and sending use the serializer (PayloadTruncator.java:40,47). The truncation helpers do not recurse into user maps (TruncationHelper.java:165-180), so they do not add a recursion path of their own.
  • Tests should compile and pass.
    • The test changes, including dropping the (Object) cast at JsonSerializerImplTest.java:228, compile under Java 8 type inference, and the build uses release 8 (build.gradle.kts:58-64).
    • Checkstyle runs only on main sources (gradle/quality.gradle:8).
    • The new Javadoc <p> layout matches existing code (VersionHelper.java:6-8).

Notes outside the changed lines (not findings)

  • Pre-existing bug: maps with non-String keys. In asMap (JsonSerializerImpl.java:311-326), the (Map<String, Object>) cast is unchecked and cannot fail at runtime, so the ClassCastException fallback that converts keys with toString() never runs. A map with non-String keys (e.g. Map<Integer, ?> in custom data) instead fails at serializeString(builder, entry.getKey()) (:165), where the compiler inserts a cast to String. That throws ClassCastException and fails the whole payload. This PR did not cause it; it is worth a follow-up.
  • Unchanged by design: arbitrary objects, such as JVMTI-captured locals, are still written via toString() and are not walked. A toString() that recurses forever can still overflow the stack (:251-262). The existing comment there says this is deliberate.
  • Possible doc addition: a caller-supplied maxDepth smaller than the payload's own structure (roughly fewer than 8 levels) would replace required fields such as data or body with the placeholder string. Mentioning that in the constructor Javadoc may help.

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.

2 participants