Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: rails3
Choose a base branch
from

Conversation

plindelauf
Copy link

See issue #108

@plindelauf
Copy link
Author

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

@radar
Copy link
Collaborator

radar commented Mar 18, 2014

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

@plindelauf
Copy link
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

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

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
Copy link
Collaborator

radar commented Oct 11, 2014

@penguincoder Any luck on your investigating of this issue?

@penguincoder
Copy link

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

@radar
Copy link
Collaborator

radar commented Oct 19, 2014

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.

None yet

3 participants