Skip to content

feat(java): reject repeated JSON properties on read - #4166

Open
pavel-ptashyts wants to merge 1 commit into
apache:mainfrom
pavel-ptashyts:feat/json-duplicate-properties
Open

pavel-ptashyts wants to merge 1 commit into
apache:mainfrom
pavel-ptashyts:feat/json-duplicate-properties

Conversation

@pavel-ptashyts

@pavel-ptashyts pavel-ptashyts commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Why?

When a JSON object repeats one of a bean's known properties, fory-json keeps the last value and there is no way to reject the document instead. Parsers disagree on duplicate names (first wins, last wins, or fail), so a service that validates a document from a third party and passes it on can read a different value than the next hop. Jackson only offers the parser-wide STRICT_DUPLICATE_DETECTION, which applies to every object and map and costs every read. Details and the motivating case (rejecting repeated top-level fields of inbound OpenRTB requests) are in #4165.

What does this PR do?

  • API. ForyJsonBuilder.failOnDuplicateProperties(boolean), default false (unchanged behaviour: the last occurrence wins), and a class annotation @JsonDuplicateProperties(fail = true | false) that overrides the builder for one class. Like @JsonInclude, the annotation is not inherited and an exact-target Mixin can supply it; it is registered in the runtime and annotation-processor Mixin lists. It applies where the class is read as a JSON object.
  • Semantics. A known property (read-enabled field, setter, creator parameter, or a name flattened in by @JsonUnwrapped) that appears twice in one object fails the read with ForyJsonException: Duplicate JSON property <name> on <class>. An explicit null counts as an occurrence, also under onNullRead(SKIP). Unknown and ignored names and Any entries are not tracked. Nested objects follow their own class setting.
  • Interpreter (ObjectCodec). Strict ordinary beans use their own per-representation loop (read*StrictFixedObject): one extra check inside the lenient bean loop measured about +8% on interpreted reads, so the lenient loop is left exactly as it was. Creator, Any and unwrapped loops check a bit set that is allocated only for strict types; their lenient timings stayed within noise.
  • Generated readers (JsonReaderCodegen). The decision is made at generation time, so lenient types generate exactly the same source as before. Strict bean readers keep long words seen0..seenK: the fast path (which never repeats a member) only sets bits, group helpers receive the words as parameters and return their own group-relative bits as a long (-1 when the object is finished), and slow methods take the words as parameters and check every resolved known index. Ordered creator prefixes have no look-ahead, so the creator slow method seeds its bits from the prefix length. Unwrapped and Any-creator readers keep the words as locals of their single loop. Names for runtime-index failures come from a cold generated switch.
  • Generated class sharing. The effective setting is part of the generated codec key for reader roles, so strict and lenient instances in one JVM never share a reader class.
  • Docs. docs/json/object-mapping.md (builder option) and docs/json/annotations.md (annotation).
  • Tests. New JsonDuplicatePropertiesTest (both codegen modes, Latin1/UTF-16/UTF-8 String/bytes/stream inputs): beans, explicit nulls, class override in both directions, nested objects, ordered and unordered creators, Any beans and Any creators, unwrapped with creators and prefixes, unwrapped with Any, a subtype with Any, a generic bean, a Mixin, a 70-property bean whose group straddles the 64-bit word boundary, a slow fallback from inside a group helper, long (non-direct) names, and two instances with different settings in one JVM. JsonGeneratedCodecTest.duplicateTrackingSource checks the generated Latin1, UTF-16 and UTF-8 reader sources: no tracking for a lenient class, tracking for a strict one.

Related issues

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described in AI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.

Completed checklist from AI_POLICY.md section 9:

  • Substantial AI assistance was used in this PR: yes
  • I included the standardized AI Usage Disclosure block below.
  • I can explain and defend all important changes without AI help.
  • I reviewed AI-assisted code changes line by line before submission.
  • I completed line-by-line self-review first and fixed issues before requesting AI review.
  • I ran two fresh AI review agents on the current PR diff or current HEAD after the latest code changes: one Fory-guided reviewer using AGENTS.md and .agents/ci-and-pr.md, and one independent general reviewer in a separate clean-context review session that was not pointed to .agents/ci-and-pr.md or any copied Fory-specific review checklist.
  • I addressed all AI review comments and repeated the review loop until both ai reviewers reported no further actionable comments.
  • I attached equivalent persisted links of the final clean AI review results from both fresh reviewers on the current PR head in this PR body.
  • I ran adequate human verification and recorded evidence (checks run locally, pass/fail summary, and confirmation I reviewed results).
  • I added/updated tests and specs where required.
  • I validated protocol/performance impacts with evidence when applicable.
  • I verified licensing and provenance compliance.
