Implement respond_to? in RubyMessage (protocolbuffers#9677)

All synthetic methods implemented by `method_missing` are now supported by `respond_to?`.

Fixes issue protocolbuffers#9202.

* Fix null pointer exceptions exposed by new regression tests.
* Fix clear_ on oneofs so that it is safe to call repeatedly and so that respond_to? does not depend on whether the oneof is currently cleared.
* Code cleanup: reenable more tests on JRuby.
* Align JRuby behavior with CRuby by throwing a RuntimeError when attempting to assign to a oneof.

ruby/src/main/java/com/google/protobuf/jruby/RubyDescriptor.java

@@ -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;
     }
 

ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java

@@ -260,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)
@@ -291,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]);
 
@@ -331,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) {
@@ -379,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()) {
@@ -404,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]);
                     }

ruby/tests/basic.rb

@@ -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
 
@@ -99,7 +102,7 @@ def test_issue_9507
 
       begin
         encoded = msgclass.encode(m)
-      rescue java.lang.NullPointerException => e
+      rescue java.lang.NullPointerException
         flunk "NPE rescued"
       end
       decoded = msgclass.decode(encoded)
@@ -173,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
 
@@ -568,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: {}}
@@ -583,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: {}}
 
@@ -594,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 = {
@@ -618,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)
@@ -694,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

ruby/tests/basic_proto2.rb

@@ -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
@@ -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