Skip to content

fix: exclude JVM-singleton empty collections from raw $ref path tracking - #7796

Open
1919chichi wants to merge 2 commits into
alibaba:mainfrom
1919chichi:fix/issue-7795-shared-empty-collection-ref
Open

fix: exclude JVM-singleton empty collections from raw $ref path tracking#7796
1919chichi wants to merge 2 commits into
alibaba:mainfrom
1919chichi:fix/issue-7795-shared-empty-collection-ref

Conversation

@1919chichi

Copy link
Copy Markdown
Contributor

Summary

Fixes #7795.

Collections.emptySet() / emptyList() / emptyMap() are JVM-wide singletons. ObjectWriterProvider.isNotReferenceDetect() is meant to exclude these (and other immutable value types) from $ref reference-detection, since tracking a JVM singleton by identity is meaningless. The guarded JSONWriter.setPath() / popPath() overloads already check this via isRefDetect(Object).

However, ASM-generated bean field writers (ObjectWriterCreatorASM) call the raw setPath0(FieldWriter, Object) overload directly for non-Object-declared fields, bypassing that exclusion entirely on the push side. Meanwhile popPath0(Object) only special-cased Collections.EMPTY_LIST/EMPTY_SET by identity on the pop side (not EMPTY_MAP, and not any of the other excluded types reachable through this path). This push/pop asymmetry left JSONWriter.path one level too deep after writing a shared empty collection field, corrupting every path computed afterward.

Repro (works against plain com.alibaba.fastjson2.JSON, not just the fastjson1-compatible module):

Set<Integer> sharedEmptySet = Collections.emptySet();
Map<Integer, Model> map = new LinkedHashMap<>();
map.put(1001, new Model(sharedEmptySet));
map.put(1002, new Model(sharedEmptySet));

JSON.toJSONString(map, JSONWriter.Feature.ReferenceDetection);
// before: {1001:{"ids":[]},1002:{"ids":{"$ref":"$.ids"}}}  <- bogus ref, path missing "1001"
// after:  {1001:{"ids":[]},1002:{"ids":[]}}

Deserializing the bogus $ref silently resolved to null instead of throwing or resolving to the real empty collection.

Fix

Centralize the exclusion check in the raw choke-point methods on JSONWriter instead of patching every individual ASM codegen call site (which is scattered across ~5 methods in ObjectWriterCreatorASM.java and is how this asymmetry was introduced in the first place):

  • setPath0(FieldWriter, Object): early-return when ObjectWriterProvider.isNotReferenceDetect(object.getClass()).
  • setPath0(int, Object): same guard, for symmetry with the sibling raw overload (its one current caller, ObjectWriterImplList.writeJSONB, already pre-filters via a guarded check, so this is defense-in-depth on the public raw API rather than a currently-reachable fix).
  • popPath0(Object): replaced the EMPTY_LIST/EMPTY_SET-only identity check with the same class-based isNotReferenceDetect check, so push and pop are symmetric again (this also correctly extends the exclusion to EMPTY_MAP).

