This project is archived and is in readonly mode.

#6666 open
Alexey Muranov

save (ActiveRecord::Persistence) can return true even when the model is not saved, and seems to break the principle of "least surprise"

Reported by Alexey Muranov | April 3rd, 2011 @ 12:35 PM

These two examples illustrate the unexpected (to me) behavior of save method in ActiveRecord::Persistence for a sample application created with

$ rails new test_app
$ cd test_app
$ rails generate model Person name:string
$ rake db:migrate

First example. In console:

p = Person.create(:name=>'Bill')
p.destroy
p.name  # => "Bill"
p.save  # => true

Despite the true returned by save, the database is empty in the end.

Second example. In console:

p = Person.create(:name=>'Bill')
Person.find(p.id).destroy
p.persisted?     # => true
p.destroyed?     # => false
p.name = "John"  # => "John"
p.save           # => true

Again, despite the true returned by save, the database is empty in the end.

This problem was discussed by me and others here: http://www.ruby-forum.com/topic/1386349

I saw other tickets about unexpected behavior of save, but didn't find an exact match to this problem.

Comments and changes to this ticket

  • Alexey Muranov

    Alexey Muranov April 3rd, 2011 @ 12:43 PM

    I am new to Rails, i am only starting my first Rails application, so please feel free to express your opinions about this problem, or how it should be dealt with. I know that i am supposed to work on it myself if i want it to be fixed, but i am not ready for that, i am just interested in knowing if others consider this a bug too.

  • Alexey Muranov

    Alexey Muranov April 3rd, 2011 @ 12:56 PM

    I want however to state what behavior would look to me natural and "least surprising", or how i would have changed this part of ActiveRecord::Persistence if there were no need to keep compatibility with already existing applications:

    1. implement insert and update methods to simply execute SQL and return true or false accordingly,
    2. switch @persisted to false when the record is destroyed or deleted or when id=() method is called,
    3. remove destroyed? method and @destroyed instance variable,
    4. implement save so that it would only return true if the model/record has been saved.
  • Alexey Muranov

    Alexey Muranov April 3rd, 2011 @ 03:17 PM

    In fact, i think that if in item (3) above destroyed? is not removed but only deprecated, than the suggested changes should not break most of applications...

  • Neeraj Singh

    Neeraj Singh April 4th, 2011 @ 04:33 AM

    • Importance changed from “” to “Low”

    Following code is causing problem

    p = Person.create(:name=>'Bill')
    Person.find(p.id).destroy
    p.persisted?     # => true
    p.destroyed?     # => false
    p.name = "John"  # => "John"
    p.save           # => true
    

    However if you try following code then you will get "TypeError: can't modify frozen hash".

    p = Person.create(:name=>'Bill')
    p2 = Person.find(p.id)
    p2.destroy
    p2.persisted?     
    p2.destroyed?     
    p2.name = "John"  
    p2.save
    

    In the first example author is manipulating record 'P' which is still not frozen.

    I guess a good fix for this problem would be to make sure that p2 is same as p. I turned on the identity mapping and still p2.object_id != p.object_id .

    I did not follow very closely how identity mapping is implemented. I guess it is time to see how it is done. I'm sure there is a reason why

    p1 = User.create
    p2 = User.find(p1.id)
    p3 = User.last
    
    puts p1.object_id == p2.object_id #=> false
    puts p1.object_id == p3.object_id #=> false
    puts p2.object_id == p3.object_id #=> false
    
  • Neeraj Singh
  • Alexey Muranov

    Alexey Muranov April 4th, 2011 @ 08:37 AM

    The main issue here that i consider a bug is that save returns true despite not saving.

  • Neeraj Singh

    Neeraj Singh April 4th, 2011 @ 01:28 PM

    • State changed from “new” to “resolved”

    Fixed in https://github.com/rails/rails/commit/b35617235d43bdb32016a623044e7... .

    Make sure that in console first you do

    ActiveRecord::IdentityMap.enabled = true
    
  • Alexey Muranov

    Alexey Muranov April 4th, 2011 @ 01:36 PM

    This does not resolve the issue that save returns true without saving in the above examples, or if the record has been deleted in the background by another application.

  • Neeraj Singh

    Neeraj Singh April 4th, 2011 @ 06:06 PM

    Before hitting save you are doing

    p.name = "John"
    

    Now that line is throwing exception since the object is frozen. So you don't even get to save method.

  • Alexey Muranov

    Alexey Muranov April 4th, 2011 @ 06:27 PM

    Ok, i see your point. However, if the record is deleted in the background by another process, save is going to return true without saving anything. Is this a desired behavior?

  • Neeraj Singh

    Neeraj Singh April 4th, 2011 @ 08:14 PM

    • State changed from “resolved” to “open”

    @Alexey. Now I see your point.

    class User < ActiveRecord::Base
      def self.lab
        u1 = User.create
        User.delete_all
        u1.name = 'foo'
        puts u1.save #=> true
    
        u2 = User.create
        User.delete_all
        u2.name='foo1'
        puts u2.save! #=> true
      end
    end
    

    I am marking this ticket as open so that more people can look at it. I will look into this.

  • Alexey Muranov

    Alexey Muranov April 19th, 2011 @ 09:22 AM

    • Tag set to activerecord rails3
  • Wicked Tribe

    Wicked Tribe April 20th, 2011 @ 06:20 PM

    There really seems to be two issues here. The first would be how active record instance behaves on a save when it knows the record has been deleted. That I believe would be the easiest to solve. The second is how or should an active record instance attempt to determine if its record has been deleted without its knowledge.

    The complexity on the second issue revolves around when and if the database should be checked to determine if a record still exists. If a response from the database during an update can be used to determine that a record is no longer there, the instance can then execute the code that would solve the first issue. If not, then things get ugly.

    Additionally there is the issue of what should happen when no attributes have been changed, a second matching instance has deleted the record, and then the first has been saved. Rails will desire to skip an actual call to the database as this would normally be an performance improvement by not having the db update the record with identical values. Having rails make an extra call in this situation just to see if the record still exists would work against that optimization.

    For now I would suggest looking to solve the first problem as the issues with the second are non trivial.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages