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

Errors can be indexed with nested attributes #19686

Merged
merged 1 commit into from
Oct 26, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,31 @@
* Add option to index errors in nested attributes

For models which have nested attributes, errors within those models will
now be indexed if :index_errors is specified when defining a
has_many relationship, or if its set in the global config.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo of it's?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is the correct way to spell its here

On Mon, Oct 26, 2015, 5:20 PM Shunsuke Aida notifications@github.com
wrote:

In activerecord/CHANGELOG.md
#19686 (comment):

@@ -1,3 +1,31 @@
+* Add option to index errors in nested attributes
+

  • For models which have nested attributes, errors within those models will
  • now be indexed if :index_errors is specified when defining a
  • has_many relationship, or if its set in the global config.

typo of it's?


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/19686/files#r43067124.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best you check its meaning 🤓

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a user reading this. Where would you find "global config?"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its indicates possession.
it's is a contraction for it is.
The latter seems to be the case here. If so, it is would be even better.


E.X.

```ruby
class Guitar < ActiveRecord::Base
has_many :tuning_pegs
accepts_nested_attributes_for :tuning_pegs
end

class TuningPeg < ActiveRecord::Base
belongs_to :guitar
validates_numericality_of :pitch
end
```

- Old style
- `guitar.errors["tuning_pegs.pitch"] = ["is not a number"]`

- New style (if defined globally, or set in has_many_relationship)
- `guitar.errors["tuning_pegs[1].pitch"] = ["is not a number"]`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example doesn't show either way to configure indexed errors. This is the place to spell it out.


*Michael Probber and Terence Sun*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!


* Fixed a bug where uniqueness validations would error on out of range values,
even if an validation should have prevented it from hitting the database.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ def self.macro
end

def self.valid_options(options)
super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache, :join_table, :foreign_type]
super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache, :join_table, :foreign_type, :index_errors]
end

def self.valid_dependent_options
Expand Down
12 changes: 9 additions & 3 deletions activerecord/lib/active_record/autosave_association.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ def self.valid_options

included do
Associations::Builder::Association.extensions << AssociationBuilderExtension
mattr_accessor :index_nested_attribute_errors, instance_writer: false
self.index_nested_attribute_errors = false
end

module ClassMethods
Expand Down Expand Up @@ -315,22 +317,26 @@ def validate_single_association(reflection)
def validate_collection_association(reflection)
if association = association_instance_get(reflection.name)
if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
records.each { |record| association_valid?(reflection, record) }
records.each_with_index { |record, index| association_valid?(reflection, record, index) }
end
end
end

# Returns whether or not the association is valid and applies any errors to
# the parent, <tt>self</tt>, if it wasn't. Skips any <tt>:autosave</tt>
# enabled records if they're marked_for_destruction? or destroyed.
def association_valid?(reflection, record)
def association_valid?(reflection, record, index=nil)
return true if record.destroyed? || record.marked_for_destruction?

validation_context = self.validation_context unless [:create, :update].include?(self.validation_context)
unless valid = record.valid?(validation_context)
if reflection.options[:autosave]
record.errors.each do |attribute, message|
attribute = "#{reflection.name}.#{attribute}"
if index.nil? || (!reflection.options[:index_errors] && !ActiveRecord::Base.index_nested_attribute_errors)
attribute = "#{reflection.name}.#{attribute}"
else
attribute = "#{reflection.name}[#{index}].#{attribute}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is the same as the first, so I think we can improve this conditionals to have only two branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We originally had it as 2 branches, but the first branch looked excessively long. Should we still do 2 branches?

end
errors[attribute] << message
errors[attribute].uniq!
end
Expand Down
36 changes: 36 additions & 0 deletions activerecord/test/cases/autosave_association_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
require 'models/member'
require 'models/member_detail'
require 'models/organization'
require 'models/guitar'
require 'models/tuning_peg'

class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
def test_autosave_validation
Expand Down Expand Up @@ -385,6 +387,40 @@ def test_invalid_adding_with_nested_attributes
assert_not molecule.persisted?, 'Molecule should not be persisted when its electrons are invalid'
end

def test_errors_should_be_indexed_when_passed_as_array
guitar = Guitar.new
tuning_peg_valid = TuningPeg.new
tuning_peg_valid.pitch = 440.0
tuning_peg_invalid = TuningPeg.new

guitar.tuning_pegs = [tuning_peg_valid, tuning_peg_invalid]

assert_not tuning_peg_invalid.valid?
assert tuning_peg_valid.valid?
assert_not guitar.valid?
assert_equal ["is not a number"], guitar.errors["tuning_pegs[1].pitch"]
assert_not_equal ["is not a number"], guitar.errors["tuning_pegs.pitch"]
end

def test_errors_should_be_indexed_when_global_flag_is_set
old_attribute_config = ActiveRecord::Base.index_nested_attribute_errors
ActiveRecord::Base.index_nested_attribute_errors = true

molecule = Molecule.new
valid_electron = Electron.new(name: 'electron')
invalid_electron = Electron.new

molecule.electrons = [valid_electron, invalid_electron]

assert_not invalid_electron.valid?
assert valid_electron.valid?
assert_not molecule.valid?
assert_equal ["can't be blank"], molecule.errors["electrons[1].name"]
assert_not_equal ["can't be blank"], molecule.errors["electrons.name"]
ensure
ActiveRecord::Base.index_nested_attribute_errors = old_attribute_config
end

def test_valid_adding_with_nested_attributes
molecule = Molecule.new
valid_electron = Electron.new(name: 'electron')
Expand Down
4 changes: 4 additions & 0 deletions activerecord/test/models/guitar.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class Guitar < ActiveRecord::Base
has_many :tuning_pegs, index_errors: true
accepts_nested_attributes_for :tuning_pegs
end
4 changes: 4 additions & 0 deletions activerecord/test/models/tuning_peg.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class TuningPeg < ActiveRecord::Base
belongs_to :guitar
validates_numericality_of :pitch
end
9 changes: 9 additions & 0 deletions activerecord/test/schema/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ def except(adapter_names_to_exclude)
t.column :key, :string
end

create_table :guitar, force: true do |t|
t.string :color
end

create_table :inept_wizards, force: true do |t|
t.column :name, :string, null: false
t.column :city, :string, null: false
Expand Down Expand Up @@ -789,6 +793,11 @@ def except(adapter_names_to_exclude)
t.belongs_to :ship
end

create_table :tuning_pegs, force: true do |t|
t.integer :guitar_id
t.float :pitch
end

create_table :tyres, force: true do |t|
t.integer :car_id
end
Expand Down
4 changes: 4 additions & 0 deletions guides/source/configuring.md
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,10 @@ All these configuration options are delegated to the `I18n` library.
by a query exceeds the threshold, a warning is logged. This can be used to
identify queries which might be causing memory bloat.

* `config.active_record.index_nested_attribute_errors` allows errors for nested
has_many relationships to be displayed with an index as well as the error.
Defaults to false.

The MySQL adapter adds one additional configuration option:

* `ActiveRecord::ConnectionAdapters::MysqlAdapter.emulate_booleans` controls whether Active Record will consider all `tinyint(1)` columns in a MySQL database to be booleans and is true by default.
Expand Down