AI Usage Disclosure
- substantial_ai_assistance: yes
- scope: design drafting, code drafting, tests, benchmark harness, docs, PR text
- affected_files_or_subsystems: java/fory-json (builder/config, ObjectCodec interpreter, JsonReaderCodegen, generated codec key, Mixin registration), fory-annotation-processor Mixin list, docs/json
- ai_review: line-by-line self-review of the AI-drafted change is pending by the contributor (see unchecked items above). Two-reviewer loop on fresh sessions for every round, three rounds. Fixed from review: UTF-16 test input no longer starts with an unknown member, so the ordered fast path is covered there too; the error names the owning class; unwrapped names are resolved only on failure; added tests for a slow fallback from inside a group helper, unwrapped creators and prefixes, unwrapped with Any, a subtype with Any, creator nulls under SKIP, a generic bean, an unordered (17-member) creator, non-direct names, and a generated-source check for lenient and strict readers; the annotation scope is documented. Found by the author's own measurements between rounds: the first design (no fast-path look-ahead for strict readers) cost +63% on documents missing an early member and was replaced by carrying the bits through the fast path; one check inside the lenient interpreted bean loop cost +8% and was moved to a separate strict loop. Kept by decision: deferred creator members (language-module object models only) share the creator loop check without a Java fixture; a Native Image build-time/runtime setting mismatch falls back to the interpreter like failOnMissingRequiredProperties. Final round on the current head: both reviewers report no further actionable comments.
- ai_review_artifacts: Fory-guided reviewer: https://github.com/apache/fory/pull/4166#issuecomment-6111855733 ; independent general reviewer: https://github.com/apache/fory/pull/4166#issuecomment-6111855963 (both on head commit 6fe221e0, final round, no further actionable comments)
- human_verification: see "Verification" below
- performance_verification: see "Benchmark" below
- provenance_license_confirmation: all code written for this PR against the Apache Fory sources; no third-party code introduced

Verification

JDK 25.0.2, Windows 11 x86_64, from java/:

  • mvn -pl fory-json -am install -DskipTests: success
  • mvn -pl fory-json,fory-annotation-processor test: fory-json 1324 tests, fory-annotation-processor 102 tests, 0 failures, 0 errors, 0 skipped
  • mvn -pl fory-json,fory-annotation-processor spotless:check checkstyle:check: clean
  • Mutation checks, each made JsonDuplicatePropertiesTest fail as intended: no interpreter marks; no generated marks; no fast-path marks; no second-word carry of group bits; group bits not passed from a helper to the slow method; setting removed from the generated codec key.
  • Generated-source identity: with FORY_CODE_DIR, the 30 generated sources for a bean, an Any bean, a creator, an unwrapped model and a 70-property bean (all writer and reader roles) are byte-identical between apache/main (b602392) and this branch.

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change? ForyJsonBuilder.failOnDuplicateProperties(boolean) and @JsonDuplicateProperties; the default behaviour is unchanged.
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

Default (lenient) path: generated readers are byte-identical to apache/main (see above). Interpreted lenient reads (withCodegen(false), 24 int fields, UTF-8 bytes, ad-hoc timing loop, median of 6 alternating JVMs per jar): declaration-ordered document 343 ns on main vs 340 ns on this branch; document with the first member last 384 vs 365 ns (that case is bimodal across JVMs on both jars). Interpreted 10-member creator and Any bean: within noise.

Strict readers, generated code (same 24-field bean, strict vs lenient class in one JVM): declaration-ordered document within noise; document with the first member moved to the end about +5..10%.

Add ForyJsonBuilder.failOnDuplicateProperties(boolean) and the class
annotation @JsonDuplicateProperties(fail = ...). When enabled for a type,
a known property (field, setter, creator parameter or unwrapped name)
that appears twice in one JSON object fails the read with a
ForyJsonException naming the property. The default is unchanged: the
last occurrence wins, and lenient types generate the same reader code as
before.

Strict generated readers carry one bit per member through the fast path,
its group helpers and the slow method; the interpreter keeps a per-object
bit set. The effective setting is part of the generated codec key.

Closes apache#4165

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pavel-ptashyts

Copy link
Copy Markdown
Contributor Author

