Skip to content

fix(json): distinguish root and declared codec factory lookups - #4186

Open
mohitjoshi-hey wants to merge 5 commits into
apache:mainfrom
mohitjoshi-hey:fix/json-root-codec-factory-lookup
Open

mohitjoshi-hey wants to merge 5 commits into
apache:mainfrom
mohitjoshi-hey:fix/json-root-codec-factory-lookup

Conversation

@mohitjoshi-hey

@mohitjoshi-hey mohitjoshi-hey commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Why?

Issue #4169: a registered JSON codec factory could not reliably distinguish a root lookup from a declared or field lookup. A declined root lookup could be repeated as a declared lookup with runtimeType=false, and the cached fallback could suppress later declared factory resolution.

What does this PR do?

  • Prevents root fallback resolution from querying module codec factories a second time after a declined runtimeType=true lookup.
  • Keeps the declined-root fallback out of the declared-type cache so later declared or field lookups can still receive runtimeType=false.
  • Adds a regression test covering both root and declared lookups.
  • Changes only Java JSON resolver and module-test files.
  • Does not change the wire format, public API, serializer protocol, or unrelated runtimes.

Validation:

  • mvn -T1 -pl fory-json -Dtest=JsonModuleTest#factoryDoesNotRequeryDeclinedRoot test — passed (1 passed, 0 failed, 0 errors).
  • The test verifies the factory receives runtimeType=true for the root lookup and runtimeType=false for the later declared lookup.
  • A broader JsonModuleTest run was attempted, but two existing tests encountered NoClassDefFoundError: org/apache/fory/json/JsonTestSupport during Java 9 test-classpath setup. The issue-specific regression test passes independently.

Result:

Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
image

Related issues

Fixes #4169

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes
  • If yes, I included a completed AI Contribution Checklist and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and final clean review result from both required reviewers.

AI Usage Disclosure

  • substantial_ai_assistance: yes
  • scope: issue analysis, Java code drafting, regression-test design, validation, and PR description drafting
  • affected_files_or_subsystems: java/fory-json type resolution, shared codec registry, and JsonModuleTest
  • ai_review: Completed line-by-line self-review and two fresh review loops. The final Fory-guided reviewer and independent general reviewer reported no actionable findings after the cache-ordering correction.
  • ai_review_artifacts: Final review results were recorded for commit 9cd292fd4; the final Fory-guided review reported no actionable findings after re-checking the outer resolution transaction, and the final independent review reported no actionable findings.
  • human_verification: The contributor reviewed the changed resolver and test code. The focused Maven test passed with 1 test, 0 failures, and 0 errors. The broader class run had the unrelated Java 9 test-classpath failure described above.
  • performance_verification: Not applicable; this is a cold type-resolution correctness fix with no wire-format or serialization-hot-path change.
  • provenance_license_confirmation: Apache-2.0-compatible provenance confirmed; no incompatible third-party code introduced.

Completed AI Review Checklist

  • Substantial AI assistance was used in this PR: yes
  • If yes, I included the standardized AI Usage Disclosure above.
  • If yes, I can explain and defend all important changes without AI help.
  • If yes, I reviewed AI-assisted code changes line by line.
  • If yes, I completed line-by-line self-review first and fixed issues before requesting AI review.
  • If yes, I ran two fresh AI review agents on the current PR diff or current HEAD after the latest code changes: one Fory-guided reviewer and one independent general reviewer.
  • If yes, I addressed all actionable AI review comments and repeated the review loop until both AI reviewers reported no further actionable comments.
  • If yes, I recorded the final clean-review results from both reviewers above.
  • If yes, I ran a focused test command and recorded its result above.
  • If yes, I added a regression test for the reported issue.
  • If yes, I validated protocol and performance impacts; no binary protocol change was introduced.
  • If yes, I verified licensing and provenance compliance.

Does this PR introduce any user-facing change?

This PR does not introduce any user-facing API, binary protocol, or cross-runtime behavior change.

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

Not applicable. This is a resolver correctness fix and does not change serialization wire encoding or a serialization hot path.

mohitjoshi-hey and others added 2 commits October 16, 2026 15:27
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mohitjoshi-hey
mohitjoshi-hey marked this pull request as ready for review October 16, 2026 10:23
mohitjoshi-hey and others added 2 commits October 16, 2026 19:08
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mohitjoshi-hey
mohitjoshi-hey marked this pull request as draft October 16, 2026 13:51
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mohitjoshi-hey
mohitjoshi-hey marked this pull request as ready for review October 16, 2026 14:15

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.

Registered factory cannot tell a field from the root of the call

1 participant