Datamapper DataMapper

Comments and changes to this ticket

• You flagged this item as spam.
  

  tedkimble July 21st, 2010 @ 02:41 AM

  Another issue that seems to be related, is that no hooks for :save, :update, or :destroy will be called unless a DataMapper property has been modified. Take the following simple user model as an example:

  class User
    include DataMapper::Resource
  
    property :id, Serial
    property :name, String
    property :password_hash, String, :writer => :private
    attr_accessor :password
  
    before :save, :encrypt_password
  
    private
  
    def encrypt_password
      # this would normally do something worthwhile, but as an example...
      self.password_hash = @password unless @password.blank?
    end
  end
  

  If a DataMapper property is modified, all is well:
  

  > user = User.new
  > user.name = "ted"
  > user.password = "password"
  > user.save
  true
  <User @id=1, @name="ted", @password_hash="password">
  

  But what if I don't want to modify anything but my password (and therefore password_hash)?
  

  > user = User.last
  <User @id=1, @name="ted", @password_hash="password">
  > user.password = "new_password"
  > user.save
  true
  <User @id=1, @name="ted", @password_hash="password">
  

  The hook was never called because no DataMapper properties were changed before the validations were run. So let's change one of those properties:
  

  > user = User.last
  <User @id=1, @name="ted", @password_hash="password">
  > user.name = "teddy"
  > user.password = "new_password"
  > user.save
  true
  <User @id=1, @name="teddy", @password_hash="new_password">
  

  This time a property was changed, so the hook was called. But if I don't want to change the name, I'm stuck.

  Maybe this is the desired behavior, maybe not. But this seems like a common setup; I see this all of the time in ActiveRecord.

  Is this setup really incompatible with DataMapper? I'm new to DataMapper and am loving it so far, but this seems like a deal breaker. Otherwise, is there a better way to deal with attr_accessors in DM?

  Ted

• You flagged this item as spam.
  

  tedkimble July 21st, 2010 @ 05:22 AM

  After scouring the source code, it seems like modifying non-DataMapper properties (ie attr_accessors) has no effect on the resource's persisted_state, or persisted_state.original_attributes.

  This means that resource.dirty_self? will return false and the save (update, destroy) chain will be halted and no hooks will be called. As a hack, you could do the following:

  # let's say we have a attr_accessor :password that we want to "monitor"
  # we need to modify it's setter method
  
  def password=(password)
  
    # don't do anything if the resource is already dirty
    if persisted_state.class == DataMapper::Resource::State::Clean
  
      # if it's clean, make it dirty
      self.persisted_state = DataMapper::Resource::State::Dirty.new(self)
  
      # now that it responds to original_attributes,
      # (Clean version does not) we can set an ugly hack
      # it appears that on a save these attributes are just wiped,
      # so I don't see how this would cause any other errors
      self.persisted_state.original_attributes[:hack] = true
    end
    @password = password
  end
  

  Now whenever password is set, the resource becomes "dirty" and all hooks will be called on save, update, destroy, etc....

  Is there a better way, or am I going crazy at this hour in the night (or morning, i guess)?

• You flagged this item as spam.
  

  Kevin Watt July 21st, 2010 @ 10:41 PM

  Are you running this in the rails console? It seems to have problems with datamapper...

• You flagged this item as spam.
  

  tedkimble July 21st, 2010 @ 11:17 PM

  I was originally noticing this from within a Sinatra app. I then went to the console using racksh.

  Having spent some time looking through the source, this isn't an "error" in DataMapper, it's just the way it is. A model's .persisted_state will always be of kind DataMapper::Resource::State::Clean unless you modify a property that was defined as "property :something, etc...".

  And if the .persisted_state is Clean, no callbacks (and thus I believe validations) will ever be triggered. The only hack I can find is the one I listed above.

  The alternative to this, would be to set a class method that tells DataMapper to run the save chain no matter what. So the default would remain the same, but in my case I could tell DataMapper to always validate and run hooks when I run the methods :save, :update, or :delete.

  class TestDM
    include DataMapper
  
    property :id, Serial
    attr_accessor :something
  
    always_validate true
  

  Or maybe adding an optional param to the save method
  

  my_model.save(:validate => true)
  my_model.save(:force => true)
  my_model.save(true)
  

• 

  Tony Pitale September 24th, 2010 @ 10:48 PM

  • State changed from “new” to “confirmed”

  +1

  Here's a runnable script demonstrating some of the problems: http://gist.github.com/445456

• 

  Tony Pitale September 24th, 2010 @ 11:19 PM

  I have no issue with the dirtiness tracking short-circuting the persistence. The problem is the hooks. If I have a hook that updates a property using accessors it should be run, thus dirtying the resource. This may also be a problem with dm-validations and valid? not being run before save (or so it seems).

  save_self in dm-validations should not be concerned with dirtiness, perhaps

• 

  Tony Pitale September 24th, 2010 @ 11:30 PM

  I've made some changes that move dirtiness checks into create_with_hooks and update_with_hooks, as well as save_self but I sincerely doubt that it'll pass the specs. I suppose I'll have to write a failing spec and finally get the specs running on my machine.

• You flagged this item as spam.
  

  Kabari December 18th, 2010 @ 10:56 AM

  [PATCH] https://gist.github.com/746653

  That patch makes before hooks run on a non dirty resource. I figure because there are many before hooks that would change the state of a resource, just like we've seen in the examples. Then checks dirty? to decide whether or not to save the resource.

• You flagged this item as spam.
  

  Kabari December 18th, 2010 @ 03:55 PM

  • Assigned user set to “Martin Gamsjaeger (snusnu)”
• 

  Dan Kubb (dkubb) December 22nd, 2010 @ 12:10 AM

  It's great to see this discussion in the thread. @snusnu pointed me here and asked me to comment. I'll start with tedkimble's original post and work downwards.

  @tedkimble: In general, unless there's something stepping in the way, I would expect the return value of test.save to always be equal to test.valid?. If something is valid it should be saved. It would only be in rare cases where a database might go down between the validation check and the actual persistence when this might fail, otherwise it should always succeed if the object is valid. So what you're seeing is a bug.

  @Tony: Thanks for making a script. These go a long way when trying to reproduce a problem; sometimes I might run such a script a hundred times when experimenting and spiking a solution. I often even use them as a basis for the specs when I write the real fix afterwards.

  @Kabari: This is awesome. I presume by the presence of specs that this passes all the specs in dm-core? What about the other dm gems? Usually I try to get something passing with dm-core, and then I run all the other gems against it to make sure I haven't accidentally broken something else that relies on the behaviour. Don't worry if you've not done this yet; I will likely do it before I push anything to the repo. If you ever want to drop into #datamapper on IRC (freenode.net) I or snusnu can walk you through setting this up.

  My only reservation is that this does change some behaviour that people could be relying on. When a change like this is made, we have to be really careful we do it not just to fix a localized problem, but because it's the right thing overall.

  In the beginning hooks were this kind of thing we thought would be cool to bind to method execution. We created this fancy hook lib in Extlib that wraps a method with before/after procs that are executed before and after the original method body. This worked out ok, but we started to see issues when we refactored things (since it was based on a method name, a method that may not necessarily be running the actual logic). Plus there were cases with executing the hooks in the correct order when you're saving an object graph. All the issues get quite crazy complex and there are still some issues surrounding them I haven't been able to fix, despite taking 3 months off last year to focus on them full-time.

  One of the key things that the hook system has morphed into is an event-based hook system, rather than a per-method hook, which I admit now was elegant, and completely wrong. The idea with what we have now is: "an event is received by the system and we run the before hook, handle the event, then run the after hook". If something happens prior to the event that makes it so it never executes, then it's hooks don't execute either; they are coupled. For consistency, if a before hook is executed then the event is handled and the after hook is always executed; and the only thing that can stop an event from being handled is if the before hook uses throw(:halt) I believe.

  I'm not saying that the current approach is right, and the proposed one is wrong. It could most certainly be the case that the current approach is flawed. We just have to be absolutely certain that whichever approach we choose is correct, and we need to have it be consistent across the board.

  It does seem a bit strange to me to have the hooks fire when the event may or may not ever fire .. I wonder if perhaps things like this specific case could be handled by a before(:valid?) hook? (BTW I hate that syntax, I want to alias it to the label ":validation" and then say that you must modify internal state before validation if you want to persist something untracked)

• You flagged this item as spam.
  

  Kabari December 22nd, 2010 @ 10:16 AM

  @dkubb I did not test it against the other gems. I can do that today though.

  The idea with what we have now is: "an event is received by the system and we run the before hook, handle the event, then run the after hook". If something happens prior to the event that makes it so it never executes, then it's hooks don't execute either; they are coupled.

  My thinking is that you want a before hook to execute before a method is called, regardless of the method result, and an after hook to execute only when a method runs successfully. However, the way I interpret successfully is different with regard to persistence because those hooks are managed internally by dm-core, and are not like the user doing before :random_method. I figure to the user, a successful :save means the object has persisted, but since I prevent that step I also prevent the after callbacks from running.

  The same should go for any persisted state I guess.

• 

  Dan Kubb (dkubb) December 24th, 2010 @ 12:17 AM

  @kabari: How did your testing go?

  I think you're correct if you were writing something that actually hooked the methods, but this is hooking an event named "save" not the "save" method. It's incidental that it shares the same name as the method; there are lots of other methods that also result in a "save" event that do not necessarily use Resource#save at all.

  I guess the question comes down to: does it make sense for a hook on an event to fire if up until that point the event was never going to be fired at all?

  If you look at every library that provides a way to do callbacks for certain events you will never see this behaviour. I think that's why I keep coming back to question it. It breaks most people's mental model of how callbacks are normally fired.

  Now this doesn't mean the current behaviour in DM isn't broken, it just means the proposed solutions would just be broken in a different way. We need to come up with something that is both correct and follows the principle of least surprised (POLS). It must match people's mental model of how things normally work.

• You flagged this item as spam.
  

  Kabari December 24th, 2010 @ 11:03 AM

  @dkubb It seems like it works, but I only tested against dm-more plugins. My dm-dev is kind of jagged though, so I recommend getting a second opinion :)

  The way it was before, if you called #save it would return without running any hooks at all unless the resource was #dirty?, which was definitely not expected since it didn't even start the callback chain. I think what people expect to happen is the way it worked when Extlib::Hook was included, which is what I intended.

  This is also how ActiveRecord (ActiveSupport::Callbacks) does it:

  def update(*) #:nodoc:
    _run_update_callbacks { super }
  end
  

  I do agree that this patch needs to be redone, since I did not notice that #update_with_hooks calls #before_save_hooks and #before_update_hooks. Maybe it should just skip the #_persist part if the record isn't dirty, rather than stop the chain like it does now.

• You flagged this item as spam.
  

  Kabari December 26th, 2010 @ 06:24 PM

  Randomly I just found another reference to this issue, view the code: http://rdoc.info/gems/carrierwave/0.5.1/CarrierWave/DataMapper:moun...