Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't downcast when assigning to a field_array #252

Merged
merged 1 commit into from
Apr 14, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/protobuf/field/field_array.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ def normalize(value)

if field.is_a?(::Protobuf::Field::EnumField)
field.type_class.fetch(value)
elsif field.is_a?(::Protobuf::Field::MessageField) && value.is_a?(field.type_class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, makes sense. Good spot.

value
elsif field.is_a?(::Protobuf::Field::MessageField) && value.respond_to?(:to_hash)
field.type_class.new(value.to_hash)
else
Expand Down
2 changes: 1 addition & 1 deletion lib/protobuf/field/message_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class MessageField < BaseField
#

def acceptable?(val)
unless val.instance_of?(type_class) || val.respond_to?(:to_hash)
unless val.is_a?(type_class) || val.is_a?(Hash)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may be a bit outside the scope of this fix. The reason I made this change was because this line was failing without it. IT seemed wrong that an incorrect object that has no relationship to the expected object should be acceptable. It was passing this method because of val.respond_to?(:to_hash), which seems too open ended. Also, using instance_of? doesn't take into account class inheritance while is_a? does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the change for is_a? but the swap from respond_to seems unnecessary. We use duck type checks all over in this lib purposefully. You should be able to pass a hash-like object here. If you change that back I'll get this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, if I move this to unless val.is_a?(type_class) || val.respond_to?(:to_hash), then this test fails.

I put that test in there because assigning an unrelated object to a MessageField and having it pass (and possibly drop fields silently if they don't match) seems wrong. But if that is ok or the desired behavior I'll go ahead and make the change and remove the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I think that's ok is because you could do OtherBasicMessage.new.to_hash and pass it down... the message will ignore any fields it doesn't know about so it seems ok to me. Thoughts?

fail TypeError, "Expected value of type '#{type_class}' for field #{name}, but got '#{val.class}'"
end

Expand Down
75 changes: 75 additions & 0 deletions spec/lib/protobuf/field/field_array_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
require 'spec_helper'

RSpec.describe Protobuf::Field::FieldArray do

class SomeBasicMessage < ::Protobuf::Message
optional :string, :field, 1
end

class MoreComplexMessage < SomeBasicMessage
end

class OtherBasicMessage < ::Protobuf::Message
optional :string, :other_field, 1
end

class SomeRepeatMessage < ::Protobuf::Message
optional :string, :some_string, 1
repeated :string, :multiple_strings, 2
repeated SomeBasicMessage, :multiple_basic_msgs, 3
end

let(:instance) { SomeRepeatMessage.new }

%w(<< push).each do |method|
describe "\##{method}" do
context 'when applied to a string field array' do
it 'adds a string' do
expect(instance.multiple_strings).to be_empty
instance.multiple_strings.send(method, 'string 1')
expect(instance.multiple_strings).to eq(['string 1'])
instance.multiple_strings.send(method, 'string 2')
expect(instance.multiple_strings).to eq(['string 1', 'string 2'])
end

it 'fails if not adding a string' do
expect { instance.multiple_strings.send(method, 100.0) }.to raise_error(TypeError)
end
end

context 'when applied to a MessageField field array' do
it 'adds a MessageField object' do
expect(instance.multiple_basic_msgs).to be_empty
basic_msg1 = SomeBasicMessage.new
instance.multiple_basic_msgs.send(method, basic_msg1)
expect(instance.multiple_basic_msgs).to eq([basic_msg1])
basic_msg2 = SomeBasicMessage.new
instance.multiple_basic_msgs.send(method, basic_msg2)
expect(instance.multiple_basic_msgs).to eq([basic_msg1, basic_msg2])
end

it 'adds a Hash from a MessageField object' do
expect(instance.multiple_basic_msgs).to be_empty
basic_msg1 = SomeBasicMessage.new
basic_msg1.field = 'my value'
instance.multiple_basic_msgs.send(method, basic_msg1.to_hash)
expect(instance.multiple_basic_msgs).to eq([basic_msg1])
end

it 'does not downcast a MessageField' do
expect(instance.multiple_basic_msgs).to be_empty
basic_msg1 = MoreComplexMessage.new
instance.multiple_basic_msgs.send(method, basic_msg1)
expect(instance.multiple_basic_msgs).to eq([basic_msg1])
expect(instance.multiple_basic_msgs.first).to be_a(MoreComplexMessage)
end

it 'fails if not adding the expected MessageField object' do
expect { instance.multiple_basic_msgs.send(method, 100.0) }.to raise_error(TypeError)
expect { instance.multiple_basic_msgs.send(method, OtherBasicMessage.new) }.to raise_error(TypeError)
end
end
end

end
end