Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

handle cyclic associations #26

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

freng
Copy link

@freng freng commented Apr 3, 2013

cyclic associations lead to an infinite loop on dump, for instance

class Order < ActiveRecord::Base
  has_many    :states
  belongs_to  :last_state, :class_name => 'State'
end

class State < ActiveRecord::Base
  belongs_to  :order
end

I suggest two changes which I have implemented as a PoC:

  1. on dump: prevent following loops in the object association graph by saving and checking for objects already seen in the traversal
  2. on load: when a referenced object is missing, put the referrer on a wait list and reload it when the referee is loaded - otherwise there would be missing foreign keys

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)
Copy link
Owner

Choose a reason for hiding this comment

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

Passing the local_id here is going to break backward compatibility unfortunately. Existing programs that implement custom load_replicant method will raise an ArgumentError.

@rtomayko
Copy link
Owner

rtomayko commented Apr 3, 2013

Thanks for this. I've run into the problem a few times now.

I need to do a bit closer review of the diff and approach but I'm definitely interested in landing something that fixes this.

/cc @jbarnette

@robink
Copy link

robink commented Sep 25, 2013

Any update on this? why not just giving a default nil value to local_id?

@jjbohn
Copy link

jjbohn commented Jun 26, 2014

Anything new on this? I just cut a new fork where I've merged this into it, but would prefer to track the gem from the source.

@@ -16,6 +16,7 @@ class Loader < Emitter

def initialize
@keymap = Hash.new { |hash,k| hash[k] = {} }
@wait = Hash.new { |hash,k| hash[k] = [] }
Copy link

Choose a reason for hiding this comment

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

If this does go forward, this should be a Hash so non Integer ids don't error.

freng added 2 commits January 7, 2015 11:28
…ences objects, check for overridden load_replicant for backward compat
@freng
Copy link
Author

freng commented Jan 7, 2015

Trying to revive this, merged the rtomayko's current master branch and made two changes:

  • for backward compat when load_replicant overriden: check in load method the arity of the load_replicant method, if it's not -4, then we cannot reload (i.e. update the already loaded object when dependent foreign keys become available) since method is overriden, no more ArgumentError (of course there will be missing foreign keys then)
  • use of a wait list now instead of single waiting element since multiple objects could wait for the same dependent object

It would be nice if you could test dump & restore in (non-critical ,-) environments of your real world applications.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants