Fix NPE during encoding and add regression test for issue 9507.

mkruskal-google · Mar 16, 2022 · 58e320a · 58e320a
1 parent 5dab094
commit 58e320a
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 7 deletions.
diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
@@ -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);
 
@@ -677,6 +680,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);
@@ -707,13 +712,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));
             }

diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb
@@ -79,6 +79,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 => e
+        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?