Merge pull request #10984 from protocolbuffers/googleberg-patch-neste…

…d-builder-fix Fix stale cached submessages by marking nested builder as clean after clear is called
protocolbuffers · Nov 14, 2022 · 3c3e66a · 3c3e66a
2 parents 4f393bd + 736e9b8
commit 3c3e66a
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 0 deletions.
diff --git a/java/core/src/main/java/com/google/protobuf/SingleFieldBuilderV3.java b/java/core/src/main/java/com/google/protobuf/SingleFieldBuilderV3.java
@@ -199,6 +199,9 @@ public SingleFieldBuilderV3<MType, BType, IType> clear() {
       builder = null;
     }
     onChanged();
+    // After clearing, parent is dirty, but this field builder is now clean and any changes should
+    // trickle up.
+    isClean = true;
     return this;
   }
 

diff --git a/java/core/src/test/java/com/google/protobuf/NestedBuildersTest.java b/java/core/src/test/java/com/google/protobuf/NestedBuildersTest.java
@@ -33,6 +33,7 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import protobuf_unittest.Engine;
+import protobuf_unittest.TimingBelt;
 import protobuf_unittest.Vehicle;
 import protobuf_unittest.Wheel;
 import java.util.ArrayList;
@@ -48,6 +49,27 @@
 @RunWith(JUnit4.class)
 public class NestedBuildersTest {
 
+  @Test
+  public void test3LayerPropagationWithIntermediateClear() {
+    Vehicle.Builder vehicleBuilder = Vehicle.newBuilder();
+    vehicleBuilder.getEngineBuilder().getTimingBeltBuilder();
+
+    // This step detaches the TimingBelt.Builder (though it leaves a SingleFieldBuilder in place)
+    vehicleBuilder.getEngineBuilder().clear();
+
+    // These steps build the middle and top level messages, it used to leave the vestigial
+    // TimingBelt.Builder in a state where further changes didn't propagate anymore
+    Object unused = vehicleBuilder.getEngineBuilder().build();
+    unused = vehicleBuilder.build();
+
+    TimingBelt expected = TimingBelt.newBuilder().setNumberOfTeeth(124).build();
+    vehicleBuilder.getEngineBuilder().setTimingBelt(expected);
+    // Testing that b/254158939 is fixed. It used to be that the setTimingBelt call above didn't
+    // propagate a change notification and the call below would return a stale version of the timing
+    // belt.
+    assertThat(vehicleBuilder.getEngine().getTimingBelt()).isEqualTo(expected);
+  }
+
   @Test
   public void testMessagesAndBuilders() {
     Vehicle.Builder vehicleBuilder = Vehicle.newBuilder();

diff --git a/java/core/src/test/proto/com/google/protobuf/nested_builders_test.proto b/java/core/src/test/proto/com/google/protobuf/nested_builders_test.proto
@@ -45,6 +45,11 @@ message Vehicle {
 message Engine {
   optional int32 cylinder = 1;
   optional int32 liters = 2;
+  optional TimingBelt timing_belt = 3;
+}
+
+message TimingBelt {
+  optional int32 number_of_teeth = 1;
 }
 
 message Wheel {