Test plan

  • Added core/src/test/java/com/alibaba/fastjson2/issues_7000/Issue7795.java:
    • testSharedEmptySetAcrossMapEntries / testSharedEmptyMapAcrossMapEntries: reproduce the reported scenario (Map<Integer, Bean> with a shared empty Set/Map field); verified these fail without the setPath0(FieldWriter, Object) fix and pass with it.
    • testSetPath0IntOverloadExcludesSingletons: direct contract test for the setPath0(int, Object) overload.
  • mvn -pl core -am validate (checkstyle) clean.
  • mvn -pl core -am test: full core suite green (7977 tests, 0 failures/errors), both incrementally and after a clean rebuild.
  • Independently reviewed by two separate agents (one Claude-based, one Codex-based); both confirmed the root-cause diagnosis and fix are correct with no NPE risk or regression, and both independently caught a test-quality gap in an earlier draft (a List-based test that didn't actually exercise the fixed code path), which was reworked before this PR.
中文说明

概述

修复 #7795

Collections.emptySet() / emptyList() / emptyMap() 是 JVM 全局单例,ObjectWriterProvider.isNotReferenceDetect() 的设计意图就是把它们(以及其他不可变值类型)从 $ref 引用检测中排除,因为按对象身份去追踪一个全局单例是没有意义的。有保护的 JSONWriter.setPath()/popPath() 已经通过 isRefDetect(Object) 做了这个检查。

但 ASM 生成的 Bean 字段写入代码(ObjectWriterCreatorASM)对于非 Object 声明类型的字段,会直接调用裸的 setPath0(FieldWriter, Object),在压栈侧完全绕过了这层排除;而 popPath0(Object) 在出栈侧只是按对象身份特判了 Collections.EMPTY_LIST/EMPTY_SET 两个单例(不包括 EMPTY_MAP,也不包括这条路径上其他本该排除的类型)。这种压栈会记、弹栈不弹的不对称,导致写完一个共享的空集合字段后,JSONWriter.path 多陷了一层且不再恢复,污染了后续所有路径计算。

修复方案

没有去逐个修补 ObjectWriterCreatorASM.java 里分散在约 5 处的 ASM 字节码生成调用点(这种分散正是这次不对称产生的原因),而是把排除判断集中收口到 JSONWriter 上的裸方法本身:

  • setPath0(FieldWriter, Object):命中 ObjectWriterProvider.isNotReferenceDetect(object.getClass()) 时提前返回。
  • setPath0(int, Object):同样加上这层判断,保持和另一个裸重载的契约一致(它目前唯一的调用方 ObjectWriterImplList.writeJSONB 在调用前已经做了有保护的判断,所以这里属于对公开裸 API 的防御性加固,不对应当前可触发的路径)。
  • popPath0(Object):把原来只认 EMPTY_LIST/EMPTY_SET 两个单例的身份判断,换成同样的按 class 排除判断,让压栈和弹栈重新对称(顺带正确覆盖了 EMPTY_MAP)。

测试

新增 core/src/test/java/com/alibaba/fastjson2/issues_7000/Issue7795.java,并做了完整的正反验证:去掉修复代码时对应测试会失败、加回后通过;mvn validate(checkstyle)干净;core 模块全量测试 7977 个用例全部通过。修复方案本身也经过两个独立 agent(一个基于 Claude、一个基于 Codex)分别审查,结论一致:修复正确、无 NPE 风险、无回归;两边还各自独立发现了草稿阶段一个测试没有真正覆盖到修复点的问题,已在提交前修正。

🤖 Generated with Claude Code

https://claude.ai/code/session_01YFzJPj75rwUU5YetDZLSyd

Collections.emptySet()/emptyList()/emptyMap() are JVM-wide singletons,
so ObjectWriterProvider.isNotReferenceDetect() is meant to exclude them
from $ref reference detection. The guarded JSONWriter.setPath/popPath
already check that. But the raw setPath0/popPath0 overloads - called
directly by ASM-generated bean field writers for non-Object-declared
fields - did not, while popPath0 separately special-cased only the two
singletons on the pop side. That push/no-pop asymmetry corrupted
JSONWriter.path after writing a shared empty collection, producing a
bogus $ref (missing outer path segments) for the next occurrence, which
then deserializes to null. Fixes alibaba#7795.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFzJPj75rwUU5YetDZLSyd
@CLAassistant

CLAassistant commented Aug 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@1919chichi
1919chichi force-pushed the fix/issue-7795-shared-empty-collection-ref branch from 0285424 to 6d22ca8 Compare August 21, 2026 08:38
* @return the previous path as a string, or null if no previous path exists
*/
public final String setPath0(FieldWriter fieldWriter, Object object) {
if (ObjectWriterProvider.isNotReferenceDetect(object.getClass())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Critical] These raw path methods now dereference nullable values before applying the class exclusion. After a caller initializes JSONWriter.path, setPath0(FieldWriter, null), setPath0(int, null), or popPath0(null) reaches object.getClass() and throws, while the previous implementations accepted null. A probe against this PR failed at this line, and guarding the classifier with object != null made it pass. Please preserve the previous null behavior in all three changed methods.

PR: JSONWriterNullProbeTest.rawPathMethodsAcceptNull -> NullPointerException
 at JSONWriter.setPath0(JSONWriter.java:302)
With null guard: Tests run: 1, Failures: 0, Errors: 0; BUILD SUCCESS

Add a regression test that calls both setPath0 overloads and popPath0(null); removing the null guard must make that test fail.

中文说明

这些裸 path 方法现在会在应用 class 排除判断之前解引用可能为 null 的值。调用方初始化 JSONWriter.path 后,setPath0(FieldWriter, null)setPath0(int, null)popPath0(null) 都会执行 object.getClass() 并抛出异常,而旧实现可以接受 null。针对当前 PR 的探针在此行失败,增加 object != null 保护后通过。请在三个改动的方法中都保留原有的 null 行为。并增加覆盖两个 setPath0 重载和 popPath0(null) 的回归测试;移除 null 保护时该测试必须失败。

— gpt-5.6-sol via Qwen Code /review (v0.22.0)

* @return the previous path as a string, or null if no previous path exists
*/
public final String setPath0(FieldWriter fieldWriter, Object object) {
if (ObjectWriterProvider.isNotReferenceDetect(object.getClass())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Suggestion] The public Javadocs for both setPath0 overloads and popPath0 still say they operate “without reference detection,” but these methods now skip every class rejected by ObjectWriterProvider.isNotReferenceDetect. That wording can lead callers to rely on a bypass that no longer exists. Please clarify that the raw methods bypass the feature-level check while still enforcing the unsupported-class exclusion.

中文说明

两个 setPath0 重载和 popPath0 的公开 Javadoc 仍然写着“without reference detection”,但这些方法现在会跳过 ObjectWriterProvider.isNotReferenceDetect 排除的所有 class。这个表述可能让调用方误以为仍能完全绕过引用检测条件。建议明确说明:裸方法只绕过 feature 层检查,但仍会执行不支持引用检测类型的排除。

— gpt-5.6-sol via Qwen Code /review (v0.22.0)

@1919chichi
1919chichi requested a review from wenshao August 25, 2026 13:45
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.

[BUG] fastjson1-compatible 序列化 Map<Integer, Bean> 中的共享 Collection 时生成缺少 Map key 的 $ref

3 participants