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

Improved the error messaging for the DangerousAttributeError exception #16669

Merged

Conversation

aantix
Copy link
Contributor

@aantix aantix commented Aug 24, 2014

When the DangerousAttributeError exception is thrown, it's not entirely clear where the conflict is originating from. Now the user is alerted that the conflict is a similarly named column.

I ran into this issue the other day when a column "valid" was added via a migration. I received the message:

valid? is defined by Active Record

By the looks of that message, I thought that I had created a method called valid? on my model (which wasn't the case). It took me a while to track down that a column named 'valid' had been added to the table via a migration.

@schneems
Copy link
Member

Seems reasonable

@senny
Copy link
Member

senny commented Aug 27, 2014

This could cause confusion when the conflicting method is not a column on the models table. For example when defining an enum we make sure that the methods generated by the chosen states do not conflict. This will result in an error message suggesting you rename a column that doesn't exist.

@aantix aantix force-pushed the dangerous_attribute_error_better_message branch from fe558b4 to a77527f Compare August 30, 2014 06:14
@aantix
Copy link
Contributor Author

aantix commented Aug 30, 2014

@senny, I've added additional logic so that the rename column suggestion is only output when it's actually a column in conflict.

@aantix aantix force-pushed the dangerous_attribute_error_better_message branch from a77527f to 7d7c9ff Compare September 6, 2014 17:18
@rafaelfranca
Copy link
Member

I don't think we should add logic to this warning. We can come up with a more generic message? Something like:

raise DangerousAttributeError, "#{method_name} is defined by Active Record. Check if you don't have any attribute or method with the same name"

@senny
Copy link
Member

senny commented Sep 8, 2014

I'm with @rafaelfranca on this one. A generic message that gives hints to what could be the cause is fine but I'd rather not add any logic. At the point where this warning is raised the mistake has already been made. What would be nice is a warning when running migrations that add colliding columns. Would be a bigger change though.

@aantix
Copy link
Contributor Author

aantix commented Sep 8, 2014

@rafaelfranca @senny Good points. I'll make the changes for @rafaelfranca suggestion.

@aantix aantix force-pushed the dangerous_attribute_error_better_message branch from 7d7c9ff to 5454408 Compare September 10, 2014 04:54
@aantix
Copy link
Contributor Author

aantix commented Sep 10, 2014

@rafaelfranca Updated with clearer messaging and logic removed.

@rafaelfranca
Copy link
Member

Could you squash your commits?

…aging that the conflict could be because of a conflicting attribute.
@aantix aantix force-pushed the dangerous_attribute_error_better_message branch from 5454408 to 53e753b Compare September 11, 2014 04:40
@aantix
Copy link
Contributor Author

aantix commented Sep 11, 2014

Squashed.

rafaelfranca added a commit that referenced this pull request Sep 12, 2014
…r_message

Improved the error messaging for the DangerousAttributeError exception
@rafaelfranca rafaelfranca merged commit 7578037 into rails:master Sep 12, 2014
@rafaelfranca
Copy link
Member

Thanks

rafaelfranca added a commit that referenced this pull request Sep 12, 2014
…r_message

Improved the error messaging for the DangerousAttributeError exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants