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

Weird handling of nested association error messages when assigned to :base attribute #19863

Open
ramsdenct opened this issue Apr 22, 2015 · 8 comments

Comments

@ramsdenct
Copy link

Typically when an error is assigned to the :base attribute the message will not be prepended with the humanized class name. In the case of associations where a child class assigns an error to :base and the parent attempts to save the item the child item will instead return the error in the format: "{Child class} base {message}".

Contrived Example

class Container < ActiveRecord::Base
  has_many :items
  accepts_nested_attributes_for :items
end

class Item < ActiveRecord::Base
  belongs_to :containers

  validate do |item|
    item.errors.add(:base, 'An item must have a name.') unless item.name?
  end
end

c = Container.new(name: 'Magic Box')
c.items << Item.new
c.save
c.errors
c.errors.full_messages

Output

irb(main):016:0> c = Container.new(name: 'Magic Box')
=> #<Container id: nil, name: "Magic Box", created_at: nil, updated_at: nil>
irb(main):017:0> c.items << Item.new
=> #<ActiveRecord::Associations::CollectionProxy [#<Item id: nil, name: nil, created_at: nil, updated_at: nil, container_id: nil>]>
irb(main):018:0> c.save
   (0.1ms)  begin transaction
   (0.1ms)  rollback transaction
=> false
irb(main):019:0> c.errors
=> #<ActiveModel::Errors:0x0000010b019aa8 @base=#<Container id: nil, name: "Magic Box", created_at: nil, updated_at: nil>, @messages={:"items.base"=>["An item must have a name."]}>
irb(main):020:0> c.errors.full_messages
=> ["Items base An item must have a name."]
irb(main):021:0> 

I believe this can be corrected by a small change to ActiveRecord::AutosaveAssociation::association_valid?:

attribute = "#{reflection.name}.#{attribute}"

becomes

attribute = "#{reflection.name}.#{attribute}" unless attribute == :base

Though I'm not sure whether there would be unanticipated effects from this change or whether the existing behavior is intended for some reason.

@derekprior
Copy link
Contributor

Here is a reproducible bug script for this: https://gist.github.com/derekprior/f231c46d626697b9965f

@imanel
Copy link
Contributor

imanel commented Apr 26, 2015

It's slightly more complicated.

Line you're mentioning is in AutosaveAssociation, and is responsible for properly setting error name. Right now resulting c.errors.keys will look have proper namespace:

> c.errors.keys
#=> [:"items.base"]

If you will remove it then it will change to just :base, and ActiveModel::Errors will treat it as error on main model instead of associated one:

> c.errors.full_messages
=> ["An item must have a name."]

Everything looks nice and clean... unless you're using slightly less descriptive errors:

class Post < ActiveRecord::Base
  has_many :comments
  accepts_nested_attributes_for :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
  validate do |item|
    item.errors.add(:base)
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.new
    post.comments << Comment.new
    post.save

    assert_equal ["Comments base is invalid"], post.errors.full_messages
  end
end

After removal of mentioned line error message would look as follows:

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.new
    post.comments << Comment.new
    post.save

    assert_equal ["is invalid"], post.errors.full_messages
    assert_equal ["is invalid"], post.errors.on(:base)
  end
end

adam12 added a commit to adam12/rails that referenced this issue Jul 21, 2015
Error messages created by the saving of associated models (via
`autosave: true` on association or other) are namespaced and separated
by a period. Any error messages set by the associated model on their
base attribute should be returned in their entirety, and not prefixed
with a humanized representation of the attribute name.

Possible fix for rails#19863
adam12 added a commit to adam12/rails that referenced this issue Jul 21, 2015
Error messages created by the saving of associated models (via
`autosave: true` on association or other) are namespaced and separated
by a period. Any error messages set by the associated model on their
base attribute should be returned in their entirety, and not prefixed
with a humanized representation of the attribute name.

Possible fix for rails#19863
@rails-bot
Copy link

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@lukeredpath
Copy link
Contributor

I am still seeing this behaviour on Rails 4.2.5

@dzmitrys-dev
Copy link

still in 5.1.2

@imanel
Copy link
Contributor

imanel commented Mar 28, 2018

@rafaelfranca do you think we should treat it as a bug, and fix it in similar way to #20962 (I believe it was closed because of no response for nearly two years), or it should stay as it is and this issue should be closes as correct behaviour?

@lulalala
Copy link
Contributor

lulalala commented Apr 2, 2018

@imanel Currently I am working on #32313. With the introduction of nested errors, there is a slight chance to fix this along the way.

However this issue does not really mention what the correct behavior should look like. As you mentioned, it would be weird:

    assert_equal ["is invalid"], post.errors.full_messages

As a proposal: I think this may be good enough:

message: "An item must have a name."
full_message: "Items: An item must have a name."

@spaghetticode
Copy link
Contributor

I don't know if this can be considered a proper fix (IMHO it is... it just needs documentation) but it's at least a good workaround until somebody produces the right code fix.

It all boils down to adding a translation key for the model/association name as in this example:

# models:

class User < ApplicationRecord
  has_one :home
  has_many :phones

  accepts_nested_attributes_for :home
  accepts_nested_attributes_for :phones
end

class Home < ApplicationRecord
  belongs_to :user

  validate do |i|
    i.errors.add :base, :invalid
  end
end

class Phone < ApplicationRecord
  belongs_to :user

  validate do |i|
    i.errors.add :base, :invalid
  end
end

# config/locales/en.yml

en
  activerecord:
    attributes:
      user/home:
        base: Home
      user/phones:
        base: Phone

# try it:

u = User.new
home = Home.new(user: u)
u.valid?
u.errors.full_messages
# => ["Home is invalid"]

u = User.new
u.phones << Phone.new
u.valid?
u.errors.full_messages
# => ["Phone is invalid"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants