Descriptors$FieldDescriptor.<init>(Descriptors.java:1107) isPacked() NPE

[protobufel]

Here is the excerpt from Descriptors$FieldDescriptor constructor:

    private FieldDescriptor(final FieldDescriptorProto proto,
                            final FileDescriptor file,
                            final Descriptor parent,
                            final int index,
                            final boolean isExtension)
                     throws DescriptorValidationException {
      this.index = index;
      this.proto = proto;
      fullName = computeFullName(file, parent, proto.getName());
      this.file = file;

      if (proto.hasType()) {
        type = Type.valueOf(proto.getType());
      }

      if (getNumber() <= 0) {
        throw new DescriptorValidationException(this,
          "Field numbers must be positive integers.");
      }

      // Only repeated primitive fields may be packed.
      if (proto.getOptions().getPacked() && !isPackable()) {

... 

    public boolean isPackable() {
      return isRepeated() && getLiteType().isPackable();
    }

    public WireFormat.FieldType getLiteType() {
      return table[type.ordinal()];
    }

However, if there is an enum or a message proto field with unresolved type (which is okay during cross-linking), then isPackable() will NPE because type ==null in getLiteType() !

This is a serious bug, preventing build of ANY protos with enum or message fields with only the TypeName set. The protoc does its own cross-linking so its Type fields are always set, anything else will fail miserably!

The resolution is simple add type != null in this condition, and if wanted check again after cross-linking:

if ((type != null) && proto.getOptions().getPacked() && !isPackable()) {

Thanks in advance, and hope it will be released soon!

Regards,
David

[protobufel]

And here is the test for this case:

  @Test(expected = NullPointerException.class)
  public void testPackedEnumFieldInProto() throws Exception {
    final FileDescriptorProto.Builder protoBuilder = FileDescriptorProto.newBuilder()
        .setName("test1.proto");
    protoBuilder.addEnumTypeBuilder()
        .setName("Enum1").addValueBuilder().setName("V1").setNumber(1);
    protoBuilder.addMessageTypeBuilder()
        .setName("Message1").addFieldBuilder().setName("field1").setTypeName("Enum1").setNumber(999)
        .setLabel(Label.LABEL_REPEATED).getOptionsBuilder().setPacked(true);

    // The buildFrom() will fail, but it shouldn't!
    final FileDescriptor fileDescriptor = FileDescriptor.buildFrom(protoBuilder.build(), new FileDescriptor[0]);
  }

or just remove (expected = NullPointerException.class) and see it fail!

[protobufel]

BTW, you're not seeing a lot of problems because the protoc does a lot of cross-linking/name resolutions in protos for you already, so it gives you nice RESOLVED ones, and a lot of the Java "build" stuff, therefore, goes untested and problems are undetected!