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

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 »

This project is archived and is in readonly mode.

#6666 open

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

You flagged this item as spam.
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.
You flagged this item as spam.
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.
You flagged this item as spam.
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 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 April 4th, 2011 @ 05:04 AM
It seems miloops has a fix for Identity mapping. https://github.com/rails/rails/pull/251/files
You flagged this item as spam.
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 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
```
You flagged this item as spam.
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 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.
You flagged this item as spam.
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 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.

You flagged this item as spam.
Alexey Muranov April 19th, 2011 @ 09:22 AM
- Tag set to “activerecord rails3”
You flagged this item as spam.
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.