#2627 has_many :dependent => :delete_all throws exception on models in modules - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2627 ✓committed

has_many :dependent => :delete_all throws exception on models in modules

Reported by Brian Samson | May 9th, 2009 @ 07:03 AM | in 2.3.6

If a model that is declared inside a module is associated via a has_many to another model in the same module, and :dependent is set to :delete_all or :nullify, an exception is thrown stating that "(Module) is not missing Constant (class)"

This is because the method that sets up the before_destroy handler just uses a text representation of the model name instead of the actual model class. The example I've been using is this:

class Astronomy::SolarSystem < ActiveRecord::Base
  has_many :planets, :dependent => :delete_all
end

The before destroy handler ends up making a call to Planet.delete_all instead of Astronomy::Planet.delete_all. I am attaching a patch with a test case that show this (and a similar flaw with :dependent => :nullify) and a solution to the problem.

I discovered this bug in rails 2.1 and it is still present in edge rails. The patch is meant to be applied to edge.

As an aside, I don't really like my particular solution because of its reliance on 'eval.' I think it would be a better idea to change the dependent_conditions to be parameterized instead of a string that needs to be double-evaled, but I'm not sure that will play well will every database backend. If it's ok to change that conditions clause to a parameterized query, let me know and I'll update the patch.

has_many_delete_all_in_modules.patch 9.8 KB

Comments and changes to this ticket

You flagged this item as spam.
Geoff Coffey May 11th, 2009 @ 05:49 PM
+1
CancelProfileIsBroken August 6th, 2009 @ 02:34 PM
- Tag changed from “active_record, dependent, has_many, module” to “active_record, bugmash, dependent, has_many, module”
Dan Pickett August 8th, 2009 @ 11:21 PM
+1 verified

this applies cleanly to master with a few whitespace warnings

test suite passes

note that it does add a few models
You flagged this item as spam.
Brian Samson October 12th, 2009 @ 08:14 PM
This bug still exists as far as I can tell. I updated this patch to apply cleanly to edge, if somebody could double-check that for me quick I'd appreciate it.
- has_many_delete_all_in_modules_20091012.patch 10 KB
Rizwan Reza October 14th, 2009 @ 04:26 PM
-1 fail

The patch fails when testing sqlite3 adapter but passes on MySQL. I've checked on the master branch.
You flagged this item as spam.
Brian Samson October 14th, 2009 @ 10:48 PM
Good catch, Rizwan.

I was using double quotes in the eval which threw off sqlite's quoting. I changed it to %() which should work unless you know of a database that uses unmatched parentheses as a way to quote column names :)

If you have the time, it would be cool if you could retest, but I checked on sqlite3 and mysql and the tests all pass now.
- has_many_delete_all_in_modules_20091014-1.patch 10.2 KB
Rizwan Reza November 3rd, 2009 @ 04:17 PM
- Tag changed from “active_record, bugmash, dependent, has_many, module” to “active_record, bugmash, dependent, has_many, module, review”
+1 verified

The patch applies cleanly and passes tests all tests.
Andrew White November 3rd, 2009 @ 05:10 PM
Actually, isn't this another instance of this bug: https://rails.lighthouseapp.com/projects/8994/tickets/2283

Wouldn't it be better to fix the actual problem rather than patching around it all over the place in ActiveRecord?
Rizwan Reza February 12th, 2010 @ 12:46 PM
- Tag changed from “active_record, bugmash, dependent, has_many, module, review” to “active_record, dependent, has_many, module, review”
[bulk edit]
You flagged this item as spam.
Brian Samson March 30th, 2010 @ 05:54 PM
I think that perhaps Andrew's patch would fix this problem, but it looks like that has been pushed off to 3.0. Basically my patch just makes this behavior ask for Mod::Class instead of just Class, which I think it should be doing anyway.

Is there any way we can get this patch put in to 2.3.6?
Andrew White March 31st, 2010 @ 07:50 AM
- Tag changed from “active_record, dependent, has_many, module, review” to “active_record, dependent, has_many, module, patch, review”
- Assigned user set to “Yehuda Katz (wycats)”
My patch for compute_type from 2283 is a necessary fix but some other changes are required as well. The attached patch provides those fixes and tests. The dependency callbacks were as you pointed out hardcoded, so I just defer the class lookup until the callback is executed. The patch is for master but I can do a backport once it's been accepted.

I did try and use existing schema and fixtures but it broke too many tests.
- 0001-fix-delete-all-dependency.diff 8.6 KB

Jeremy Kemper April 3rd, 2010 @ 03:52 PM

State changed from “new” to “open”
Assigned user changed from “Yehuda Katz (wycats)” to “Jeremy Kemper”
Milestone changed from “2.x” to “2.3.6”

