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

Message on AR::UnknownAttributeError should include the class name of a record #17019

Merged
merged 1 commit into from Oct 20, 2014

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Sep 23, 2014

This would be helpful if 2 models have an attribute that has a similar name to the other. e.g.
before:

User.new(name: "Yuki Nishijima", projects_attributes: [name: "kaminari"])
# => ActiveRecord::UnknownAttributeError: unknown attribute: name

after:

User.new(name: "Yuki Nishijima", projects_attributes: [name: "kaminari"])
# => ActiveRecord::UnknownAttributeError: unknown attribute on User: name

@yuki24 yuki24 force-pushed the add-class-name-to-unknown-attr-error branch from 0407917 to adbbf9d Compare September 24, 2014 01:51
This would be helpful if 2 models have an attribute that has a similar
name to the other. e.g:

before:

    User.new(name: "Yuki Nishijima", projects_attributes: [name: "kaminari"])
    # => ActiveRecord::UnknownAttributeError: unknown attribute: name

after:

    User.new(name: "Yuki Nishijima", projects_attributes: [name: "kaminari"])
    # => ActiveRecord::UnknownAttributeError: unknown attribute on User: name
@yuki24 yuki24 force-pushed the add-class-name-to-unknown-attr-error branch from adbbf9d to 074880c Compare October 16, 2014 00:34
@yuki24
Copy link
Contributor Author

yuki24 commented Oct 16, 2014

rebased twice so far. Any chance this pull request could be merged into master?

@sgrif
Copy link
Contributor

sgrif commented Oct 16, 2014

Should this handle ActiveModel::MissingAttributeError, which is raised by many methods called by Active Record internally? https://github.com/rails/rails/blob/master/activemodel/lib/active_model/attribute_methods.rb#L476-L478

@yuki24
Copy link
Contributor Author

yuki24 commented Oct 17, 2014

It's a good point, I'll change ActiveModel::MissingAttributeError to include the class name as well!

@sgrif
Copy link
Contributor

sgrif commented Oct 17, 2014

❤️

@sgrif
Copy link
Contributor

sgrif commented Oct 17, 2014

[]= is the easiest way to raise for testing

@yuki24
Copy link
Contributor Author

yuki24 commented Oct 19, 2014

After spending some time on this, It turns out that ActiveRecord::Attribute::Null, which eventually raises ActiveModel::MissingAttributeError when []= is called, can't put a class name into the message, as it doesn't know about the class it belongs to.

To make ActiveRecord::Attribute::Null aware of the class name, we have to change these changes:

  1. ActiveRecord::AttributeSet has to pass a class name to ActiveRecord::Attribute::Null
  2. ActiveRecord::AttributeSet has to know about the class name to do 1
  3. ActiveRecord::AttributeSet::Builder has to give a class name to an attribute set, to do 2
  4. ActiveRecord::ModelSchema has to pass a class name to the AttributeSet::Builder to do 3

Is that a good idea? Probably not. I think we should change only ActiveRecord::UnknownAttributeError for now. Does it make sense or should we just close this request?

@sgrif
Copy link
Contributor

sgrif commented Oct 19, 2014

Yes I do not think we should pass the class to any of those subclasses. I
think just handling the active record error is fine for now.
On Oct 18, 2014 9:39 PM, "Yuki Nishijima" notifications@github.com wrote:

After spending some time on this, It turns out that
ActiveRecord::Attribute::Null will raise
ActiveModel::MissingAttributeError when []= is called, but it can't put a
class name into the message, as ActiveRecord::Attribute::Null doesn't
know about the class it belongs to.

To make ActiveRecord::Attribute::Null be aware of the class name, we have
to change these changes:

  1. ActiveRecord::AttributeSet has to pass a class name to
    ActiveRecord::Attribute::Null
  2. ActiveRecord::AttributeSet has to know about the class name to do 1
  3. ActiveRecord::AttributeSet::Builder has to give a class name to an
    attribute set, to do 2
  4. ActiveRecord::ModelSchema have to pass a class name to the
    AttributeSet::Builder to do 3

Is that a good idea? Probably not. I think we should change only
ActiveRecord::UnknownAttributeError for now. Does it make sense or should
we just close this request?


Reply to this email directly or view it on GitHub
#17019 (comment).

@senny senny merged commit 074880c into rails:master Oct 20, 2014
senny added a commit that referenced this pull request Oct 20, 2014
…error

Message on AR::UnknownAttributeError should include the class name of a record
@senny
Copy link
Member

senny commented Oct 20, 2014

@yuki24 thank you 💛 reworded the message slightly and shortened the CHANGELOG.

@yuki24
Copy link
Contributor Author

yuki24 commented Oct 21, 2014

Thank you! ❤️ ❤️ ❤️

@yuki24 yuki24 deleted the add-class-name-to-unknown-attr-error branch October 21, 2014 01:05
@rebyn
Copy link
Contributor

rebyn commented Oct 21, 2014

Awesome change 👍.

yuki24 added a commit to ruby/did_you_mean that referenced this pull request Oct 24, 2014
Error message has been changed on rails/rails#17019
And did_you_mean should be aware of it.
yuki24 added a commit to ruby/did_you_mean that referenced this pull request Oct 24, 2014
Error message has been changed on rails/rails#17019
and did_you_mean should be aware of it.
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