Skip to content

Added test for failing destroy on parent model destroying very related models anyway#110

Open
plindelauf wants to merge 2 commits into
rubysherpas:rails3from
plindelauf:rails3
Open

Added test for failing destroy on parent model destroying very related models anyway#110
plindelauf wants to merge 2 commits into
rubysherpas:rails3from
plindelauf:rails3

Conversation

@plindelauf

Copy link
Copy Markdown

See issue #108

@plindelauf

Copy link
Copy Markdown
Author

@radar did you have any chance to look at these pull requests and associated issues yet?

@radar

radar commented Mar 18, 2014

Copy link
Copy Markdown
Collaborator

@plindelauf Thanks for submitting that PR. I am not sure what is happening here... could you try explaining it again please?

@plindelauf

Copy link
Copy Markdown
Author

@radar Sure. A has many B's. A and B are both marked as "paranoid" and there's a :dependent => :destroy on the has_many relationship. So when I delete A, I expect all related B's to be deleted too. But not actually deleted, since they are all "paranoid". Unfortunately, the gem currently actually destroys all B's instead of marking them as deleted. Hence, the "paranoia" flag is not taken into account on B in this scenario.

@penguincoder

Copy link
Copy Markdown

I have been able to confirm this behavior exists in the wild. I have a very long association chain and (apparently) a inconsistent return value that does not always evaluate to true in an after_destroy callback. My chain is 5 models deep. Because of the complexity of the association, none of our normal tests were able to predict or catch this behavior until I started receiving unrelated bug reports that let me down a terribly long rabbit hole.

From what the Travis log says, @plindelauf has proved the bug exists. However, the only solution I can see to resolve this is to modify the behavior of the callbacks for paranoid models to never ever stop the callback chain, even when one returns false. This is contradictory to the design of the callbacks in general and is likely to open a new pandora's box of problems.

My interim solution is to modify all of my *_destroy callbacks to return true from every possible exit location. I usually have a sanity check at the top that now reads return true if... and a simple true on the last line of the method.

So @radar I am interesting in hearing if you have a good direction to go. I need to protect my data; I can offer my help fixing this ASASP.

@penguincoder

Copy link
Copy Markdown

Actually, I wonder if you could modify the callback behavior to simply raise an exception if you get a falsey value back from one of the destroy callbacks. That should sufficiently abort the current transaction and prevent actual deletion.

@radar

radar commented Oct 11, 2014

Copy link
Copy Markdown
Collaborator

@penguincoder Any luck on your investigating of this issue?

@penguincoder

Copy link
Copy Markdown

I haven't come up with any real solution, sorry.

@radar

radar commented Oct 19, 2014

Copy link
Copy Markdown
Collaborator

No worries, thought I would check anyway :)

@radar radar closed this Oct 19, 2014
@radar radar reopened this Oct 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants