diff --git a/CHANGES.txt b/CHANGES.txt index fa223a22d1..6f5751767a 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -40,8 +40,15 @@ * Optimized Java proto serialization gencode for protos having many extension ranges with few fields in between. * More thoroughly annotate public generated code in Java lite protocol buffers. * Fixed Bug in proto3 java lite repeated enum fields. Failed to call copyOnWrite before modifying previously built message. Causes modification to already "built" messages that should be immutable. - * Refactoring java full runtime to reuse sub-message builders and prepare to migrate parsing logic from parse constructor to builder. * Fix Java reflection serialization of empty packed fields. + * Refactoring java full runtime to reuse sub-message builders and prepare to + migrate parsing logic from parse constructor to builder. + * Move proto wireformat parsing functionality from the private "parsing + constructor" to the Builder class. + * Change the Lite runtime to prefer merging from the wireformat into mutable + messages rather than building up a new immutable object before merging. This + way results in fewer allocations and copy operations. + * Make message-type extensions merge from wire-format instead of building up instances and merging afterwards. This has much better performance. Python * Changes ordering of printed fields in .pyi files from lexicographic to the same ordering found in the proto descriptor. diff --git a/java/core/src/main/java/com/google/protobuf/MessageReflection.java b/java/core/src/main/java/com/google/protobuf/MessageReflection.java index e9bc9f1330..294d841774 100644 --- a/java/core/src/main/java/com/google/protobuf/MessageReflection.java +++ b/java/core/src/main/java/com/google/protobuf/MessageReflection.java @@ -378,6 +378,7 @@ class MessageReflection { static class BuilderAdapter implements MergeTarget { private final Message.Builder builder; + private boolean hasNestedBuilders = true; @Override public Descriptors.Descriptor getDescriptorForType() { @@ -393,6 +394,17 @@ class MessageReflection { return builder.getField(field); } + private Message.Builder getFieldBuilder(Descriptors.FieldDescriptor field) { + if (hasNestedBuilders) { + try { + return builder.getFieldBuilder(field); + } catch (UnsupportedOperationException e) { + hasNestedBuilders = false; + } + } + return null; + } + @Override public boolean hasField(Descriptors.FieldDescriptor field) { return builder.hasField(field); @@ -400,6 +412,12 @@ class MessageReflection { @Override public MergeTarget setField(Descriptors.FieldDescriptor field, Object value) { + if (!field.isRepeated() && value instanceof MessageLite.Builder) { + if (value != getFieldBuilder(field)) { + builder.setField(field, ((MessageLite.Builder) value).buildPartial()); + } + return this; + } builder.setField(field, value); return this; } @@ -413,12 +431,18 @@ class MessageReflection { @Override public MergeTarget setRepeatedField( Descriptors.FieldDescriptor field, int index, Object value) { + if (value instanceof MessageLite.Builder) { + value = ((MessageLite.Builder) value).buildPartial(); + } builder.setRepeatedField(field, index, value); return this; } @Override public MergeTarget addRepeatedField(Descriptors.FieldDescriptor field, Object value) { + if (value instanceof MessageLite.Builder) { + value = ((MessageLite.Builder) value).buildPartial(); + } builder.addRepeatedField(field, value); return this; } @@ -536,11 +560,19 @@ class MessageReflection { Message defaultInstance) throws IOException { if (!field.isRepeated()) { + Message.Builder subBuilder; if (hasField(field)) { - input.readGroup(field.getNumber(), builder.getFieldBuilder(field), extensionRegistry); - return; + subBuilder = getFieldBuilder(field); + if (subBuilder != null) { + input.readGroup(field.getNumber(), subBuilder, extensionRegistry); + return; + } else { + subBuilder = newMessageFieldInstance(field, defaultInstance); + subBuilder.mergeFrom((Message) getField(field)); + } + } else { + subBuilder = newMessageFieldInstance(field, defaultInstance); } - Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance); input.readGroup(field.getNumber(), subBuilder, extensionRegistry); Object unused = setField(field, subBuilder.buildPartial()); } else { @@ -558,11 +590,19 @@ class MessageReflection { Message defaultInstance) throws IOException { if (!field.isRepeated()) { + Message.Builder subBuilder; if (hasField(field)) { - input.readMessage(builder.getFieldBuilder(field), extensionRegistry); - return; + subBuilder = getFieldBuilder(field); + if (subBuilder != null) { + input.readMessage(subBuilder, extensionRegistry); + return; + } else { + subBuilder = newMessageFieldInstance(field, defaultInstance); + subBuilder.mergeFrom((Message) getField(field)); + } + } else { + subBuilder = newMessageFieldInstance(field, defaultInstance); } - Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance); input.readMessage(subBuilder, extensionRegistry); Object unused = setField(field, subBuilder.buildPartial()); } else { @@ -586,11 +626,14 @@ class MessageReflection { public MergeTarget newMergeTargetForField( Descriptors.FieldDescriptor field, Message defaultInstance) { Message.Builder subBuilder; - if (defaultInstance != null) { - subBuilder = defaultInstance.newBuilderForType(); - } else { - subBuilder = builder.newBuilderForField(field); + if (!field.isRepeated() && hasField(field)) { + subBuilder = getFieldBuilder(field); + if (subBuilder != null) { + return new BuilderAdapter(subBuilder); + } } + + subBuilder = newMessageFieldInstance(field, defaultInstance); if (!field.isRepeated()) { Message originalMessage = (Message) getField(field); if (originalMessage != null) { @@ -626,7 +669,7 @@ class MessageReflection { @Override public Object finish() { - return builder.buildPartial(); + return builder; } } diff --git a/ruby/compatibility_tests/v3.0.0/tests/basic.rb b/ruby/compatibility_tests/v3.0.0/tests/basic.rb index 7228144cb1..d45c19678f 100755 --- a/ruby/compatibility_tests/v3.0.0/tests/basic.rb +++ b/ruby/compatibility_tests/v3.0.0/tests/basic.rb @@ -667,8 +667,8 @@ module BasicTest assert m["z"] == :C m["z"] = 2 assert m["z"] == :B - m["z"] = 4 - assert m["z"] == 4 + m["z"] = 5 + assert m["z"] == 5 assert_raise RangeError do m["z"] = :Z end diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index b11878545f..5acff8096d 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -1263,15 +1263,20 @@ VALUE build_module_from_enumdesc(VALUE _enumdesc) { int n = upb_EnumDef_ValueCount(e); for (int i = 0; i < n; i++) { const upb_EnumValueDef* ev = upb_EnumDef_Value(e, i); - const char* name = upb_EnumValueDef_Name(ev); + char* name = strdup(upb_EnumValueDef_Name(ev)); int32_t value = upb_EnumValueDef_Number(ev); if (name[0] < 'A' || name[0] > 'Z') { - rb_warn( + if (name[0] >= 'a' && name[0] <= 'z') { + name[0] -= 32; // auto capitalize + } else { + rb_warn( "Enum value '%s' does not start with an uppercase letter " "as is required for Ruby constants.", name); + } } rb_define_const(mod, name, INT2NUM(value)); + free(name); } rb_define_singleton_method(mod, "lookup", enum_lookup, 1); diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java index 65328676e1..0eb7c939cb 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java @@ -162,9 +162,10 @@ public class RubyEnumDescriptor extends RubyObject { boolean defaultValueRequiredButNotFound = descriptor.getFile().getSyntax() == FileDescriptor.Syntax.PROTO3; for (EnumValueDescriptor value : descriptor.getValues()) { - String name = value.getName(); - // Make sure its a valid constant name before trying to create it - if (Character.isUpperCase(name.codePointAt(0))) { + String name = fixEnumConstantName(value.getName()); + // Make sure it's a valid constant name before trying to create it + int ch = name.codePointAt(0); + if (Character.isUpperCase(ch)) { enumModule.defineConstant(name, runtime.newFixnum(value.getNumber())); } else { runtime @@ -189,6 +190,22 @@ public class RubyEnumDescriptor extends RubyObject { return enumModule; } + private static String fixEnumConstantName(String name) { + if (name != null && name.length() > 0) { + int ch = name.codePointAt(0); + if (ch >= 'a' && ch <= 'z') { + // Protobuf enums can start with lowercase letters, while Ruby's constant should + // always start with uppercase letters. We tolerate this case by capitalizing + // the first character if possible. + return new StringBuilder() + .appendCodePoint(Character.toUpperCase(ch)) + .append(name.substring(1)) + .toString(); + } + } + return name; + } + private EnumDescriptor descriptor; private EnumDescriptorProto.Builder builder; private IRubyObject name; diff --git a/ruby/tests/basic_test.proto b/ruby/tests/basic_test.proto index fb70f479db..d480d48e54 100644 --- a/ruby/tests/basic_test.proto +++ b/ruby/tests/basic_test.proto @@ -73,6 +73,7 @@ enum TestEnum { A = 1; B = 2; C = 3; + v0 = 4; } message TestEmbeddedMessageParent { diff --git a/ruby/tests/basic_test_proto2.proto b/ruby/tests/basic_test_proto2.proto index 0c1a2b9836..ac705ed630 100644 --- a/ruby/tests/basic_test_proto2.proto +++ b/ruby/tests/basic_test_proto2.proto @@ -69,6 +69,7 @@ enum TestEnum { A = 1; B = 2; C = 3; + v0 = 4; } enum TestNonZeroEnum { diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 5918c8a8b1..928842553f 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -331,14 +331,16 @@ module CommonTests l.push :A l.push :B l.push :C - assert l.count == 3 + l.push :v0 + assert l.count == 4 assert_raise RangeError do l.push :D end assert l[0] == :A + assert l[3] == :v0 - l.push 4 - assert l[3] == 4 + l.push 5 + assert l[4] == 5 end def test_rptfield_initialize @@ -542,8 +544,8 @@ module CommonTests assert m["z"] == :C m["z"] = 2 assert m["z"] == :B - m["z"] = 4 - assert m["z"] == 4 + m["z"] = 5 + assert m["z"] == 5 assert_raise RangeError do m["z"] = :Z end @@ -712,14 +714,17 @@ module CommonTests assert proto_module::TestEnum::A == 1 assert proto_module::TestEnum::B == 2 assert proto_module::TestEnum::C == 3 + assert proto_module::TestEnum::V0 == 4 assert proto_module::TestEnum::lookup(1) == :A assert proto_module::TestEnum::lookup(2) == :B assert proto_module::TestEnum::lookup(3) == :C + assert proto_module::TestEnum::lookup(4) == :v0 assert proto_module::TestEnum::resolve(:A) == 1 assert proto_module::TestEnum::resolve(:B) == 2 assert proto_module::TestEnum::resolve(:C) == 3 + assert proto_module::TestEnum::resolve(:v0) == 4 end def test_enum_const_get_helpers @@ -788,7 +793,7 @@ module CommonTests assert_raise(NoMethodError) { m.a } assert_raise(NoMethodError) { m.a_const_const } end - + def test_repeated_push m = proto_module::TestMessage.new @@ -1762,7 +1767,7 @@ module CommonTests assert_raise(FrozenErrorType) { m.repeated_msg = proto_module::TestMessage2.new } assert_raise(FrozenErrorType) { m.repeated_enum = :A } end - + def test_eq m1 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2']) m2 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2']) diff --git a/ruby/tests/generated_code.proto b/ruby/tests/generated_code.proto index bfdfa5aa78..5f017ba7ca 100644 --- a/ruby/tests/generated_code.proto +++ b/ruby/tests/generated_code.proto @@ -67,6 +67,8 @@ enum TestEnum { A = 1; B = 2; C = 3; + + v0 = 4; } message testLowercaseNested { diff --git a/ruby/tests/generated_code_proto2.proto b/ruby/tests/generated_code_proto2.proto index 1e957219fa..1a50b84be5 100644 --- a/ruby/tests/generated_code_proto2.proto +++ b/ruby/tests/generated_code_proto2.proto @@ -68,6 +68,8 @@ enum TestEnum { A = 1; B = 2; C = 3; + + v0 = 4; } message TestUnknown { diff --git a/ruby/tests/repeated_field_test.rb b/ruby/tests/repeated_field_test.rb index 881810cdc5..de96869960 100755 --- a/ruby/tests/repeated_field_test.rb +++ b/ruby/tests/repeated_field_test.rb @@ -697,6 +697,7 @@ class RepeatedFieldTest < Test::Unit::TestCase value :A, 1 value :B, 2 value :C, 3 + value :v0, 4 end end