Skip to content

fix: JSONB shared reference in non-List collection elements unresolvable on deserialization#7641

Open
yibiner wants to merge 3 commits into
alibaba:mainfrom
yibiner:main
Open

fix: JSONB shared reference in non-List collection elements unresolvable on deserialization#7641
yibiner wants to merge 3 commits into
alibaba:mainfrom
yibiner:main

Conversation

@yibiner

@yibiner yibiner commented May 12, 2026

Copy link
Copy Markdown

What this PR does / why we need it?

反序列化时,非列表集合元素中的 JSONB 共享引用无法解析。

Summary of your change

  • ObjectWriterImplCollection.java — 对非 List 集合(如 HashSet)的元素禁用 ReferenceDetection,改为内联写入,避免生成 $.foo 这类以 Set 为根的路径在反序列化时无法解析。
  • SharedReferenceInSetTest.java — 新增回归测试。

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

@CLAassistant

CLAassistant commented May 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@wenshao wenshao left a comment

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.

Review 总结

[Critical] write() 方法(JSON 文本路径)存在相同 bug 但未修复

write() 方法(第 153-206 行)对非 List 集合也没有抑制引用检测。使用 JSON.toJSONString(set, ReferenceDetection) 序列化含共享内部引用的 Set 时,子 writer 仍会生成不可解析的 $ref 路径。已实测确认:

序列化输出: [{"sn":"sn-1","codes":["c3","c1","c2"]},{"sn":"sn-3","codes":{"$ref":"$.codes"}},{"sn":"sn-2","codes":{"$ref":"$.codes"}}]
反序列化后: sn-2 和 sn-3 的 codes 字段为 null

建议在 write() 方法中也应用同样的 suppressElementRefDetect + try/finally 模式。

[Critical] 单元素非 List 集合的循环引用可能导致 StackOverflowError

清除 context 级 ReferenceDetection 后,isRefDetect() 对所有子 writer 返回 false。对于包含循环引用的单元素非 List 集合(如 HashSet 中唯一 Bean 持有指回该 Set 的字段),递归写入时无循环检测,会导致无限递归。

[Suggestion] 测试覆盖缺口

  1. TreeSet 测试(关键):TreeSet 是唯一使 refDetect 保持 true 的非 List 集合类型(SortedSet 被排除在 size > 1 条件外),是验证新代码路径的核心场景
  2. List 回归测试:确保 ArrayList + ReferenceDetection 的行为不受影响
  3. 边界测试:空集合、单元素集合
  4. 不开启 ReferenceDetection 的测试:确认无副作用
  5. JSON 文本格式测试:使用 JSON.toJSONString() 而非 JSONB.toBytes()

— qwen3.7-max via Qwen Code /review

Comment thread core/src/main/java/com/alibaba/fastjson2/writer/ObjectWriterImplCollection.java Outdated
@yibiner

yibiner commented Jul 1, 2026

Copy link
Copy Markdown
Author

已修复Review 问题。 d425e5b

问题: 非 List 集合(如 HashSet)开启 ReferenceDetection 时,元素内共享引用会输出不可解析的 $ref(如 "$.codes"),反序列化后字段为 null;write() 文本路径也有同样问题。

修复: 在 ObjectWriterImplCollection 的 writeJSONB() / write() 中,写每个元素前用 JSONWriter.saveReferences() / restoreReferences() 回滚 sibling 注册的 refs,强制内联共享对象;不关闭 ReferenceDetection,单元素循环引用仍可正常 $ref 终止。

文件变更:
JSONWriter:新增 saveReferences() / restoreReferences()
ObjectWriterImplCollection:两条序列化路径同步修复
测试:SharedReferenceInSetTest(18 用例)+ JSONWriterTest(2 个 API 单测)

@wenshao wenshao left a comment

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.

Review 总结(针对最新提交 b57c8816 的复审)

