Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAssociations now raises `ArgumentError` on name conflicts. #13896
Conversation
carlosantoniodasilva
reviewed
Jan 31, 2014
View changes
test 'dangerous association name raises ArgumentError' do | ||
[:errors, 'errors', :save, 'save'].each do |name| | ||
assert_raises(ArgumentError, "Association #{name} should not be allowed") do | ||
Post.belongs_to name |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
reviewed
Jan 31, 2014
View changes
define_callbacks model, reflection | ||
builder.define_extensions model | ||
reflection | ||
end |
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
Jan 31, 2014
Member
I think you don't need to use an else block here, you can left the code untouched and just add the conditional to raise.
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
reviewed
Jan 31, 2014
View changes
@@ -1,3 +1,19 @@ | |||
* Associations now raises `ArgumentError` on name conflicts. |
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
reviewed
Jan 31, 2014
View changes
@@ -1,3 +1,19 @@ | |||
* Associations now raises `ArgumentError` on name conflicts. | |||
|
|||
Dangerous association names conflicts includes instance or class method already |
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
reviewed
Jan 31, 2014
View changes
if model.dangerous_attribute_method?(name) | ||
raise ArgumentError, "You tried to define an association named #{name} on the model #{model.name}, but "\ | ||
"this will generate an instance method #{name}, which is already defined by Active Record." | ||
end |
This comment has been minimized.
This comment has been minimized.
carlosantoniodasilva
Jan 31, 2014
Member
How about: but this will conflict with a method #{name} already defined by Active Record. Please choose a different association name.
so that we direct the user towards a solution.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
chancancode
Jan 31, 2014
Member
Might want to use an anonymous Class
in the tests instead of Post.belongs_to ...
because the belongs_to
call might have irreversible side-effect on the Post
class, which might break other tests cases.
In a real app this wouldn't matter because they except would just blow up the app completely.
This comment has been minimized.
This comment has been minimized.
@carlosantoniodasilva @chancancode updated with the suggested changes. Thanks guys :) |
added a commit
that referenced
this pull request
Jan 31, 2014
carlosantoniodasilva
merged commit 5df5296
into
rails:master
Jan 31, 2014
1 check passed
This comment has been minimized.
This comment has been minimized.
Thanks! |
This comment has been minimized.
This comment has been minimized.
Thanks |
laurocaetano commentedJan 31, 2014
It's an attempt to fix #13217.
In order to avoid unexpected behavior, dangerous association names should raise
ArgumentError
.This way, models like this:
Will raise
ArgumentError
, saying that the association name will generate an instance method which is already defined byActiveRecord::Base
.