Skip to content

Commit

Permalink
Merge pull request #9714 from JasonLunn/3.20.x
Browse files Browse the repository at this point in the history 
Cherry pick JRuby changes that fix NPE during encoding (#9507) and implements respond_to? (#9202) into 3.20.x.
  • Loading branch information
@jorgbrown
jorgbrown committed Apr 2, 2022
2 parents faa42e9 + 31a6482 commit e097b36
Show file tree
Hide file tree
Showing 5 changed files with 382 additions and 37 deletions.
2 changes: 2 additions & 0 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyDescriptor.java
View file
Expand Up @@ -209,6 +209,8 @@ public IRubyObject allocate(Ruby runtime, RubyClass klazz) {
klass.include(new IRubyObject[] {messageExts});
klass.instance_variable_set(runtime.newString(Utils.DESCRIPTOR_INSTANCE_VAR), this);
klass.defineAnnotatedMethods(RubyMessage.class);
// Workaround for https://github.com/jruby/jruby/issues/7154
klass.searchMethod("respond_to?").setIsBuiltin(false);
return klass;
}

Expand Down
117 changes: 103 additions & 14 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
View file
Expand Up @@ -62,6 +62,9 @@
import java.util.Map;

public class RubyMessage extends RubyObject {
private final String DEFAULT_VALUE = "google.protobuf.FieldDescriptorProto.default_value";
private final String TYPE = "type";

public RubyMessage(Ruby runtime, RubyClass klazz, Descriptor descriptor) {
super(runtime, klazz);

Expand Down Expand Up @@ -257,6 +260,86 @@ public IRubyObject eq(ThreadContext context, IRubyObject other) {
return runtime.getTrue();
}

/*
* call-seq:
* Message.respond_to?(method_name, search_private_and_protected) => boolean
*
* Parallels method_missing, returning true when this object implements a method with the given
* method_name.
*/
@JRubyMethod(name="respond_to?", required = 1, optional = 1)
public IRubyObject respondTo(ThreadContext context, IRubyObject [] args) {
String methodName = args[0].asJavaString();
if (descriptor.findFieldByName(methodName) != null) {
return context.runtime.getTrue();
}
RubyDescriptor rubyDescriptor = (RubyDescriptor) getDescriptor(context, metaClass);
IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, args[0]);
if (!oneofDescriptor.isNil()) {
return context.runtime.getTrue();
}
if (methodName.startsWith(CLEAR_PREFIX)) {
String strippedMethodName = methodName.substring(6);
oneofDescriptor = rubyDescriptor.lookupOneof(context, context.runtime.newSymbol(strippedMethodName));
if (!oneofDescriptor.isNil()) {
return context.runtime.getTrue();
}

if (descriptor.findFieldByName(strippedMethodName) != null) {
return context.runtime.getTrue();
}
}
if (methodName.startsWith(HAS_PREFIX) && methodName.endsWith(QUESTION_MARK)) {
String strippedMethodName = methodName.substring(4, methodName.length() - 1);
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(strippedMethodName);
if (fieldDescriptor != null &&
(!proto3 || fieldDescriptor.getContainingOneof() == null || fieldDescriptor
.getContainingOneof().isSynthetic()) &&
fieldDescriptor.hasPresence()) {
return context.runtime.getTrue();
}
oneofDescriptor = rubyDescriptor.lookupOneof(context, RubyString.newString(context.runtime, strippedMethodName));
if (!oneofDescriptor.isNil()) {
return context.runtime.getTrue();
}
}
if (methodName.endsWith(AS_VALUE_SUFFIX)) {
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(
methodName.substring(0, methodName.length() - 9));
if (fieldDescriptor != null && isWrappable(fieldDescriptor)) {
return context.runtime.getTrue();
}
}
if (methodName.endsWith(CONST_SUFFIX)) {
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(
methodName.substring(0, methodName.length() - 6));
if (fieldDescriptor != null) {
if (fieldDescriptor.getType() == FieldDescriptor.Type.ENUM) {
return context.runtime.getTrue();
}
}
}
if (methodName.endsWith(Utils.EQUAL_SIGN)) {
String strippedMethodName = methodName.substring(0, methodName.length() - 1);
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(strippedMethodName);
if (fieldDescriptor != null) {
return context.runtime.getTrue();
}
if (strippedMethodName.endsWith(AS_VALUE_SUFFIX)) {
strippedMethodName = methodName.substring(0, strippedMethodName.length() - 9);
fieldDescriptor = descriptor.findFieldByName(strippedMethodName);
if (fieldDescriptor != null && isWrappable(fieldDescriptor)) {
return context.runtime.getTrue();
}
}
}
boolean includePrivate = false;
if (args.length == 2) {
includePrivate = context.runtime.getTrue().equals(args[1]);
}
return metaClass.respondsToMethod(methodName, includePrivate) ? context.runtime.getTrue() : context.runtime.getFalse();
}

/*
* call-seq:
* Message.method_missing(*args)
Expand Down Expand Up @@ -288,10 +371,9 @@ public IRubyObject eq(ThreadContext context, IRubyObject other) {
public IRubyObject methodMissing(ThreadContext context, IRubyObject[] args) {
Ruby runtime = context.runtime;
String methodName = args[0].asJavaString();
RubyDescriptor rubyDescriptor = (RubyDescriptor) getDescriptor(context, metaClass);

if (args.length == 1) {
RubyDescriptor rubyDescriptor = (RubyDescriptor) getDescriptor(context, metaClass);

// If we find a Oneof return it's name (use lookupOneof because it has an index)
IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, args[0]);

Expand Down Expand Up @@ -328,9 +410,12 @@ public IRubyObject methodMissing(ThreadContext context, IRubyObject[] args) {
if (methodName.startsWith(CLEAR_PREFIX)) {
methodName = methodName.substring(6);
oneofDescriptor = rubyDescriptor.lookupOneof(context, runtime.newSymbol(methodName));

if (!oneofDescriptor.isNil()) {
fieldDescriptor = oneofCases.get(((RubyOneofDescriptor) oneofDescriptor).getDescriptor());
if (fieldDescriptor == null) {
// Clearing an already cleared oneof; return here to avoid NoMethodError.
return context.nil;
}
}

if (fieldDescriptor == null) {
Expand Down Expand Up @@ -376,8 +461,7 @@ public IRubyObject methodMissing(ThreadContext context, IRubyObject[] args) {
} else if (methodName.endsWith(CONST_SUFFIX)) {
methodName = methodName.substring(0, methodName.length() - 6);
fieldDescriptor = descriptor.findFieldByName(methodName);

if (fieldDescriptor.getType() == FieldDescriptor.Type.ENUM) {
if (fieldDescriptor != null && fieldDescriptor.getType() == FieldDescriptor.Type.ENUM) {
IRubyObject enumValue = getFieldInternal(context, fieldDescriptor);

if (!enumValue.isNil()) {
Expand All @@ -401,17 +485,21 @@ public IRubyObject methodMissing(ThreadContext context, IRubyObject[] args) {

methodName = methodName.substring(0, methodName.length() - 1); // Trim equals sign
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(methodName);

if (fieldDescriptor != null) {
return setFieldInternal(context, fieldDescriptor, args[1]);
}

IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, RubyString.newString(context.runtime, methodName));
if (!oneofDescriptor.isNil()) {
throw runtime.newRuntimeError("Oneof accessors are read-only.");
}

if (methodName.endsWith(AS_VALUE_SUFFIX)) {
methodName = methodName.substring(0, methodName.length() - 9);

fieldDescriptor = descriptor.findFieldByName(methodName);

if (fieldDescriptor != null) {
if (fieldDescriptor != null && isWrappable(fieldDescriptor)) {
if (args[1].isNil()) {
return setFieldInternal(context, fieldDescriptor, args[1]);
}
Expand Down Expand Up @@ -677,6 +765,8 @@ protected DynamicMessage build(ThreadContext context, int depth, int recursionLi
throw context.runtime.newRuntimeError("Recursion limit exceeded during encoding.");
}

RubySymbol typeBytesSymbol = RubySymbol.newSymbol(context.runtime, "TYPE_BYTES");

// Handle the typical case where the fields.keySet contain the fieldDescriptors
for (FieldDescriptor fieldDescriptor : fields.keySet()) {
IRubyObject value = fields.get(fieldDescriptor);
Expand Down Expand Up @@ -707,13 +797,12 @@ protected DynamicMessage build(ThreadContext context, int depth, int recursionLi
* stringDefaultValue}.
*/
boolean isDefaultStringForBytes = false;
FieldDescriptor enumFieldDescriptorForType =
this.builder.getDescriptorForType().findFieldByName("type");
String type = enumFieldDescriptorForType == null ?
null : fields.get(enumFieldDescriptorForType).toString();
if (type != null && type.equals("TYPE_BYTES") &&
fieldDescriptor.getFullName().equals("google.protobuf.FieldDescriptorProto.default_value")) {
isDefaultStringForBytes = true;
if (DEFAULT_VALUE.equals(fieldDescriptor.getFullName())) {
FieldDescriptor enumFieldDescriptorForType =
this.builder.getDescriptorForType().findFieldByName(TYPE);
if (typeBytesSymbol.equals(fields.get(enumFieldDescriptorForType))) {
isDefaultStringForBytes = true;
}
}
builder.setField(fieldDescriptor, convert(context, fieldDescriptor, value, depth, recursionLimit, isDefaultStringForBytes));
}
Expand Down
91 changes: 80 additions & 11 deletions ruby/tests/basic.rb
View file
Expand Up @@ -66,8 +66,11 @@ def test_issue_8311_crash
def test_issue_8559_crash
msg = TestMessage.new
msg.repeated_int32 = ::Google::Protobuf::RepeatedField.new(:int32, [1, 2, 3])
# TODO: Remove the platform check once https://github.com/jruby/jruby/issues/6818 is released in JRuby 9.3.0.0
GC.start(full_mark: true, immediate_sweep: true) unless RUBY_PLATFORM == "java"

# https://github.com/jruby/jruby/issues/6818 was fixed in JRuby 9.3.0.0
if cruby_or_jruby_9_3_or_higher?
GC.start(full_mark: true, immediate_sweep: true)
end
TestMessage.encode(msg)
end

Expand All @@ -79,6 +82,34 @@ def test_issue_9440
assert_equal 8, msg.id
end

def test_issue_9507
pool = Google::Protobuf::DescriptorPool.new
pool.build do
add_message "NpeMessage" do
optional :type, :enum, 1, "TestEnum"
optional :other, :string, 2
end
add_enum "TestEnum" do
value :Something, 0
end
end

msgclass = pool.lookup("NpeMessage").msgclass

m = msgclass.new(
other: "foo" # must be set, but can be blank
)

begin
encoded = msgclass.encode(m)
rescue java.lang.NullPointerException
flunk "NPE rescued"
end
decoded = msgclass.decode(encoded)
decoded.inspect
decoded.to_proto
end

def test_has_field
m = TestSingularFields.new
assert !m.has_singular_msg?
Expand Down Expand Up @@ -145,7 +176,7 @@ def test_set_clear_defaults
m = TestSingularFields.new

m.singular_int32 = -42
assert_equal -42, m.singular_int32
assert_equal( -42, m.singular_int32 )
m.clear_singular_int32
assert_equal 0, m.singular_int32

Expand Down Expand Up @@ -540,8 +571,6 @@ def test_to_h


def test_json_maps
# TODO: Fix JSON in JRuby version.
return if RUBY_PLATFORM == "java"
m = MapMessage.new(:map_string_int32 => {"a" => 1})
expected = {mapStringInt32: {a: 1}, mapStringMsg: {}, mapStringEnum: {}}
expected_preserve = {map_string_int32: {a: 1}, map_string_msg: {}, map_string_enum: {}}
Expand All @@ -555,8 +584,6 @@ def test_json_maps
end

def test_json_maps_emit_defaults_submsg
# TODO: Fix JSON in JRuby version.
return if RUBY_PLATFORM == "java"
m = MapMessage.new(:map_string_msg => {"a" => TestMessage2.new(foo: 0)})
expected = {mapStringInt32: {}, mapStringMsg: {a: {foo: 0}}, mapStringEnum: {}}

Expand All @@ -566,8 +593,6 @@ def test_json_maps_emit_defaults_submsg
end

def test_json_emit_defaults_submsg
# TODO: Fix JSON in JRuby version.
return if RUBY_PLATFORM == "java"
m = TestSingularFields.new(singular_msg: proto_module::TestMessage2.new)

expected = {
Expand All @@ -590,8 +615,6 @@ def test_json_emit_defaults_submsg
end

def test_respond_to
# This test fails with JRuby 1.7.23, likely because of an old JRuby bug.
return if RUBY_PLATFORM == "java"
msg = MapMessage.new
assert msg.respond_to?(:map_string_int32)
assert !msg.respond_to?(:bacon)
Expand Down Expand Up @@ -666,5 +689,51 @@ def test_utf8
m2 = proto_module::TestMessage.decode(proto_module::TestMessage.encode(m))
assert_equal m2, m
end

def test_map_fields_respond_to? # regression test for issue 9202
msg = proto_module::MapMessage.new
assert msg.respond_to?(:map_string_int32=)
msg.map_string_int32 = Google::Protobuf::Map.new(:string, :int32)
assert msg.respond_to?(:map_string_int32)
assert_equal( Google::Protobuf::Map.new(:string, :int32), msg.map_string_int32 )
assert msg.respond_to?(:clear_map_string_int32)
msg.clear_map_string_int32

assert !msg.respond_to?(:has_map_string_int32?)
assert_raise NoMethodError do
msg.has_map_string_int32?
end
assert !msg.respond_to?(:map_string_int32_as_value)
assert_raise NoMethodError do
msg.map_string_int32_as_value
end
assert !msg.respond_to?(:map_string_int32_as_value=)
assert_raise NoMethodError do
msg.map_string_int32_as_value = :boom
end
end
end

def test_oneof_fields_respond_to? # regression test for issue 9202
msg = proto_module::OneofMessage.new
# `has_` prefix + "?" suffix actions should only work for oneofs fields.
assert msg.has_my_oneof?
assert msg.respond_to? :has_my_oneof?
assert !msg.respond_to?( :has_a? )
assert_raise NoMethodError do
msg.has_a?
end
assert !msg.respond_to?( :has_b? )
assert_raise NoMethodError do
msg.has_b?
end
assert !msg.respond_to?( :has_c? )
assert_raise NoMethodError do
msg.has_c?
end
assert !msg.respond_to?( :has_d? )
assert_raise NoMethodError do
msg.has_d?
end
end
end
17 changes: 16 additions & 1 deletion ruby/tests/basic_proto2.rb
View file
Expand Up @@ -142,7 +142,7 @@ def test_set_clear_defaults
m = TestMessageDefaults.new

m.optional_int32 = -42
assert_equal -42, m.optional_int32
assert_equal( -42, m.optional_int32 )
assert m.has_optional_int32?
m.clear_optional_int32
assert_equal 1, m.optional_int32
Expand Down Expand Up @@ -255,5 +255,20 @@ def test_file_descriptor
assert_equal "tests/basic_test_proto2.proto", file_descriptor.name
assert_equal :proto2, file_descriptor.syntax
end

def test_oneof_fields_respond_to? # regression test for issue 9202
msg = proto_module::OneofMessage.new(a: "foo")
# `has_` prefix + "?" suffix actions should only work for oneofs fields.
assert msg.respond_to? :has_my_oneof?
assert msg.has_my_oneof?
assert msg.respond_to? :has_a?
assert msg.has_a?
assert msg.respond_to? :has_b?
assert !msg.has_b?
assert msg.respond_to? :has_c?
assert !msg.has_c?
assert msg.respond_to? :has_d?
assert !msg.has_d?
end
end
end

0 comments on commit e097b36

Please sign in to comment.