From 3ac4003eba97c9364ff27e6bf482e34bd5f3f0a8 Mon Sep 17 00:00:00 2001 From: Ran Chen Date: Sat, 10 Oct 2015 11:48:26 +0800 Subject: [PATCH 1/2] Improve assignment to array 1. Orignally checking for is_a?(Array) is before nil?, so when assigning nil to an repeated value, TypeError will be raised, and the nil? part will never be reached. This is rather undesirable I think. 2. Call field.coerce! when assigning to repeated value, to keep consistent with assigning to non-repeated value. --- lib/protobuf/field/base_field.rb | 13 +++--- lib/protobuf/field/field_array.rb | 2 +- spec/functional/assignment_spec.rb | 69 ++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 spec/functional/assignment_spec.rb diff --git a/lib/protobuf/field/base_field.rb b/lib/protobuf/field/base_field.rb index 440bbd72..35ffe930 100644 --- a/lib/protobuf/field/base_field.rb +++ b/lib/protobuf/field/base_field.rb @@ -194,6 +194,11 @@ def define_array_setter message_class.class_eval do define_method(method_name) do |val| + if val.nil? || (val.respond_to?(:empty?) && val.empty?) + @values.delete(field.name) + return + end + if val.is_a?(Array) val = val.dup val.compact! @@ -204,12 +209,8 @@ def define_array_setter TYPE_ERROR end - if val.nil? || (val.respond_to?(:empty?) && val.empty?) - @values.delete(field.name) - else - @values[field.name] ||= ::Protobuf::Field::FieldArray.new(field) - @values[field.name].replace(val) - end + @values[field.name] ||= ::Protobuf::Field::FieldArray.new(field) + @values[field.name].replace(val) end end diff --git a/lib/protobuf/field/field_array.rb b/lib/protobuf/field/field_array.rb index 78bb79e0..8cc521aa 100644 --- a/lib/protobuf/field/field_array.rb +++ b/lib/protobuf/field/field_array.rb @@ -71,7 +71,7 @@ def normalize(value) elsif field.is_a?(::Protobuf::Field::MessageField) && value.respond_to?(:to_hash) field.type_class.new(value.to_hash) else - value + field.coerce!(value) end end diff --git a/spec/functional/assignment_spec.rb b/spec/functional/assignment_spec.rb new file mode 100644 index 00000000..ea384872 --- /dev/null +++ b/spec/functional/assignment_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' +require 'spec/support/test/resource_service' + +RSpec.describe 'assign' do + class Foo < ::Protobuf::Message + repeated :string, :string, 1 + repeated :float, :float, 2 + repeated :double, :double, 3 + repeated :int32, :int32, 4 + repeated :int64, :int64, 5 + end + + let(:target) { Foo.new } + + specify 'string to repeated string' do + target.string = ["foo1", "foo2"] + expect(target.string).to eq(["foo1", "foo2"]) + end + + specify 'string to repeated int32' do + target.int32 = ["1", "-1"] + expect(target.int32).to eq([1, -1]) + end + + specify 'string to repeated int64' do + target.int64 = [String(2**41), String(-2**41)] + expect(target.int64).to eq([2**41, -2**41]) + end + + specify 'string to repeated float' do + target.float = ["1.1", "-1.1"] + expect(target.float).to eq([1.1, -1.1]) + end + + specify 'string to repeated double' do + target.double = ["1.1", "-1.1"] + expect(target.double).to eq([1.1, -1.1]) + end + + specify 'nil to repeated string' do + target.string = ["foo1", "foo2"] + target.string = nil + expect(target.string).to eq([]) + end + + specify 'nil to repeated int32' do + target.int32 = ["1", "2"] + target.int32 = nil + expect(target.int32).to eq([]) + end + + specify 'nil to repeated int64' do + target.int64 = [String(1**40), String(1**41)] + target.int64 = nil + expect(target.int64).to eq([]) + end + + specify 'nil to repeated float' do + target.float = ["1.1", "1.2"] + target.float = nil + expect(target.float).to eq([]) + end + + specify 'nil to repeated double' do + target.double = ["1.1", "1.2"] + target.double = nil + expect(target.double).to eq([]) + end +end From bbf38cd223ce18bdf989d22a0f0573f7516b2c6e Mon Sep 17 00:00:00 2001 From: Ran Chen Date: Mon, 12 Oct 2015 09:51:10 +0800 Subject: [PATCH 2/2] Update specs, move them to the right place --- spec/functional/assignment_spec.rb | 69 --------------------- spec/lib/protobuf/field/field_array_spec.rb | 19 ++++++ 2 files changed, 19 insertions(+), 69 deletions(-) delete mode 100644 spec/functional/assignment_spec.rb diff --git a/spec/functional/assignment_spec.rb b/spec/functional/assignment_spec.rb deleted file mode 100644 index ea384872..00000000 --- a/spec/functional/assignment_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'spec_helper' -require 'spec/support/test/resource_service' - -RSpec.describe 'assign' do - class Foo < ::Protobuf::Message - repeated :string, :string, 1 - repeated :float, :float, 2 - repeated :double, :double, 3 - repeated :int32, :int32, 4 - repeated :int64, :int64, 5 - end - - let(:target) { Foo.new } - - specify 'string to repeated string' do - target.string = ["foo1", "foo2"] - expect(target.string).to eq(["foo1", "foo2"]) - end - - specify 'string to repeated int32' do - target.int32 = ["1", "-1"] - expect(target.int32).to eq([1, -1]) - end - - specify 'string to repeated int64' do - target.int64 = [String(2**41), String(-2**41)] - expect(target.int64).to eq([2**41, -2**41]) - end - - specify 'string to repeated float' do - target.float = ["1.1", "-1.1"] - expect(target.float).to eq([1.1, -1.1]) - end - - specify 'string to repeated double' do - target.double = ["1.1", "-1.1"] - expect(target.double).to eq([1.1, -1.1]) - end - - specify 'nil to repeated string' do - target.string = ["foo1", "foo2"] - target.string = nil - expect(target.string).to eq([]) - end - - specify 'nil to repeated int32' do - target.int32 = ["1", "2"] - target.int32 = nil - expect(target.int32).to eq([]) - end - - specify 'nil to repeated int64' do - target.int64 = [String(1**40), String(1**41)] - target.int64 = nil - expect(target.int64).to eq([]) - end - - specify 'nil to repeated float' do - target.float = ["1.1", "1.2"] - target.float = nil - expect(target.float).to eq([]) - end - - specify 'nil to repeated double' do - target.double = ["1.1", "1.2"] - target.double = nil - expect(target.double).to eq([]) - end -end diff --git a/spec/lib/protobuf/field/field_array_spec.rb b/spec/lib/protobuf/field/field_array_spec.rb index 48133381..c980d46c 100644 --- a/spec/lib/protobuf/field/field_array_spec.rb +++ b/spec/lib/protobuf/field/field_array_spec.rb @@ -17,6 +17,7 @@ class SomeRepeatMessage < ::Protobuf::Message optional :string, :some_string, 1 repeated :string, :multiple_strings, 2 repeated SomeBasicMessage, :multiple_basic_msgs, 3 + repeated :int64, :multiple_integers, 4 end let(:instance) { SomeRepeatMessage.new } @@ -64,6 +65,24 @@ class SomeRepeatMessage < ::Protobuf::Message expect(instance.multiple_basic_msgs.first).to be_a(MoreComplexMessage) end end + + context 'when applied to an Integer field array' do + it 'does conversion if adding a string' do + expect(instance.multiple_integers).to be_empty + instance.multiple_integers.send(method, '1') + expect(instance.multiple_integers).to eq([1]) + end + end + end + + describe 'assign' do + context 'nil to an array' do + it 'empties the array' do + instance.multiple_strings = ['string 1'] + instance.multiple_strings = nil + expect(instance.multiple_strings).to be_empty + end + end end end end