This project is archived and is in readonly mode.
has_many :dependent => :delete_all throws exception on models in modules
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 endThe 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.
-
Geoff Coffey+1
-
CancelProfileIsBroken
- Tag changed from active_record, dependent, has_many, module to active_record, bugmash, dependent, has_many, module
-
Dan Pickett+1 verified
this applies cleanly to master with a few whitespace warnings
test suite passes
note that it does add a few models
-
Brian Samson
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.
-
Rizwan Reza-1 fail
The patch fails when testing sqlite3 adapter but passes on MySQL. I've checked on the master branch.
-
Brian Samson
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.
-
Rizwan Reza
- 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 WhiteActually, isn't this another instance of this bug: #2283 Unnecessary exception raised in AS::Dependencies.load_missing_constant
Wouldn't it be better to fix the actual problem rather than patching around it all over the place in ActiveRecord?
-
Rizwan Reza
- Tag changed from active_record, bugmash, dependent, has_many, module, review to active_record, dependent, has_many, module, review
-
Brian Samson
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
- 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.
-
Jeremy Kemper- 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
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
Ah, I missed that dependency. That sounds great; thanks Andrew!
-
Andrew White
Patch for master adding tests for preventing regression.
-
Andrew White
Patch for 2-3-stable backporting changes from Carl's commits and adding tests from patch above
-
Andrew White
Updated backport patch - the previous patch didn't send the right class to the dependent_conditions method.
-
Repository
- State changed from open to committed
(from [e617af13a2cee3278c6f76121bbb777fc7f0fea5]) Backport of lazy evaluation of has_many ..., :dependent => :___
[#2627 has_many :dependent => :delete_all throws exception on models in modules state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/e617af13a2cee3278c6f76121bbb77... -
Repository
(from [98b4424141b4775183618bc307e0e7d09d74df6e]) Add tests to prevent regression of lazy evaluation of has_many ..., :dependent => :___
[#2627 has_many :dependent => :delete_all throws exception on models in modules state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/98b4424141b4775183618bc307e0e7... -
CDD Developers
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 endclass Message < ActiveRecord:Base
has_many :file_associations, :as => :associated_record, :dependent => :delete_all endclass Mail < Message
endsome_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 #2756 [patch] The polymorphic type field should be subclass name instead of base cl... that would require a different fix)