Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark nested builder as clean after clear is called #10984

Merged
merged 4 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ option java_outer_classname = "NestedBuilders";
message Vehicle {
optional Engine engine = 1;
repeated Wheel wheel = 2;
optional TimingBelt timing_belt = 3;
}

message TimingBelt {
optional int32 number_of_teeth = 1;
}

message Engine {
Expand Down