save method not catching validations on attr_accessors

[solnic]

It seems as though :save will not catch any validations errors if no DM properties were modified, but :valid? will. Is this the correct behavior? For example:

class TestDM
  include DataMapper::Resource

  property :id, Serial
  property :name
  attr_accessor :password

  validates_with_block do
    @password.blank? || @password.length >= 8 ? true : [false, "Password must be at least 8 characters"]
  end
end

Create a new TestDM:

> test = TestDM.new
> test.name = "ted"
> test.save
true

Find that last TestDM and add an invalid password (too short) and save. It should be false, but it returns true instead.

> test = TestDM.last
> test.password = "asdf"
> test.save
true

Do the same thing, but check valid? instead of save. This works as expected.

> test = TestDM.last
> test.password = "asdf"
> test.valid?
false

Do the same thing, but change any DM property and save. This works as expected.

> test = TestDM.last
> test.password = "asdf"
> test.name = "teddy"
> test.save
false

The implications of this are that all of my controller methods need to do:

if @test.valid?
  @test.save
  ...
else
  ...
end

which is not as nice as you would normally do it. Thoughts?

---

Created by tedkimble - 2010-12-24 06:17:20 UTC

Original Lighthouse ticket: http://datamapper.lighthouseapp.com/projects/20609/tickets/1372

[solnic]

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

by tedkimble

[solnic]

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)?

by tedkimble

[solnic]

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

by Kevin Watt

[solnic]

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)

by tedkimble

[solnic]

+1

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

by Tony Pitale

[solnic]

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

by Tony Pitale

[solnic]

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.

by Tony Pitale

[solnic]

[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.

by Kabari

[solnic]

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)

by Dan Kubb (dkubb)

[solnic]

@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.

by Kabari

[solnic]

@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.

by Dan Kubb (dkubb)

[solnic]

@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.

by Kabari

[solnic]

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

by Kabari

[probablykabari]

I came up with a solution to this after that thread between dkubb and I, but not sure if it's the right way or where to actually put the code. I'll start a thread on google.