From 346da05c0d465589ef97d1d7c9051e35db2b9c8f Mon Sep 17 00:00:00 2001 From: freng Date: Wed, 27 Mar 2013 16:15:08 +0100 Subject: [PATCH 1/3] prevent infinite loops while dumping by checking objects seen so far --- lib/replicate/dumper.rb | 25 ++++++++++--- test/active_record_test.rb | 74 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/lib/replicate/dumper.rb b/lib/replicate/dumper.rb index a54598c..e922de9 100644 --- a/lib/replicate/dumper.rb +++ b/lib/replicate/dumper.rb @@ -22,6 +22,7 @@ class Dumper < Emitter # block - Dump context block. If given, the end of the block's execution # is assumed to be the end of the dump stream. def initialize(io=nil) + @seen = Hash.new { |hash,k| hash[k] = {} } @memo = Hash.new { |hash,k| hash[k] = {} } super() do marshal_to io if io @@ -76,7 +77,8 @@ def dump(*objects) end objects = objects[0] if objects.size == 1 && objects[0].respond_to?(:to_ary) objects.each do |object| - next if object.nil? || dumped?(object) + next if object.nil? || dumped?(object) || seen?(object) + see!(object) if object.respond_to?(:dump_replicant) args = [self] args << opts unless object.method(:dump_replicant).arity == 1 @@ -87,8 +89,8 @@ def dump(*objects) end end - # Check if object has been written yet. - def dumped?(object) + # type and id helper + def type_and_id(object) if object.respond_to?(:replicant_id) type, id = object.replicant_id elsif object.is_a?(Array) @@ -96,7 +98,21 @@ def dumped?(object) else return false end - @memo[type.to_s][id] + yield type, id + end + + # Check if object has been written yet. + def dumped?(object) + type_and_id(object) { |type,id| @memo[type.to_s][id] } + end + + # Check if object has been seen yet (needed for loop prevention) + def seen?(object) + type_and_id(object) { |type,id| @seen[type.to_s][id] } + end + + def see!(object) + type_and_id(object) { |type,id| @seen[type.to_s][id] = true } end # Called exactly once per unique type and id. Emits to all listeners. @@ -111,6 +127,7 @@ def write(type, id, attributes, object) type = type.to_s return if dumped?([type, id]) @memo[type][id] = true + @seen[type].delete(id) emit type, id, attributes, object end diff --git a/test/active_record_test.rb b/test/active_record_test.rb index ef9fb11..6012cda 100644 --- a/test/active_record_test.rb +++ b/test/active_record_test.rb @@ -53,6 +53,18 @@ t.integer "notable_id" t.string "notable_type" end + + create_table "orders", :force => true do |t| + t.string "name" + t.integer "last_state_id" + end + + create_table "states", :force => true do |t| + t.string "name" + t.datetime "created_at" + t.datetime "updated_at" + t.integer "order_id" + end end # models @@ -87,6 +99,15 @@ class Note < ActiveRecord::Base belongs_to :notable, :polymorphic => true end +class Order < ActiveRecord::Base + has_many :states + belongs_to :last_state, :class_name => 'State' +end + +class State < ActiveRecord::Base + belongs_to :order +end + # The test case loads some fixture data once and uses transaction rollback to # reset fixture state for each test's setup. class ActiveRecordTest < Test::Unit::TestCase @@ -186,6 +207,59 @@ def test_omit_dumping_of_association type, id, attrs, obj = objects.shift assert_equal 'User', type end + + def test_dump_and_load_correctly_despite_association_cycle + objects = [] + @dumper.listen { |type, id, attrs, obj| objects << [type, id, attrs, obj] } + + Order.replicate_associations :states + o = Order.create! :name => 'beer' + s1 = State.create! :name => 'ordered', :order => o + s2 = State.create! :name => 'paid', :order => o + o.last_state = s2 + o.save! + + @dumper.dump o + + assert_equal 3, objects.size + + # last state is resolved by Order belongs_to and is dumped first + type, id, attrs, obj = objects[0] + assert_equal 'State', type + assert_equal 2, attrs['id'] + assert_equal [:id, "Order", 1], attrs['order_id'] + + # next comes the order itself + type, id, attrs, obj = objects[1] + assert_equal 'Order', type + assert_equal 1, attrs['id'] + assert_equal [:id, "State", 2], attrs['last_state_id'] + + # and finally the has_many states, in this case just one left + type, id, attrs, obj = objects[2] + assert_equal 'State', type + assert_equal 1, attrs['id'] + assert_equal [:id, "Order", 1], attrs['order_id'] + + # destroy objects + State.delete_all + Order.delete_all + + assert_equal 0, Order.all.count + assert_equal 0, State.all.count + + assert_equal 3, objects.size + + # restore dump + objects.each { |type, id, attrs, obj| @loader.feed type, id, attrs } + + assert_equal 1, Order.all.count + assert_equal 2, State.all.count + # all states correctly linked to order? + assert_equal false, State.all.map {|s| s.order == Order.first}.include?(false) + # order linked to existing last_state? + assert_equal true, State.all.include?(Order.first.last_state) + end if ActiveRecord::VERSION::STRING[0, 3] > '2.2' def test_dump_and_load_non_standard_foreign_key_association From 1b469b8baa7c7040abb588736458f6c887dae503 Mon Sep 17 00:00:00 2001 From: freng Date: Wed, 27 Mar 2013 17:26:35 +0100 Subject: [PATCH 2/3] added wait list for unresolved remote_ids while loading, trigger object reload when resolving possible --- lib/replicate/active_record.rb | 8 +++++--- lib/replicate/loader.rb | 18 +++++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/replicate/active_record.rb b/lib/replicate/active_record.rb index 8a8172e..9fd3905 100644 --- a/lib/replicate/active_record.rb +++ b/lib/replicate/active_record.rb @@ -251,10 +251,11 @@ def replicate_omit_attributes=(attribute_names) # id - Primary key id of the record on the dump system. This must be # translated to the local system and stored in the keymap. # attrs - Hash of attributes to set on the new record. + # local_id - to reload an object with given local id # # Returns the ActiveRecord object instance for the new record. - def load_replicant(type, id, attributes) - instance = replicate_find_existing_record(attributes) || new + def load_replicant(type, id, attributes, local_id = nil) + instance = replicate_find_existing_record(attributes, local_id) || new create_or_update_replicant instance, attributes end @@ -262,7 +263,8 @@ def load_replicant(type, id, attributes) # values. # # Returns the existing record if found, nil otherwise. - def replicate_find_existing_record(attributes) + def replicate_find_existing_record(attributes, id = nil) + return find_by_id(id) if not id.nil? and not find_by_id(id).nil? return if replicate_natural_key.empty? conditions = {} replicate_natural_key.each do |attribute_name| diff --git a/lib/replicate/loader.rb b/lib/replicate/loader.rb index 050b778..ce25920 100644 --- a/lib/replicate/loader.rb +++ b/lib/replicate/loader.rb @@ -16,6 +16,7 @@ class Loader < Emitter def initialize @keymap = Hash.new { |hash,k| hash[k] = {} } + @wait = Hash.new { |hash,k| hash[k] = [] } @stats = Hash.new { |hash,k| hash[k] = 0 } super end @@ -67,13 +68,18 @@ def read(io) # id - Primary key id of the record on the dump system. This must be # translated to the local system and stored in the keymap. # attrs - Hash of attributes to set on the new record. + # local_id - to reload an object with given local id # # Returns the new object instance. - def load(type, id, attributes) + def load(type, id, attributes, local_id = nil) model_class = constantize(type) translate_ids type, id, attributes begin - new_id, instance = model_class.load_replicant(type, id, attributes) + if not local_id.nil? + new_id, instance = model_class.load_replicant(type, id, attributes, local_id) + else + new_id, instance = model_class.load_replicant(type, id, attributes) + end rescue => boom warn "error: loading #{type} #{id} #{boom.class} #{boom}" raise @@ -100,8 +106,9 @@ def translate_ids(type, id, attributes) if local_id = @keymap[referenced_type][remote_id] local_id else + @wait[referenced_type][remote_id] = [type, id, attributes.clone()] warn "warn: #{referenced_type}(#{remote_id}) not in keymap, " + - "referenced by #{type}(#{id})##{key}" + "referenced by #{type}(#{id})##{key}, added to wait hash" end end if value.is_a?(Array) @@ -121,6 +128,11 @@ def register_id(object, type, remote_id, local_id) @keymap[c.name][remote_id] = local_id c = c.superclass end + if not @wait[type][remote_id].nil? + waiting_type, waiting_id, waiting_attributes = @wait[type][remote_id] + @wait[type].delete(remote_id) + load(waiting_type, waiting_id, waiting_attributes, @keymap[waiting_type][waiting_id]) + end end # Turn a string into an object by traversing constants. Identical to From d14887c6b4965c4ea7d7e0ba261ba4c8111178eb Mon Sep 17 00:00:00 2001 From: freng Date: Wed, 7 Jan 2015 15:46:18 +0100 Subject: [PATCH 3/3] use waiting list instead of just the last element waiting for a references objects, check for overridden load_replicant for backward compat --- lib/replicate/loader.rb | 22 ++++++++++++---------- test/active_record_test.rb | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/lib/replicate/loader.rb b/lib/replicate/loader.rb index ce25920..18aed7e 100644 --- a/lib/replicate/loader.rb +++ b/lib/replicate/loader.rb @@ -16,7 +16,7 @@ class Loader < Emitter def initialize @keymap = Hash.new { |hash,k| hash[k] = {} } - @wait = Hash.new { |hash,k| hash[k] = [] } + @wait = Hash.new { |hash,k| hash[k] = Hash.new { |hash2,k2| hash2[k2] = [] } } @stats = Hash.new { |hash,k| hash[k] = 0 } super end @@ -73,13 +73,13 @@ def read(io) # Returns the new object instance. def load(type, id, attributes, local_id = nil) model_class = constantize(type) + if not local_id.nil? and model_class.method(:load_replicant).arity != -4 + warn "cannot reload #{type}(#{id}) since #{type}#load_replicant overriden and does not provide reload functionality" + return + end translate_ids type, id, attributes begin - if not local_id.nil? - new_id, instance = model_class.load_replicant(type, id, attributes, local_id) - else - new_id, instance = model_class.load_replicant(type, id, attributes) - end + new_id, instance = model_class.load_replicant(*([type, id, attributes, local_id].compact)) rescue => boom warn "error: loading #{type} #{id} #{boom.class} #{boom}" raise @@ -106,9 +106,9 @@ def translate_ids(type, id, attributes) if local_id = @keymap[referenced_type][remote_id] local_id else - @wait[referenced_type][remote_id] = [type, id, attributes.clone()] + @wait[referenced_type][remote_id] << [type, id, attributes.clone()] warn "warn: #{referenced_type}(#{remote_id}) not in keymap, " + - "referenced by #{type}(#{id})##{key}, added to wait hash" + "referenced by #{type}(#{id})##{key}, added to wait list" end end if value.is_a?(Array) @@ -129,9 +129,11 @@ def register_id(object, type, remote_id, local_id) c = c.superclass end if not @wait[type][remote_id].nil? - waiting_type, waiting_id, waiting_attributes = @wait[type][remote_id] + @wait[type][remote_id].each do |waiting_object| + waiting_type, waiting_id, waiting_attributes = waiting_object + load(waiting_type, waiting_id, waiting_attributes, @keymap[waiting_type][waiting_id]) + end @wait[type].delete(remote_id) - load(waiting_type, waiting_id, waiting_attributes, @keymap[waiting_type][waiting_id]) end end diff --git a/test/active_record_test.rb b/test/active_record_test.rb index 6bf7685..c7a38f9 100644 --- a/test/active_record_test.rb +++ b/test/active_record_test.rb @@ -227,7 +227,7 @@ def test_dump_and_load_correctly_despite_association_cycle o.save! @dumper.dump o - + assert_equal 3, objects.size # last state is resolved by Order belongs_to and is dumped first @@ -257,9 +257,12 @@ def test_dump_and_load_correctly_despite_association_cycle assert_equal 3, objects.size + # make more than one object to wait for referenced object + objects[1], objects[2] = objects[2], objects[1] + # restore dump objects.each { |type, id, attrs, obj| @loader.feed type, id, attrs } - + assert_equal 1, Order.all.count assert_equal 2, State.all.count # all states correctly linked to order? @@ -267,6 +270,36 @@ def test_dump_and_load_correctly_despite_association_cycle # order linked to existing last_state? assert_equal true, State.all.include?(Order.first.last_state) end + + def test_dump_and_load_with_overridden_load_replicant_method_if_association_cycle + objects = [] + @dumper.listen { |type, id, attrs, obj| objects << [type, id, attrs, obj] } + + (class << State; self end).class_eval do + define_method(:load_replicant) do |type, id, attrs| + super type, id, attrs + end + end + + Order.replicate_associations :states + o = Order.create! :name => 'beer' + s1 = State.create! :name => 'ordered', :order => o + s2 = State.create! :name => 'paid', :order => o + o.last_state = s2 + o.save! + + @dumper.dump o + + # destroy objects + State.delete_all + Order.delete_all + + # restore dump + objects.each { |type, id, attrs, obj| @loader.feed type, id, attrs } + + assert_equal 1, Order.all.count + assert_equal 2, State.all.count + end if ActiveRecord::VERSION::STRING[0, 3] > '2.2' def test_dump_and_load_non_standard_foreign_key_association