This project is archived and is in readonly mode.

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

#6666

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.

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

Activity

Alexey Muranov
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.

April 3rd, 2011 @ 12:43 PM
Alexey Muranov
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.
April 3rd, 2011 @ 12:56 PM
Alexey Muranov
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...

April 3rd, 2011 @ 03:17 PM
Neeraj Singh
- 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
```
April 4th, 2011 @ 04:33 AM
Neeraj Singh
It seems miloops has a fix for Identity mapping. https://github.com/rails/rails/pull/251/files

April 4th, 2011 @ 05:04 AM
Alexey Muranov
The main issue here that i consider a bug is that save returns true despite not saving.

April 4th, 2011 @ 08:37 AM
Neeraj Singh
- 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
```
April 4th, 2011 @ 01:28 PM
Alexey Muranov
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.

April 4th, 2011 @ 01:36 PM
Neeraj Singh
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.
April 4th, 2011 @ 06:06 PM
Alexey Muranov
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?

April 4th, 2011 @ 06:27 PM

Neeraj Singh

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.

April 4th, 2011 @ 08:14 PM

Alexey Muranov
- Tag set to activerecord rails3
April 19th, 2011 @ 09:22 AM
Wicked Tribe
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.

April 20th, 2011 @ 06:20 PM