From ad4d8523f76364db5614243f7cd12ec66fa102f5 Mon Sep 17 00:00:00 2001 From: Adam Hutchison Date: Wed, 17 Feb 2016 20:12:50 -0700 Subject: [PATCH] Remove memoization on encode Changing values in repeated fields does not clear the memoized encoded value. This means that changes to repated fields after the memoized encoding has been computed will never be encoded. The pattern works for other field types because the memoized value is being cleared in the setter on the message before we call into the field. The field itself is not involved in clearing the memoized value at all. Repeated fields are a different story. If we were completely resetting a repeated field to a new array, the memoized encoding would be correctly cleared, but because the field array we are pushing into doesn't have any concept of a message, let alone the specific message instance the field belongs to, there is no way to clear the memoized encoded message. We need to remove the memoization until we can find a better approach. Fixes #304. --- lib/protobuf/field/base_field.rb | 2 - lib/protobuf/field/bytes_field.rb | 1 - lib/protobuf/field/enum_field.rb | 1 - lib/protobuf/field/message_field.rb | 1 - lib/protobuf/message/serialization.rb | 10 ++--- spec/lib/protobuf/message_spec.rb | 58 --------------------------- 6 files changed, 4 insertions(+), 69 deletions(-) diff --git a/lib/protobuf/field/base_field.rb b/lib/protobuf/field/base_field.rb index 3ae505a4..e50895ae 100644 --- a/lib/protobuf/field/base_field.rb +++ b/lib/protobuf/field/base_field.rb @@ -198,7 +198,6 @@ def define_array_setter message_class.class_eval do define_method(method_name) do |val| - @encode = nil if val.is_a?(Array) val = val.dup val.compact! @@ -240,7 +239,6 @@ def define_setter message_class.class_eval do define_method(method_name) do |val| - @encode = nil if val.nil? || (val.respond_to?(:empty?) && val.empty?) @values.delete(field.name) elsif field.acceptable?(val) diff --git a/lib/protobuf/field/bytes_field.rb b/lib/protobuf/field/bytes_field.rb index d7f4aee4..6c017558 100644 --- a/lib/protobuf/field/bytes_field.rb +++ b/lib/protobuf/field/bytes_field.rb @@ -62,7 +62,6 @@ def define_setter message_class.class_eval do define_method(method_name) do |val| - @encode = nil case val when String, Symbol @values[field.name] = "#{val}" diff --git a/lib/protobuf/field/enum_field.rb b/lib/protobuf/field/enum_field.rb index 71632cde..ac26ea0e 100644 --- a/lib/protobuf/field/enum_field.rb +++ b/lib/protobuf/field/enum_field.rb @@ -42,7 +42,6 @@ def define_setter field = self message_class.class_eval do define_method("#{field.name}=") do |value| - @encode = nil orig_value = value if value.nil? @values.delete(field.name) diff --git a/lib/protobuf/field/message_field.rb b/lib/protobuf/field/message_field.rb index 8cb37bd7..6e62cdf4 100644 --- a/lib/protobuf/field/message_field.rb +++ b/lib/protobuf/field/message_field.rb @@ -40,7 +40,6 @@ def define_setter field = self message_class.class_eval do define_method("#{field.name}=") do |val| - @encode = nil case when val.nil? @values.delete(field.name) diff --git a/lib/protobuf/message/serialization.rb b/lib/protobuf/message/serialization.rb index 668b6cea..6526a95c 100644 --- a/lib/protobuf/message/serialization.rb +++ b/lib/protobuf/message/serialization.rb @@ -48,12 +48,10 @@ def decode_from(stream) # Encode this message # def encode - @encode ||= begin - stream = ::StringIO.new - stream.set_encoding(::Protobuf::Field::BytesField::BYTES_ENCODING) - encode_to(stream) - stream.string - end + stream = ::StringIO.new + stream.set_encoding(::Protobuf::Field::BytesField::BYTES_ENCODING) + encode_to(stream) + stream.string end # Encode this message to the given stream. diff --git a/spec/lib/protobuf/message_spec.rb b/spec/lib/protobuf/message_spec.rb index 1ac3bff0..f4b345c8 100644 --- a/spec/lib/protobuf/message_spec.rb +++ b/spec/lib/protobuf/message_spec.rb @@ -268,64 +268,6 @@ end end - describe 'memoization' do - it "should memoize enum message" do - test_enum = Test::EnumTestMessage.new - test_enum.encode - expect(test_enum.instance_variable_get(:@encode)).to eq("") - test_enum.non_default_enum = 2 - expect(test_enum.instance_variable_get(:@encode)).to be_nil - end - - context "boolean fields" do - let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } } - let(:test_resource) { ::Test::Resource.new(values) } - - it "should memoize after bool values change " do - test_resource.encode - expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode) - test_resource.ext_is_searchable = false - expect(test_resource.instance_variable_get(:@encode)).to be_nil - end - end - - context "string" do - let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } } - let(:test_resource) { ::Test::Resource.new(values) } - - it "should memoize after bool values change " do - test_resource.encode - expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode) - test_resource.name = "MVP" - expect(test_resource.instance_variable_get(:@encode)).to be_nil - end - end - - context "string" do - let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } } - let(:test_resource) { ::Test::Resource.new(values) } - - it "should memoize after string values change " do - test_resource.encode - expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode) - test_resource.name = "MVP" - expect(test_resource.instance_variable_get(:@encode)).to be_nil - end - end - - context "Int64" do - let(:values) { { :name => "STEPH CURRY", :date_created => 1454712125 } } - let(:test_resource) { ::Test::Resource.new(values) } - - it "should memoize after Int64 values change " do - test_resource.encode - expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode) - test_resource.date_created = 5554712127 - expect(test_resource.instance_variable_get(:@encode)).to be_nil - end - end - end - context "when there's no value for a required field" do let(:message) { ::Test::ResourceWithRequiredField.new }