AI review artifact: Fory-guided reviewer (final round)

  • Reviewer: Codex CLI, fresh codex exec session, read-only sandbox, prompted to follow AGENTS.md, .agents/ci-and-pr.md and .agents/languages/java.md
  • Reviewed: full diff of feat/json-duplicate-properties against apache/main b602392, head commit 6fe221e
  • Round: 3 of 3 (rounds 1 and 2 were also clean from this reviewer; the code changed between rounds because of the independent reviewer's and the author's findings)

Session header:

OpenAI Codex v0.159.3
workdir: /mnt/d/git/fory-wt-dup
model: gpt-6-astra
sandbox: read-only
reasoning effort: medium

Final output:

No actionable findings in the supplied complete diff of `feat/json-duplicate-properties` against `apache/main b6023929`.

The interpreter and generated-reader paths consistently track known properties, count skipped nulls, preserve marks across fast/slow transitions, and apply per-class settings.

Verification limitations: the reported test, mutation, and performance results were not independently verified. Exact commands/logs, remote CI results, and Native Image/Android execution evidence were not supplied. No files were changed and no builds, tests, benchmarks, or linters were run.

No further actionable comments.

@pavel-ptashyts

Copy link
Copy Markdown
Contributor Author

AI review artifact: independent general reviewer (final round)

  • Reviewer: Claude Code claude -p in a fresh clean-context session, model claude-fable-5-1 (from the session's modelUsage), tools limited to Read/Grep/Glob, told to act under the independent-review carve-out and not pointed at .agents/ci-and-pr.md or any Fory review checklist
  • Reviewed: full diff of feat/json-duplicate-properties against apache/main b602392, head commit 6fe221e
  • Round: 3 of 3 (earlier rounds: test gaps and two nits, all addressed or answered in the PR description)

Final output:

Review complete. I read the full diff and the surrounding interpreter and codegen code, traced every reader path the feature touches, and found no correctness bugs.

What I verified

  • Interpreter, fixed beans. The strict loops in ObjectCodec.java:623 mirror the lenient loop exactly and mark by readIndex(), which ObjectCodecBuilder.java:469 sets to the array position. The lenient loop is untouched, so the default path pays one boolean branch per object.
  • Interpreter, Any, creator, and unwrapped. Each loop allocates the bit array only when strict and guards the mark with a null check. Creator indexes come from JsonCreatorInfo.index, which stores the ordered field index, so fields()[index].name() is the right name. Unwrapped route names are the transformed (prefixed) copies from JsonUnwrappedInfo.java:772, so the error names n_x rather than x.
  • Codegen fast path. All four direct read sites in the main method and group helpers go through fastReadMember, which only sets bits. Every slow call and group call appends the seen words through withFastSeen, and a group helper's merge in withGroupSeen handles the two-word spill correctly. Groups hold far fewer than 64 members because HOT_INLINE_LIMIT is 325 bytes at 80 or more bytes per member.
  • Codegen slow methods. Both slow variants mark the index before the field switch and guard negatives, which covers UNKNOWN and SKIP. The first-field variant marks firstFieldIndex on the first loop iteration.
  • Codegen creators. The ordered prefix passes readCount equal to the count of fields read before the first miss, and readPrefixBits seeds the words correctly, including word boundaries. The Any-creator and unordered loops mark per case with a static name.
  • Lenient output unchanged. With the flag off, declareFastSeen, seenParameters, withSeenParameters, and withFastSeen return the original inputs, fastReadMember returns the plain read, and returnTrue and returnFalse keep their boolean form. The source test in JsonGeneratedCodecTest asserts this for all three reader roles.
  • Caching and config. The flag is part of GeneratedCodecKey, so hosted and JIT classes cannot be shared across settings. The annotation is registered for mixins in both the runtime and the annotation-processor allow lists. Kotlin and Scala builders return the Java builder, so no wrapper needs updating.
  • Tests. The new test class runs under both codegen modes and four input representations, and covers beans, split groups across the 64-bit boundary, explicit nulls, creators, Any, unwrapped with creators and prefixes, subtypes, generics, mixins, and the per-class override in both directions.

Low-severity note, not actionable. A strict class with zero known members and no unwrapped routes generates an empty switch (index) {} in an unused rejectDuplicate method. That is valid Java and harmless.

No further actionable comments.

This branch has not been deployed

No deployments
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.

[Java][JSON] No way to reject a repeated JSON property on read

1 participant