Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
lighthouse-import opened this Issue May 16, 2011 · 17 comments

Comments

Projects
None yet
5 participants

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6666
Created by Alexey Muranov - 2011-04-04 19:14:57 UTC

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.

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-03 11:43:35 UTC

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.

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-03 11:56:29 UTC

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.

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-03 14:17:04 UTC

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

Imported from Lighthouse.
Comment by Neeraj Singh - 2011-04-04 03:33:41 UTC

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

Imported from Lighthouse.
Comment by Neeraj Singh - 2011-04-04 04:04:36 UTC

It seems miloops has a fix for Identity mapping. https://github.com/rails/rails/pull/251/files

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-04 07:37:20 UTC

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

Imported from Lighthouse.
Comment by Neeraj Singh - 2011-04-04 12:28:43 UTC

Fixed in b356172 .

Make sure that in console first you do

ActiveRecord::IdentityMap.enabled = true

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-04 12:36:14 UTC

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.

Imported from Lighthouse.
Comment by Neeraj Singh - 2011-04-04 17:06:17 UTC

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.

Imported from Lighthouse.
Comment by Alexey Muranov - 2011-04-04 17:27:50 UTC

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?

Imported from Lighthouse.
Comment by Neeraj Singh - 2011-04-04 19:14:57 UTC

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

Imported from Lighthouse.
Comment by Wicked Tribe - 2011-04-20 17:20:51 UTC

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.

Contributor

alexeymuranov commented Jun 30, 2011

The issue was apparently closed automatically when migrated from Lighthouse, shouldn't it be re-open? I do not have permissions to reopen it myself here.

@spastorino spastorino reopened this Jul 2, 2011

Member

NZKoz commented Aug 28, 2011

Active Record has no way of knowing that the record has been deleted in the background. If you're dealing with a model which is likely to be edited or destroyed behind the scenes you'll need to use Base#lock, :lock=>true on find and disable partial updates.

There are dozens of other edge cases which partial updates can cause, addressing this one particular one isn't worth it imo

@NZKoz NZKoz closed this Aug 28, 2011

Contributor

alexeymuranov commented Aug 29, 2011

Why cannot Active Record look into the database? There should be a way to save a record from the memory regardless of the database state, an i think save would be a good name for such method.

Contributor

alexeymuranov commented Aug 30, 2011

At least save should return false in this case.

is this still not fixed? I get the same behaviour as of Rails 5 beta 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment