Allow repeated custom option, or disallow it as in <=2.5

[protobufel]

The recent change in 2.6.0 to kind of allow for repeated custom options, as seen in google/protobuf/unittest_custom_options.proto, feels and is, IMHO, as a hack for allowing repeated fields in custom options.

1. This is inconsistent with the "inlined" field options, that don't allow for such repetition - protoc throws a parsing error.
2. This is confusing and requires a relax, "strange" handling - what before 2.6.0 was a single field now becomes a repeated field after options' extension resolution, aka re-parsing.
3. In most cases, such repetition is really an unintentional user error.
4. If repeated fields in options to be supported, then there are, as usual, two separate cases to handle - singular, and repeated fields.

Therefore, the right thing to do is either disallow repeated option fields as before, or allow repeated LABEL and option LABEL along with it.

In case of both allowed, re-parsing of the FileDescriptorProto in FileDescriptor.buildFrom, should result in either repeated, or optional field/extension set on options.

Here is the google/protobuf/unittest_custom_options.proto excerpt:

// Note that we try various different ways of naming the same extension.
message VariousComplexOptions {
option (.protobuf_unittest.complex_opt1).foo = 42;
option (protobuf_unittest.complex_opt1).(.protobuf_unittest.quux) = 324;
option (.protobuf_unittest.complex_opt1).(protobuf_unittest.corge).qux = 876;
option (protobuf_unittest.complex_opt1).foo4 = 99;
option (protobuf_unittest.complex_opt1).foo4 = 88;
option (complex_opt2).baz = 987;
option (complex_opt2).(grault) = 654;
option (complex_opt2).bar.foo = 743;
option (complex_opt2).bar.(quux) = 1999;
option (complex_opt2).bar.(protobuf_unittest.corge).qux = 2008;
option (complex_opt2).(garply).foo = 741;
option (complex_opt2).(garply).(.protobuf_unittest.quux) = 1998;
option (complex_opt2).(protobuf_unittest.garply).(corge).qux = 2121;
option (ComplexOptionType2.ComplexOptionType4.complex_opt4).waldo = 1971;
option (complex_opt2).fred.waldo = 321;
option (complex_opt2).barney = { waldo: 101 };
option (complex_opt2).barney = { waldo: 212 };
option (protobuf_unittest.complex_opt3).qux = 9;
option (complex_opt3).complexoptiontype5.plugh = 22;
option (complexopt6).xyzzy = 24;
}

And here is the same FileDescriptorProto, as seen by TextFormat:

message_type {
name: "VariousComplexOptions"
options {
7595468 {
7593951: 24
}
7633546: "\b\263\017"
7636463: "\b\t"
7636463: "\023\030\026\024"
7636949: "\020\333\a"
7636949: "\370\346\227\035\216\005"
7636949: "\n\003\b\347\005"
7636949: "\n\006\330\205\236\035\317\017"
7636949: "\n\b\222\365\235\035\003\b\330\017"
7636949: "\302\254\227\035\003\b\345\005"
7636949: "\302\254\227\035\006\330\205\236\035\316\017"
7636949: "\302\254\227\035\b\222\365\235\035\003\b\311\020"
7636949: "\032\003\b\301\002"
7636949: ""\002\be"
7636949: ""\003\b\324\001"
7646756: "\b*"
7646756: "\330\205\236\035\304\002"
7646756: "\222\365\235\035\003\b\354\006"
7646756: " c"
7646756: " X"
}
}

I believe in 2.5 it is (if one disabled protoc 2.5 error reporting) :

message_type {
name: "VariousComplexOptions"
options {
7595468 {
7593951: 24
}
7633546: "\b\263\017"
7636463: "\023\030\026\024"
7636949: ""\003\b\324\001"
7646756: " X"
}
}

In addition, I think that whole concept of custom options defined in the same proto as a message defined in that proto, is self-referencing in a bad way, which requires extra processing for the sake of nothing. Whoever wants custom options can define his/her own format within a custom value of type string; or protobuf can provide for a standard message type for custom options, with this message type fully defined in the standard descriptor.proto, resulting in MUCH less processing, MUCH less confusion, and MUCH clearer and CLEANER specification.

I intend to expand on this thought/critique in a separate request/issue.

Regards,
David

[protobufel]

I must add that the 2.6 introduced hack for a repeated custom option violates the following important invariant:

  @Test
  public void testReserializationInvariant() throws Exception {
    assertThat(reserializationInvariant(expectedFileDescriptor), is(equalTo(true)));  
}

  public boolean reserializationInvariant(final FileDescriptor file) {
    final FileDescriptorProto expectedProtoWithUnknownFields = file.toProto();

    final ExtensionRegistry registry = getRegistryFor(file);
    final FileDescriptorProto actualProtoWithExtensions = FileDescriptorProto.parseFrom(
        expectedProtoWithUnknownFields.toByteString(), registry);
    final FileDescriptorProto actualProtoWithUnknownFields = FileDescriptorProto.parseFrom(
        actualProtoWithExtensions.toByteString());
    return actualProtoWithUnknownFields.equals(expectedProtoWithUnknownFields);
  }

Here is the excerpt of diff between the original google/protobuf/unittest_custom_options.proto and its double serialization/deserialization:

1. original:

message_type {
  name: "VariousComplexOptions"
  options {
    7595468 {
      7593951: 24
    }
    7633546: "\b\263\017"
    7636463: "\b\t"
    7636463: "\023\030\026\024"
    7636949: "\020\333\a"
    7636949: "\370\346\227\035\216\005"
    7636949: "\n\003\b\347\005"
    7636949: "\n\006\330\205\236\035\317\017"
    7636949: "\n\b\222\365\235\035\003\b\330\017"
    7636949: "\302\254\227\035\003\b\345\005"
    7636949: "\302\254\227\035\006\330\205\236\035\316\017"
    7636949: "\302\254\227\035\b\222\365\235\035\003\b\311\020"
    7636949: "\032\003\b\301\002"
    7636949: "\"\002\be"
    7636949: "\"\003\b\324\001"
    7646756: "\b*"
    7646756: "\330\205\236\035\304\002"
    7646756: "\222\365\235\035\003\b\354\006"
    7646756: " c"
    7646756: " X"
  }
}

1. doubly serialized/deserialized:

message_type {
  name: "VariousComplexOptions"
  options {
    7595468 {
      7593951: 24
    }
    7633546: "\b\263\017"
    7636463: "\b\t\023\030\026\024"
    7636949: "\n\021\b\347\005\222\365\235\035\003\b\330\017\330\205\236\035\317\017\020\333\a\032\003\b\301\002\"\002\be\"\003\b\324\001\302\254\227\035\021\b\345\005\222\365\235\035\003\b\311\020\330\205\236\035\316\017\370\346\227\035\216\005"
    7646756: "\b* c X\222\365\235\035\003\b\354\006\330\205\236\035\304\002"
  }
}

In other words, a doubly serialized/deserialized proto must be equal to its original form, i.e. proto equals doubly_serialized_deserialized(proto).

Again, the proto 2.6 hack is troublesome; instead, just introduce a repeated extension for custom options, the way it is done for a regular repeated extensions, for example:

extend google.protobuf.FileOptions {
  repeated string  my_repeated_custom_option1 = 7736974;
  repeated int my_repeated_custom_option2 = 7736975;
  repeated Message1 my_repeated_custom_option3 = 7736976;
  ...
  optional string my_optional_custom_option1 = 7736977;
}

And as a quick fix for now, for protobuf 2.6.1, disallow repeating optional custom options as it had always been prior 2.6.0.

In summary, as it is of now, in 2.6.0 and 2.6.1RC1, the repeating optional custom options is not a hack, but a bug.

[protobufel]

Please disregard all this issue; look instead at issue #59.