#5058 destroy with optimistic locking doesn't set destroyed? flag - Ruby on Rails

Svelix

Confirmed.

July 12th, 2010 @ 12:47 PM

Neeraj Singh

Milestone set to 3.x
State changed from new to open
Importance changed from to Low

@Jacob thanks for the patch and the test. It is a good patch however I would recommend following changes

as per your patch if a new record is destroyed then destroyed flag would be set to true. That should not happen. Please add a test so that Person.new.destroy.destroyed? returns false
change
```
assert_equal true, p1.destroyed?
```
to
```
assert p1.destroyed?
```
. I know you just followed what was there a line above. If you want you can fix that line too.

July 12th, 2010 @ 01:32 PM

jlewalle (at gmail)

No problem... about the destroy of a new record, that doesn't appear to be the behavior of the non-locking destroy:

def destroy
  if persisted?
    self.class.unscoped.where(self.class.arel_table[self.class.primary_key].eq(id)).delete_all
  end

  @destroyed = true
  freeze
end

Just wanna make sure I understand things.

I'll change the asserts.. thanks for the feedback!

July 12th, 2010 @ 08:00 PM

José Valim

Assigned user set to José Valim

Hey guys,

Actually, if destroy is called on a record which was not persisted? (a new_record?), it should raise an error. I believe Neeraj is already working on a patch for it. :)

July 12th, 2010 @ 09:00 PM

Neeraj Singh

I will create a new ticket and will attach patch after this one is committed.

July 12th, 2010 @ 09:04 PM

jlewalle (at gmail)

Awesome, let me know if you need anything else from me. Here's the patch with the cleaner assert's.

optimistic-locking-destroyed-flag-2.patch 1.1 KB

July 12th, 2010 @ 09:09 PM

José Valim

Jacob, your patch does not apply here. Git complains the format is invalid. Could you please follow the conventions here?

http://rails.lighthouseapp.com/projects/8994/sending-patches

Thanks!

July 13th, 2010 @ 07:34 AM

jlewalle (at gmail)

Ah ok... I verified this one works. Thanks for your patience. I've also got a github fork here:

http://github.com/jlewallen/rails

optimistic-locking-destroyed-flag-3.patch 1.32 KB

July 13th, 2010 @ 07:46 AM

Neeraj Singh

I am still not able to apply the *-flag3.patch.

However this commit http://github.com/jlewallen/rails/commit/c7d2a50309b4a74ffc3564c4b5... can be pulled .

July 14th, 2010 @ 03:23 AM

jlewalle (at gmail)

Ah... well.. still weird the patch wasn't working. I'm able to apply with:

git am < optimistic-locking-destroyed-flag-3.patch

What's the error you see?

July 14th, 2010 @ 03:28 AM

Neeraj Singh

Applying: Set destroyed=true in opt locking's destroy
error: activerecord/lib/active_record/locking/optimistic.rb: does not match index
error: activerecord/test/cases/locking_test.rb: does not match index
Patch failed at 0001 Set destroyed=true in opt locking's destroy
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

July 14th, 2010 @ 03:31 AM

jlewalle (at gmail)

Can you give me the sha1 of the branch you're applying to?

July 14th, 2010 @ 03:40 AM

Repository