此前 @wenshao 提出的两个 Critical 已在当前代码中解决:

  • write() 文本路径的共享引用 bug:已修复。write() 现与 writeJSONB 一致,对非 List 集合在每个元素前 restoreReferences,使兄弟元素间的共享引用内联,不再输出不可解析的 $reftestTextFormatHashSetWithSharedInnerSet!str.contains("$ref") 守护)。
  • 缺少 TreeSet / 单元素循环引用测试:已补充 testTreeSetWithSharedInnerSetBeantestSingleElementSelfCycleDoesNotOverflow 等。该实现通过 save/restore 引用表(而非清除 context 级 ReferenceDetection)来内联兄弟引用,因此 isRefDetect() 对子 writer 仍为 true,元素内部循环引用仍被检测,不会 StackOverflow。

新发现 2 个 Suggestion(详见行内评论,非阻塞):

  1. testTreeSetWithSharedInnerSetBeanTreeSet.containscompareTo(仅比较 sn)而无法验证 codes 字段,对 SortedSet 路径的回归无保护。
  2. 修复只重置「元素之间」的引用,未覆盖「单个元素内部」多次引用同一对象的情况(仍可能输出不可解析的 $ref → 反序列化为 null)。

构建与测试已通过:SharedReferenceInSetTest 18/18、JSONWriterTest 121/121。

— Qwen Code via Qwen Code /review

Comment on lines +122 to +125
public void testTreeSetWithSharedInnerSetBean() {
Type type = new TypeReference<TreeSet<Bean>>() {
}.getType();
assertBeanRowsPreserved(buildBeanRows(new TreeSet<>()), type);

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] testTreeSetWithSharedInnerSetBean 实际上无法验证 codes 字段是否被保留。assertBeanRowsPreserved 仅依赖 assertEquals(size, 3)back.contains(bean);而 TreeSet.containsBean.compareToreturn sn.compareTo(o.sn),只比较 sn),不比较 codes,且三个 sn 互不相同,故 size 恒为 3。 — Failure scenario: 若 SortedSet 路径回归(例如 TreeSet 元素重新输出不可解析的 $ref),codes 反序列化为 null,但该测试仍会通过(size=3、contains 按 sn 匹配),无法捕获回归;而这正是维护者专门要求覆盖的 SortedSet 路径(HashSet/LinkedHashSet 用例走 equals/hashCode 能捕获 codes 为 null,TreeSet 不能)。

建议在 assertBeanRowsPreserved 中(或针对 TreeSet 用例)直接断言 codes

for (Bean bean : back) {
    assertNotNull(bean.codes, "codes should not be null after round-trip, sn=" + bean.sn);
    assertEquals(3, bean.codes.size());
}

— Qwen Code via Qwen Code /review

Comment on lines 99 to +102
for (Iterator it = collection.iterator(); it.hasNext(); ++i) {
if (inlineSharedElementRefs) {
jsonWriter.restoreReferences(refsSnapshot);
}

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] 该修复只在「元素之间」重置引用(每次循环开始调用 restoreReferences),未处理「单个元素内部」多次引用同一对象的情况。对非 List 集合(如 size>1 的 HashSet,refDetect=false、元素路径未入栈),元素内部被多次引用的同一对象仍会以 Set 为根注册路径(如 $.k1),第二次出现时输出 {"$ref":"$.k1"}。 — Failure scenario: 开启 ReferenceDetection 序列化 HashSet<Map<String,Object>>(size>1),某个 map 把同一实例放在两个 key 下(map.put("k1", shared); map.put("k2", shared)),或某 Bean 有两个字段指向同一实例:第一次注册为 $.k1,第二次输出 {"$ref":"$.k1"};反序列化时根为数组、$.k1 无法解析,第二个字段变为 null(静默数据丢失)。新增测试每个元素只引用共享对象一次(仅覆盖「跨元素」场景),未覆盖此场景。

建议:在 inlineSharedElementRefs 生效时对每个元素子树也抑制引用检测(即 PR 描述中的「对元素禁用 ReferenceDetection」),或在写每个元素前压入一个稳定的元素路径,使元素内部的 $ref 可解析。

— Qwen Code via Qwen Code /review

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.

3 participants