You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fory.apache.org by "PiotrDuz (via GitHub)" <gi...@apache.org> on 2026/07/30 12:32:36 UTC
[PR] fix(java): reproduce silent Set child-loss for package-private nodes [fory]
PiotrDuz opened a new pull request, #3902:
URL: https://github.com/apache/fory/pull/3902
cyclic graphs
Wide fan-out (one parent, many children) cyclic graph of package-private nodes loses entries from a Set<PackagePrivateType> field on deserialize - no exception.
## Why?
fory had a regression bug of cyclic graph losing edges. It seems to be fixed on master, but want to pin it so no further regressions are possible.
## What does this PR do?
This PR adds a test case that reproduces the regression
## Related issues
## AI Contribution Checklist
The test has been generated by AI, but the idea and guidance is human - the problem and its reporduction in version 1.1.0 is tested by hand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(java): reproduce silent Set child-loss for package-private nodes [fory]
Posted by "wenshao (via GitHub)" <gi...@apache.org>.
wenshao commented on code in PR #3902:
URL: https://github.com/apache/fory/pull/3902#discussion_r3692714081
##########
java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fory.codegen.pkgprivate;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.fory.Fory;
+import org.apache.fory.ThreadSafeFory;
+import org.testng.annotations.Test;
+
+/**
+ * Regression test: a wide fan-out (one parent, many children) cyclic graph of package-private
+ * nodes silently loses entries from a {@code Set<PackagePrivateType>} field during
+ * deserialization - no exception is thrown, so unlike {@link PackagePrivateMapKeyTest} (which
+ * catches the codegen CompileException for this same field shape) this defect only surfaces as
+ * missing data.
+ *
+ * <p>Reproduced from a production graph with 100+ children under one parent node; the
+ * child->parent edge (an EnumMap-keyed back-reference) survives the round trip intact, but the
+ * parent->child edge (a plain HashSet) loses a handful of entries. Both edges are set together,
+ * atomically, at construction time, so the source object graph itself is never inconsistent -
+ * the asymmetry is introduced purely by fory's serialize/deserialize round trip.
+ */
+public class PackagePrivateCyclicSetChildLossTest {
+
+ private static final int CHILD_COUNT = 200;
+
+ @Test
+ public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() {
+ ThreadSafeFory fury =
+ Fory.builder()
+ .withXlang(false)
+ .requireClassRegistration(false)
+ .withRefTracking(true)
+ .withCompatible(false)
+ .buildThreadSafeFory();
+
+ FanOutContainer container = new FanOutContainer("v1");
+ FanOutNode parent = new FanOutNode(FanOutType.TYPE_A, "parent");
+ container.nodes.computeIfAbsent(FanOutType.TYPE_A, k -> new HashMap<>()).put(parent.id, parent);
+
+ List<String> expectedChildIds = new ArrayList<>();
+ for (int i = 0; i < CHILD_COUNT; i++) {
+ FanOutNode child = new FanOutNode(FanOutType.TYPE_B, "child-" + i);
+ parent.children.add(child);
+ child.parents.computeIfAbsent(parent.type, k -> new LinkedHashSet<>()).add(parent);
+ container.nodes.computeIfAbsent(FanOutType.TYPE_B, k -> new HashMap<>()).put(child.id, child);
+ expectedChildIds.add(child.id);
+ }
+
+ byte[] bytes = fury.serialize(container);
+ FanOutContainer result = (FanOutContainer) fury.deserialize(bytes);
+
+ FanOutNode resultParent = result.nodes.get(FanOutType.TYPE_A).get("parent");
+ assertEquals(resultParent.children.size(), CHILD_COUNT, "parent lost children from its Set field");
+
+ // Every child must still be reachable BOTH ways: down (parent.children) and up
+ // (child.parents), and both directions must point at the exact same object (fory's
+ // refTracking should unify identical (type,id) instances, not duplicate them).
+ List<String> asymmetric = new ArrayList<>();
+ for (String childId : expectedChildIds) {
+ FanOutNode resultChild = result.nodes.get(FanOutType.TYPE_B).get(childId);
+ boolean parentListsChild = resultParent.children.contains(resultChild);
+ boolean childListsParent =
+ resultChild.parents.getOrDefault(FanOutType.TYPE_A, Set.of()).contains(resultParent);
Review Comment:
**[Critical]** `Set.of()` is a Java 9+ API, but `fory-core` compiles with `maven.compiler.source=8` / `maven.compiler.target=8` (no `--release` flag), and CI runs tests on JDK 8 (matrix includes `"8"`). — Failure scenario: the JDK 8 CI leg fails at compile time with `cannot find symbol: method of()`, blocking the entire `fory-core` test module. Every other empty-set usage in fory-core tests uses `Collections.emptySet()`.
```suggestion
resultChild.parents.getOrDefault(FanOutType.TYPE_A, Collections.emptySet()).contains(resultParent);
```
_— qwen3.8-max-preview via Qwen Code /review_
##########
java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fory.codegen.pkgprivate;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.fory.Fory;
+import org.apache.fory.ThreadSafeFory;
+import org.testng.annotations.Test;
+
+/**
+ * Regression test: a wide fan-out (one parent, many children) cyclic graph of package-private
+ * nodes silently loses entries from a {@code Set<PackagePrivateType>} field during
+ * deserialization - no exception is thrown, so unlike {@link PackagePrivateMapKeyTest} (which
+ * catches the codegen CompileException for this same field shape) this defect only surfaces as
+ * missing data.
+ *
+ * <p>Reproduced from a production graph with 100+ children under one parent node; the
+ * child->parent edge (an EnumMap-keyed back-reference) survives the round trip intact, but the
+ * parent->child edge (a plain HashSet) loses a handful of entries. Both edges are set together,
+ * atomically, at construction time, so the source object graph itself is never inconsistent -
+ * the asymmetry is introduced purely by fory's serialize/deserialize round trip.
+ */
+public class PackagePrivateCyclicSetChildLossTest {
+
+ private static final int CHILD_COUNT = 200;
+
+ @Test
+ public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() {
+ ThreadSafeFory fury =
+ Fory.builder()
+ .withXlang(false)
+ .requireClassRegistration(false)
+ .withRefTracking(true)
+ .withCompatible(false)
+ .buildThreadSafeFory();
+
+ FanOutContainer container = new FanOutContainer("v1");
+ FanOutNode parent = new FanOutNode(FanOutType.TYPE_A, "parent");
+ container.nodes.computeIfAbsent(FanOutType.TYPE_A, k -> new HashMap<>()).put(parent.id, parent);
+
+ List<String> expectedChildIds = new ArrayList<>();
+ for (int i = 0; i < CHILD_COUNT; i++) {
+ FanOutNode child = new FanOutNode(FanOutType.TYPE_B, "child-" + i);
+ parent.children.add(child);
+ child.parents.computeIfAbsent(parent.type, k -> new LinkedHashSet<>()).add(parent);
+ container.nodes.computeIfAbsent(FanOutType.TYPE_B, k -> new HashMap<>()).put(child.id, child);
+ expectedChildIds.add(child.id);
+ }
+
+ byte[] bytes = fury.serialize(container);
+ FanOutContainer result = (FanOutContainer) fury.deserialize(bytes);
+
+ FanOutNode resultParent = result.nodes.get(FanOutType.TYPE_A).get("parent");
+ assertEquals(resultParent.children.size(), CHILD_COUNT, "parent lost children from its Set field");
+
+ // Every child must still be reachable BOTH ways: down (parent.children) and up
+ // (child.parents), and both directions must point at the exact same object (fory's
+ // refTracking should unify identical (type,id) instances, not duplicate them).
Review Comment:
**[Suggestion]** Comment claims reference-identity verification ("the exact same object", "not duplicate them") but the assertions use `contains()`, which delegates to value-based `equals()`/`hashCode()` on `FanOutNode` — reference duplication would pass undetected. — Concrete cost: a future maintainer reads this comment and concludes identity is already covered here, skips writing an identity (`==`) test; a refTracking regression that deserializes distinct-but-equal instances ships undetected.
```suggestion
// Every child must still be reachable BOTH ways: down (parent.children) and up
// (child.parents), and both directions must be value-equal (same (type, id)).
// Note: this checks logical equality, not reference identity.
```
_— qwen3.8-max-preview via Qwen Code /review_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(java): reproduce silent Set child-loss for package-private nodes [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang closed pull request #3902: fix(java): reproduce silent Set child-loss for package-private nodes
URL: https://github.com/apache/fory/pull/3902
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] fix(java): reproduce silent Set child-loss for package-private nodes [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #3902:
URL: https://github.com/apache/fory/pull/3902#issuecomment-5151631926
After reviewing this issue, cyclic references should not be used as `Set` elements or `Map` keys. Fory must publish an object reference before all of its fields have been fully deserialized in order to resolve cycles. If that partially materialized object is inserted into a hash-based container, `hashCode()` or `equals()` may observe incomplete state. When the remaining fields are restored, the hash may change and invalidate the container's internal structure.
This happened to work previously because the relevant `List` or collection field was serialized and deserialized last, after the fields used by `hashCode()` and `equals()` had already been restored. That ordering was incidental and is no longer guaranteed after the field-ordering semantics were adjusted.
The recommended workaround is to serialize the cyclic references as a `List`, mark the derived `Set` as `transient`, and materialize the `Set` lazily after deserialization:
```java
private List<Node> parentList = new ArrayList<>();
private transient Set<Node> parents;
public Set<Node> getParents() {
if (parents == null) {
parents = new LinkedHashSet<>(parentList);
}
return parents;
}
```
The same approach can be used for maps by serializing an ordered list of key-value entries and constructing the map after the object graph has been fully deserialized. Lists, arrays, object fields, and map values may still contain cyclic references; the limitation applies specifically to cyclic objects used as set elements or map keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org