#940 DataMapper <=0.10 does not recursively save parent resources - DataMapper

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

#940 ✓resolved

DataMapper <=0.10 does not recursively save parent resources

Reported by Ashley Moran | July 1st, 2009 @ 11:47 AM | in 0.10.0

DataMapper 0.10 now saves resource parents before resource children. This made me think you could save a whole object graph from any point. But current DataMapper behaviour makes it impossible to save complex resource hierarchies, at least in the presence of validations. The simplest failing case is this:

Child → Parent → GrandParent

where → means belongs_to.

When you save Child in this situation, it first attempts to save its parents - here, Parent. This then does not attempt to recursively save its parents, but just attempts to save itself. This fails because the GrandParent does not yet have primary key. The Child now fails to save, because the Parent has not saved. Note that without the GrandParent, both Child and Parent would have saved.

Here is proof of the current behaviour:

require 'rubygems'
  
USE_DM_0_9 = false
 
if USE_DM_0_9
  DM_GEMS_VERSION   = "0.9.11"
  DO_GEMS_VERSION   = "0.9.12"
else
  DM_GEMS_VERSION   = "0.10.0"
  DO_GEMS_VERSION   = "0.10.0"
end
 
gem "data_objects",   DO_GEMS_VERSION
# gem "do_postgres",    DO_GEMS_VERSION # If using another database, replace this
gem "do_sqlite3",     DO_GEMS_VERSION # If using another database, replace this
gem "dm-core",        DM_GEMS_VERSION   
gem "dm-types",       DM_GEMS_VERSION        
gem "dm-validations", DM_GEMS_VERSION  

require "data_objects"
require "do_postgres"
require "dm-core"
require "dm-aggregates"
require "dm-migrations"
require "dm-timestamps"
require "dm-types"
require "dm-validations"
require "dm-serializer"

require 'spec'
 
SQLITE_FILE = File.join(`pwd`.chomp, "test.db")
 
DataMapper.setup(:default, "sqlite3:#{SQLITE_FILE}")
DataMapper.setup(:reloaded, "sqlite3:#{SQLITE_FILE}")
 
class Parent
  include DataMapper::Resource

  property :id, Serial
  
  has n, :children
end
 
class Child
  include DataMapper::Resource

  property :id, Serial
  
  belongs_to :parent
  has n, :pets
end
 
class Pet
  include DataMapper::Resource

  property :id, Serial
  
  belongs_to :child
end
 
module IdentityMapHelper
  def reload(object)
    object.class.get(object.id)
  end
  
  def with_db_reconnect(&blk)
    original_repository = DataMapper::Repository.context.pop
    repository(:reloaded, &blk)
    DataMapper::Repository.context << original_repository
  end
end
 
Spec::Runner.configure do |config|
  include IdentityMapHelper
  
  config.before(:each) do
    DataMapper.auto_migrate!
  end
  
  config.before(:each) do    
    DataMapper::Repository.context << repository(:default)
  end
  
  config.after(:each) do
    DataMapper::Repository.context.pop
  end
end

describe "Accessing a subset of a collection using #[]" do
  before(:each) do
    @pet_1 = Pet.new
    @child_1 = Child.new
    @parent = Parent.new
    @child_2a = Child.new
    @child_2b = Child.new
    @pet_2ax = Pet.new
    @pet_2ay = Pet.new
    
    @parent.children << @child_1 << @child_2a << @child_2b
    @child_1.pets << @pet_1
    @child_2a.pets << @pet_2ax << @pet_2ay
  end
  
  it "should save a resource with grandparents" do
    @pet_1.save.should be_true
  end
  
  it "should save the whole object graph" do
    @pet_1.save
    [ @child_1, @parent, @child_2a, @pet_2ax, @pet_2ay, @child_2b ].each do |resource|
      resource.should_not be_new
    end
  end
end

Between me and a client, we isolated the issue between the two of us today, and produced a quick hack, which has been shown to work on object graphs many levels deep. There are two changes.

First to dm-core-0.10.0/lib/dm-core/resource.rb :

def save_parents
  parent_relationships.all? do |relationship|
    parent = relationship.get!(self)
    if parent.new? || parent.dirty?
      if parent.save
        relationship.set(self, parent)  # set the FK values
      end
    else
      relationship.set(self, parent)
    end
  end
end

Second to dm-validations-0.10.0/lib/dm-validations.rb :

chainable do
  def save(context = :default)
    return false unless save_parents && (context.nil? || valid?(context))
    save_self && save_children
  end
end

The changes are not very elegant, but they appear to work. While I can't say they don't introduce regressions, they do not appear to have broken any of the specs in our project.

Comments and changes to this ticket

Dan Kubb (dkubb) July 1st, 2009 @ 11:59 AM
- State changed from “new” to “accepted”
- Assigned user set to “Dan Kubb (dkubb)”
Dan Kubb (dkubb) July 1st, 2009 @ 12:12 PM
The only potential issue I can see with this is that it could cause an infinite loop, although I have not tested it.