Thanks Andrew! I get two errors with the patch:

  1) Error:
test_nested_models_should_not_raise_exception_when_using_delete_all_dependency_on_association(ModulesTest):
ArgumentError: Shop is not missing constant Product!



  2) Error:
test_nested_models_should_not_raise_exception_when_using_nullify_dependency_on_association(ModulesTest):
ArgumentError: Shop is not missing constant Variant!

Andrew White April 3rd, 2010 @ 05:00 PM
Jeremy, that's because the compute type fix from from 2283 is needed. Carl's recent commit makes my patch on this ticket redundant, though it still needs backporting to 2-3-stable.

I'll move the tests from here to the patches for master and 2-3-stable on 2283 and I'll add a patch here for master to test Carl's commit and then backport the commit and test to 2-3-stable and add it here also. Is this okay?
Jeremy Kemper April 3rd, 2010 @ 09:12 PM
Ah, I missed that dependency. That sounds great; thanks Andrew!
Andrew White April 4th, 2010 @ 01:30 AM
Patch for master adding tests for preventing regression.
- 0001-test-has-many-dependency-is-lazily-evaluated.diff 1.6 KB
Andrew White April 4th, 2010 @ 01:32 AM
Patch for 2-3-stable backporting changes from Carl's commits and adding tests from patch above
- 0001-backport-of-lazy-evaluation-of-has-many-dependencies.diff 6.5 KB
Andrew White April 4th, 2010 @ 07:06 AM
Updated backport patch - the previous patch didn't send the right class to the dependent_conditions method.
- 0002-backport-of-lazy-evaluation-of-has-many-dependencies.diff 6.5 KB
Repository April 4th, 2010 @ 03:02 PM
- State changed from “open” to “committed”
(from [e617af13a2cee3278c6f76121bbb777fc7f0fea5]) Backport of lazy evaluation of has_many ..., :dependent => :___

[#2627 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/e617af13a2cee3278c6f76121bbb77...
Repository April 4th, 2010 @ 03:02 PM
(from [98b4424141b4775183618bc307e0e7d09d74df6e]) Add tests to prevent regression of lazy evaluation of has_many ..., :dependent => :___

[#2627 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/98b4424141b4775183618bc307e0e7...
You flagged this item as spam.
CDD Developers April 19th, 2010 @ 11:31 PM
The patch that was applied to the 2.3 branch introduced a regression bug when declaring a polymorphic association on a STI model hierarchy.

Making up a simple example:

class FileAssociation < ActiveRecord:Base
belongs_to :associated_record, :polymorphic => true end

class Message < ActiveRecord:Base
has_many :file_associations, :as => :associated_record, :dependent => :delete_all end

class Mail < Message
end

some_mail = Mail.first
some_mail.file_associations.first # => some file associations
some_mail.file_associations.associated_record_type # => "Message"

some_mail.destroy # => does not destroy the file associations since the generated SQL checks for associated_record_type = 'Mail'

The attached patch fixes the previous commit by using the base_class of the model class. (The fix is only intended to fix the regression and does not handle the larger issue involving STI and polymorphic associations as described in https://rails.lighthouseapp.com/projects/8994/tickets/2756 that would require a different fix)
- fixed-regression-bug-with-lazy-evaluation-of.patch 1.8 KB

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Shared Ticket Bins (Sort)

↓↑ drag 455 Open Bugs
↓↑ drag 0 Recent Open Tickets
↓↑ drag 87 Open Patches
↓↑ drag 0 Open Doc Patches
↓↑ drag 542 Stale Tickets
↓↑ drag 3 Verified Patches
↓↑ drag 87 Stale Patches
↓↑ drag 0 Yesterday's Tickets
↓↑ drag 0 Two Days Ago
↓↑ drag 0 This Week
↓↑ drag 0 Last Week
↓↑ drag 7 Bugmash
↓↑ drag 0 Bugmash review queue
↓↑ drag 5 Rails 3 High Priority Tickets

People watching this ticket

Attachments

Referenced by

2283 Unnecessary exception raised in AS::Dependencies.load_missing_constant Updated patch for master with tests for has_many with dep...
2283 Unnecessary exception raised in AS::Dependencies.load_missing_constant Updated patch for 2-3-stable with tests for has_many with...
2283 Unnecessary exception raised in AS::Dependencies.load_missing_constant Updated patch for 2-3-stable with tests for has_many with...
2627 has_many :dependent => :delete_all throws exception on models in modules [#2627 state:committed]
2627 has_many :dependent => :delete_all throws exception on models in modules [#2627 state:committed]

Rails Ruby on Rails → 2.3.6