collectiveidea / awesome_nested_set

An awesome replacement for acts_as_nested_set and better_nested_set.
MIT License
2.39k stars 492 forks source link

Nested set node move breaks after_save chain #21

Closed elliotcm closed 13 years ago

elliotcm commented 15 years ago

Hey dude,

We've been using your plugin recently and while it is super cool and has sped things up considerably, we encountered a couple of nasty bugs on the creation of our hierarchical objects.

Specifically, lines 516 and 570 of awesome_nested_set.rb in #move_to cause an ActiveRecord reload which in turn halts the after_save filter chain.

We haven't been able to come up with a proper solution, so the workarounds we've implemented involve a) always invoking acts_as_nested_set after any other chains and b) always saving the newly created object before assigning any has_many association objects (this seems to be because usage of the << operator on has_many associations of unsaved new objects causes an after_save to be set up to subsequently create the join table entries).

Not sure what you can do, but thought I should bring it to your attention.

Regards, Elliot Crosby-McCullough Eden Development

sumuhan commented 15 years ago

Thanks Elliot for posting this. I was struggling with the same problem and your workaround(moving the acts_as_nested_set below the call backs) worked like a charm! I hope a proper fix will be available soon!

bkeepers commented 15 years ago

If someone could post a test case that exposes the issue, it'd be appreciated.

elliotcm commented 15 years ago

Try as I might, I can't replicate this in a controlled environment and unfortunately the project I can replicate the issue on is not open source.

Sumuhan; can I ask what plugins you are using? What other filter chains do you have in place?

If there's some commonality maybe that can help us track it down.

sumuhan commented 15 years ago

I am not using any other pluggins apart from this. I am using 2.3.4 (By the way I am using Jruby 1.3.1/MySQL/Glassfish but that should not make any difference I think) I am using a before_destroy callback and returning false after a validation. That's when it got messed up. I moved the 'acts_as_nested_set' line below the callback and everything seems fine I am going to try and find a way to replicate this. Let's see if I get lucky

elliotcm commented 15 years ago

sumuhan,

If you'd like a leg-up, fork the test repository here http://github.com/elliotcm/nested_set_bug_demo

It has rspec tests in place (which annoyingly all pass) and is pinned using submodules to 2.3.4 and the most recent awesome_nested_set.

sumuhan commented 15 years ago

Hi guys, I am a ruby newbie so I am unable to debug this myself. I have tried to find a simple way to recreate this without any success. But I can recreate this anytime very easily in the application I am working on. It is not an open source application but I can zip it up and pass it on to Brandon with an sql dump of my database if that would be of any help. Brandon, would you like me to email this to you?

bkeepers commented 14 years ago

@sumuhan: sure, that'd be great. brandon@opensoul.org

ghost commented 14 years ago

The easiest way to replicate this failing is to use the new nested forms in rails 2.3.

here is the best example I can give: http://gist.github.com/237132

sumuhan commented 14 years ago

Hi Brandon, I have finally emailed the files as discussed. I am hoping I have packaged it properly but if you come across any problems please let me know.

Thanks

bkeepers commented 14 years ago

I am not able to duplicate this on Rails 2.3.4 with the latest eversion of awesome_nested_set. @nicholashubbard, I created a fresh rails app and that gist works fine. Please re-open this if you can create a test case that exposes the issue.

http://github.com/collectiveidea/awesome_nested_set/commit/4776b4101638e698f36b9e6b4063e56527c63b04

sumuhan commented 14 years ago

Hi Brandon, did you try with the code I sent you, I am using 2.3.4 as well, That should reproduce the error.

bkeepers commented 14 years ago

@sumuhan: you're issue is actually different than the after_save issue. If you want to stop destroy, then you'll just need to declare the before_destroy before calling acts_as_nested_set. This is just how Rails callbacks work: they are executed in the order they are declared, and awesome_nested_set is declaring a before_destroy callback that is getting called before yours.

sumuhan commented 14 years ago

Thanks for explain this brandon. And by the way, this plugin is truly awesome!

ghost commented 14 years ago

@brandon I was finally able to create a clean rails app that shows this problem.

http://github.com/nicholashubbard/awesome_nested_set_breaks

elliotcm commented 14 years ago

Nicholas! Good work there!

Using your sample project I isolated the problem to the call to clear_association_cache in ActiveRecord::Base#reload, which was erasing any record of the sub-objects.

I've forked the project and put in a fix, along with a pull request to Brandon.

Thanks for finally producing a demo, much appreciated.

collectiveidea commented 14 years ago

@elliotcm: thanks for the patch. But I still need a test case the proves this fixes it. Otherwise a future update could break it again. Can you reproduce it in the tests?

elliotcm commented 14 years ago

I'll see what I can do. It's reproduced in nicholashubbard's tests in his example, will see if I can convert something over for your suite.

ivanovv commented 14 years ago

I've got the same issue here, when used with vestal_versions and validation_reflection gems. elliotcm's fix helped me out.

parndt commented 13 years ago

Did anyone turn this into a test case? Should this issue still be open?