Here's my theory: Since you're calling parent.save, what if it has a reference to the current resource as one of it's children? (this can happen if you're using an Identity Map) It'll call child.save (where child == the resource we originally saved), which will call parent.save again, looping.

What I was thinking was to change save_parents to something like:
```
    def save_parents
      parent_relationships.all? do |relationship|
        parent = relationship.get!(self)
        if parent.save_parents && parent.save_self
          relationship.set(self, parent)  # set the FK values
        end
      end
    end
```
This should walk up the stack to the top-most parent, save it and every other dependency in the correct order.

What do you think?
Dan Kubb (dkubb) July 1st, 2009 @ 04:47 PM
@Ashley: Yeah, you're right, although using parent.save rather than parent.save_parents and parent.save_self might have side effects of saving the entire object graph unintentionally, which is what I was trying to prevent.

When saving a child, we only want to save the parent because we need info from it in order to save the child. We want to do the bare minimal work necessary so that we can save a complete and valid child. We don't want that parent to also save all it's children, which could include other siblings. That should only happen with an explicit call to parent.save.

I've written a failing spec, and made it pass with the change above. I am currently running through some tests with dm-core and dm-more right now, and will push it once they are successful. Please let me know if it resolves your issue or not.
Dan Kubb (dkubb) July 1st, 2009 @ 04:52 PM
- State changed from “accepted” to “resolved”
(from [cb4301ef44b615bef37d9707663457fa1c10206d]) Save grandparents when saving a parent association

[#940 state:resolved] http://github.com/datamapper/dm-core/commit/cb4301ef44b615bef37d970...
Dirkjan Bussink July 7th, 2009 @ 02:27 AM
Personally I object the behavior of saving the whole tree at one point through a random call to just any Model#save. I work on a pretty complex app on a day to day basis and having it save the whole object graph is not an option for me. I often prepare a lot of dirty stuff and I would have it if it tries to persist everything at once.

It would kill performance for me and also often result in undesired behavior, since I don't call obj1.save for nothing if I don't want to save for example the dirty obj2 (which is related through some way, because in our app almost everything is related with each other through some way). The I continue to work with the data in the app and suddenly find that obj2 has been saved, while I perhaps didn't even want to!

I really value the principle of least surprise here and doing .save on some random object causing everything to be saved is definitely not something I would call expected behavior.

In my app some relations are from a developers standpoint much stronger than others. Some relations really make a composition out of different tables, others are far looser relations that are only used once in a while. Changing this behavior might work well for a composition, but it could created very weird behavior for in practice loosely related objects.
Dan Kubb (dkubb) July 19th, 2009 @ 02:59 AM
- State changed from “resolved” to “accepted”
@Ashley: On IRC I helped somebee work through a similar problem to yours, and I had him apply the fix in your last comment, and it resolved it.

Now that I've seen the problem myself, I think I can write a spec to reproduce it and will add your change in (assuming it doesn't break any other specs)

I can't think of too many ways to refactor that block without changing other parts of the code, but this is what I was thinking of adding:
```
def save_parents
  parent_relationships.all? do |relationship|
    parent = relationship.get!(self)

    if parent.new? || parent.dirty?
      next unless parent.save_parents && parent.save_self
    end

    relationship.set(self, parent)  # set the FK values
  end
end
```
If I expand things to change the structure of the code a bit, I might have a method that wraps up the concept of saving the parents + self in one call, and then I'd made save_parents and save_self private. I haven't investigated this yet, but I believe in every case where the parent is saved, self is saved too, and vice versa. If that turns out to be true I can probably cut down the complexity of the above method a bit.
Dan Kubb (dkubb) July 19th, 2009 @ 03:03 AM
Attached is a script which demonstrates the problem somebee was having, and which Ashley's code resolved.
- ticket-940.rb 466 Bytes
Dan Kubb (dkubb) July 19th, 2009 @ 09:58 AM
@Ashley: I created a ticket in Lighthouse's support forum for this:

http://help.lighthouseapp.com/discussions/problems/910-error-access...
Rick July 20th, 2009 @ 11:41 PM
Hey everyone, sorry for the hassle. We're running into this odd issue where S3 appears to work, but somehow loses the file. We're looking into it (it's not raising any errors in the aws-s3 gem and appears to work at the time of upload). I re-uploaded the above file, so it should work now.
Dan Kubb (dkubb) August 14th, 2009 @ 05:23 PM
- State changed from “accepted” to “resolved”
(from [65ef270d01702af9d5e922a716676a13c4b16976]) Fix recursion problem with saving a Resource when a child is also a parent

[#940 state:resolved] http://github.com/datamapper/dm-core/commit/65ef270d01702af9d5e922a...
Dan Kubb (dkubb) August 15th, 2009 @ 07:20 PM
Yeah, I've been a bit lax on the RCs due to alot of other things going on in real-life. Going to do one more, and then release 0.10.0 and start releasing more frequently. I think a bunch of smaller/faster releases would be better than holding back until every single issue is resolved, provided we publish the list of known issues somewhere so people know up-front what the problems are.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.