Merge pull request #332 from skippy/nil-out-of-bounds

ruby: return nil for out of bounds

ruby/README.md

@@ -63,7 +63,7 @@ To build this Ruby extension, you will need:
 To Build the JRuby extension, you will need:
 
 * Maven
-* The latest version of the protobuf java library
+* The latest version of the protobuf java library (see ../java/README.md)
 * Install JRuby via rbenv or RVM
 
 First switch to the desired platform with rbenv or RVM.
@@ -75,7 +75,8 @@ Then install the required Ruby gems:
 
 Then build the Gem:
 
-    $ rake gem
+    $ rake
+    $ rake clobber_package gem
     $ gem install `ls pkg/google-protobuf-*.gem`
 
 To run the specs:

ruby/Rakefile

@@ -6,6 +6,9 @@ require "rake/testtask"
 spec = Gem::Specification.load("google-protobuf.gemspec")
 
 if RUBY_PLATFORM == "java"
+  if `which mvn` == ''
+    raise ArgumentError, "maven needs to be installed"
+  end
   task :clean do
     system("mvn clean")
   end

ruby/ext/google/protobuf_c/repeated_field.c

@@ -47,6 +47,15 @@ RepeatedField* ruby_to_RepeatedField(VALUE _self) {
   return self;
 }
 
+static int index_position(VALUE _index, RepeatedField* repeated_field) {
+  int index = NUM2INT(_index);
+  if (index < 0 && repeated_field->size > 0) {
+    index = repeated_field->size + index;
+  }
+  return index;
+}
+
+
 /*
  * call-seq:
  *     RepeatedField.each(&block)
@@ -74,8 +83,7 @@ VALUE RepeatedField_each(VALUE _self) {
  * call-seq:
  *     RepeatedField.[](index) => value
  *
- * Accesses the element at the given index. Throws an exception on out-of-bounds
- * errors.
+ * Accesses the element at the given index. Returns nil on out-of-bounds
  */
 VALUE RepeatedField_index(VALUE _self, VALUE _index) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
@@ -83,9 +91,9 @@ VALUE RepeatedField_index(VALUE _self, VALUE _index) {
   upb_fieldtype_t field_type = self->field_type;
   VALUE field_type_class = self->field_type_class;
 
-  int index = NUM2INT(_index);
+  int index = index_position(_index, self);
   if (index < 0 || index >= self->size) {
-    rb_raise(rb_eRangeError, "Index out of range");
+    return Qnil;
   }
 
   void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
@@ -105,9 +113,9 @@ VALUE RepeatedField_index_set(VALUE _self, VALUE _index, VALUE val) {
   VALUE field_type_class = self->field_type_class;
   int element_size = native_slot_size(field_type);
 
-  int index = NUM2INT(_index);
+  int index = index_position(_index, self);
   if (index < 0 || index >= (INT_MAX - 1)) {
-    rb_raise(rb_eRangeError, "Index out of range");
+    return Qnil;
   }
   if (index >= self->size) {
     RepeatedField_reserve(self, index + 1);

ruby/pom.xml

@@ -78,7 +78,7 @@
         <dependency>
             <groupId>com.google.protobuf</groupId>
             <artifactId>protobuf-java</artifactId>
-            <version>3.0.0-pre</version>
+            <version>3.0.0-alpha-3-pre</version>
         </dependency>
     </dependencies>
 </project>

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

@@ -246,16 +246,15 @@ public class RubyMessage extends RubyObject {
     public IRubyObject dup(ThreadContext context) {
         RubyMessage dup = (RubyMessage) metaClass.newInstance(context, Block.NULL_BLOCK);
         IRubyObject value;
-        for (Descriptors.FieldDescriptor fieldDescriptor : builder.getAllFields().keySet()) {
+        for (Descriptors.FieldDescriptor fieldDescriptor : this.descriptor.getFields()) {
             if (fieldDescriptor.isRepeated()) {
-                dup.repeatedFields.put(fieldDescriptor, getRepeatedField(context, fieldDescriptor));
-            } else if (builder.hasField(fieldDescriptor)) {
-                dup.fields.put(fieldDescriptor, wrapField(context, fieldDescriptor, builder.getField(fieldDescriptor)));
+                dup.addRepeatedField(fieldDescriptor, this.getRepeatedField(context, fieldDescriptor));
+            } else if (fields.containsKey(fieldDescriptor)) {
+                dup.fields.put(fieldDescriptor, fields.get(fieldDescriptor));
+            } else if (this.builder.hasField(fieldDescriptor)) {
+                dup.fields.put(fieldDescriptor, wrapField(context, fieldDescriptor, this.builder.getField(fieldDescriptor)));
             }
         }
-        for (Descriptors.FieldDescriptor fieldDescriptor : fields.keySet()) {
-            dup.fields.put(fieldDescriptor, fields.get(fieldDescriptor));
-        }
         for (Descriptors.FieldDescriptor fieldDescriptor : maps.keySet()) {
             dup.maps.put(fieldDescriptor, maps.get(fieldDescriptor));
         }
@@ -411,6 +410,7 @@ public class RubyMessage extends RubyObject {
         for (int i = 0; i < count; i++) {
             ret.push(context, wrapField(context, fieldDescriptor, this.builder.getRepeatedField(fieldDescriptor, i)));
         }
+        addRepeatedField(fieldDescriptor, ret);
         return ret;
     }
 

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

@@ -108,8 +108,9 @@ public class RubyRepeatedField extends RubyObject {
      */
     @JRubyMethod(name = "[]=")
     public IRubyObject indexSet(ThreadContext context, IRubyObject index, IRubyObject value) {
+        int arrIndex = normalizeArrayIndex(index);
         Utils.checkType(context, fieldType, value, (RubyModule) typeClass);
-        this.storage.set(RubyNumeric.num2int(index), value);
+        this.storage.set(arrIndex, value);
         return context.runtime.getNil();
     }
 
@@ -117,12 +118,15 @@ public class RubyRepeatedField extends RubyObject {
      * call-seq:
      *     RepeatedField.[](index) => value
      *
-     * Accesses the element at the given index. Throws an exception on out-of-bounds
-     * errors.
+     * Accesses the element at the given index. Returns nil on out-of-bounds
      */
     @JRubyMethod(name = "[]")
     public IRubyObject index(ThreadContext context, IRubyObject index) {
-        return this.storage.eltInternal(RubyNumeric.num2int(index));
+        int arrIndex = normalizeArrayIndex(index);
+        if (arrIndex < 0 || arrIndex >= this.storage.size()) {
+            return context.runtime.getNil();
+        }
+        return this.storage.eltInternal(arrIndex);
     }
 
     /*
@@ -134,8 +138,7 @@ public class RubyRepeatedField extends RubyObject {
     @JRubyMethod(rest = true)
     public IRubyObject insert(ThreadContext context, IRubyObject[] args) {
         for (int i = 0; i < args.length; i++) {
-            Utils.checkType(context, fieldType, args[i], (RubyModule) typeClass);
-            this.storage.add(args[i]);
+            push(context, args[i]);
         }
         return context.runtime.getNil();
     }
@@ -385,6 +388,15 @@ public class RubyRepeatedField extends RubyObject {
         }
     }
 
+    private int normalizeArrayIndex(IRubyObject index) {
+        int arrIndex = RubyNumeric.num2int(index);
+        int arrSize = this.storage.size();
+        if (arrIndex < 0 && arrSize > 0) {
+            arrIndex = arrSize + arrIndex;
+        }
+        return arrIndex;
+    }
+
     private RubyArray storage;
     private Descriptors.FieldDescriptor.Type fieldType;
     private IRubyObject typeClass;

ruby/tests/basic.rb

@@ -314,6 +314,17 @@ module BasicTest
       assert l4 == [0, 0, 0, 0, 0, 42, 100, 101, 102]
     end
 
+    def test_parent_rptfield
+      #make sure we set the RepeatedField and can add to it
+      m = TestMessage.new
+      assert m.repeated_string == []
+      m.repeated_string << 'ok'
+      m.repeated_string.push('ok2')
+      assert m.repeated_string == ['ok', 'ok2']
+      m.repeated_string += ['ok3']
+      assert m.repeated_string == ['ok', 'ok2', 'ok3']
+    end
+
     def test_rptfield_msg
       l = Google::Protobuf::RepeatedField.new(:message, TestMessage)
       l.push TestMessage.new
@@ -383,10 +394,31 @@ module BasicTest
       length_methods.each do |lm|
         assert l.send(lm)  == 0
       end
+      # out of bounds returns a nil
+      assert l[0] == nil
+      assert l[1] == nil
+      assert l[-1] == nil
       l.push 4
       length_methods.each do |lm|
-        assert l.send(lm)  == 1
+        assert l.send(lm) == 1
+      end
+      assert l[0] == 4
+      assert l[1] == nil
+      assert l[-1] == 4
+      assert l[-2] == nil
+
+      l.push 2
+      length_methods.each do |lm|
+        assert l.send(lm) == 2
       end
+      assert l[0] == 4
+      assert l[1] == 2
+      assert l[2] == nil
+      assert l[-1] == 2
+      assert l[-2] == 4
+      assert l[-3] == nil
+
+      #adding out of scope will backfill with empty objects
     end
 
     def test_map_basic
@@ -724,9 +756,12 @@ module BasicTest
       m = TestMessage.new
       m.optional_string = "hello"
       m.optional_int32 = 42
-      m.repeated_msg.push TestMessage2.new(:foo => 100)
-      m.repeated_msg.push TestMessage2.new(:foo => 200)
-
+      tm1 = TestMessage2.new(:foo => 100)
+      tm2 = TestMessage2.new(:foo => 200)
+      m.repeated_msg.push tm1
+      assert m.repeated_msg[-1] == tm1
+      m.repeated_msg.push tm2
+      assert m.repeated_msg[-1] == tm2
       m2 = m.dup
       assert m == m2
       m.optional_int32 += 1