fix: exclude JVM-singleton empty collections from raw $ref path tracking - #7796
fix: exclude JVM-singleton empty collections from raw $ref path tracking#77961919chichi wants to merge 2 commits into
Conversation
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
0285424 to
6d22ca8
Compare
| * @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())) { |
There was a problem hiding this comment.
[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())) { |
There was a problem hiding this comment.
[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)
Summary
Fixes #7795.
Collections.emptySet()/emptyList()/emptyMap()are JVM-wide singletons.ObjectWriterProvider.isNotReferenceDetect()is meant to exclude these (and other immutable value types) from$refreference-detection, since tracking a JVM singleton by identity is meaningless. The guardedJSONWriter.setPath()/popPath()overloads already check this viaisRefDetect(Object).However, ASM-generated bean field writers (
ObjectWriterCreatorASM) call the rawsetPath0(FieldWriter, Object)overload directly for non-Object-declared fields, bypassing that exclusion entirely on the push side. MeanwhilepopPath0(Object)only special-casedCollections.EMPTY_LIST/EMPTY_SETby identity on the pop side (notEMPTY_MAP, and not any of the other excluded types reachable through this path). This push/pop asymmetry leftJSONWriter.pathone 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):Deserializing the bogus
$refsilently resolved tonullinstead of throwing or resolving to the real empty collection.Fix
Centralize the exclusion check in the raw choke-point methods on
JSONWriterinstead of patching every individual ASM codegen call site (which is scattered across ~5 methods inObjectWriterCreatorASM.javaand is how this asymmetry was introduced in the first place):setPath0(FieldWriter, Object): early-return whenObjectWriterProvider.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 theEMPTY_LIST/EMPTY_SET-only identity check with the same class-basedisNotReferenceDetectcheck, so push and pop are symmetric again (this also correctly extends the exclusion toEMPTY_MAP).Test plan
core/src/test/java/com/alibaba/fastjson2/issues_7000/Issue7795.java:testSharedEmptySetAcrossMapEntries/testSharedEmptyMapAcrossMapEntries: reproduce the reported scenario (Map<Integer, Bean>with a shared emptySet/Mapfield); verified these fail without thesetPath0(FieldWriter, Object)fix and pass with it.testSetPath0IntOverloadExcludesSingletons: direct contract test for thesetPath0(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.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