Oneof's Message field builder does nothing when its sub-field changes

[protobufel]

Scenario:

1. get field builder on oneof Message field
2. set another field on this oneof
3. mutate any field on the field builder (1)

Expected:

There are 2 alternatives:

1. the exception like "this oneof field builder is unset" is raised, or
2. the oneof and its case is set back to this field

Actual:

The field builder allows to mutate it with no warnings, and the oneof case is not changed back to this field!

Proposed Remedy:

Add extra oneofFieldChanged(Internal.EnumLite | FieldDescriptor) method to GeneratedMessage.BuilderParent interface, and notify parent of any oneof field builder's mutations.

When notified, either throw an exception if oneof is set to different case, or change the oneof's case to this field's case.

The good news is that it just clarifies the existing behavior and doesn't change any public interfaces and the existing techniques. Not implementing this fix will cause numerous, difficult to trace, logical problems with oneof in such a common scenario.

In addition, due to the wrong bundle number bug, you have to issue minor update anyway; so this would be the opportune time for the proposed solution.

Cheers,
David

PS: Here is the full JUnit 4 test case for the scenario as-is:

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;

import org.junit.Test;

import protobuf_unittest.UnittestProto.TestAllTypes;
import protobuf_unittest.UnittestProto.TestAllTypes.Builder;
import protobuf_unittest.UnittestProto.TestAllTypes.OneofFieldCase;

public class OneofPropertyChangeTest {

@test
public void testOneOfPropertyChangeGeneratedBuilder() {
final Builder builder = TestAllTypes.newBuilder().setOneofString("hello");
assertThat(builder.getOneofFieldCase(), is(equalTo(OneofFieldCase.ONEOF_STRING)));

final TestAllTypes.NestedMessage.Builder innerBuilder = builder.getOneofNestedMessageBuilder();
assertThat(builder.getOneofFieldCase(), is(equalTo(OneofFieldCase.ONEOF_NESTED_MESSAGE)));

builder.setOneofString("hello again");
assertThat(builder.getOneofFieldCase(), is(equalTo(OneofFieldCase.ONEOF_STRING)));

/** 
 * when oneof's field builder property changes, either set oneof to this 
 * field, or raise exception when other field set on this oneof already!
 * At present, the builder will allow any its mutation with no effect on its parent!   
 */
innerBuilder.setBb(999);
// the following should not be failing, but it fails!
assertThat(builder.getOneofFieldCase(), is(equalTo(OneofFieldCase.ONEOF_NESTED_MESSAGE)));

}
}

[xfxyjwf]

We had a discussion about field lifetime issue just recently and here is assumption that I think users should make when accessing sub-fields of a protobuf message:

After getting a reference/pointer to a field of a protobuf message, any subsequent mutation method calls to the message may invalidate this reference/pointer and accessing this reference/pointer is an undefined behavior.

This not only applies to the case you mentioned, but also applies to other cases and other languages. For example, step 3 in the following scenario will result in undefined behavior:

1. List<Integer> list = builder.getRepeatedFieldList();
2. builder.addRepeatedField(value);
3. for (Integer element : list) {...}

It might be better to throw an exception to let the user aware of the problem, but given the implementation overhead to address this, I would prefer leaving it as-is.

[protobufel]

Collections' modCount like? :)

Understood!

David