MONGOID-5888 Ensure deeply nested children are validated correctly (#6028) (#6029)

* MONGOID-5888 make sure nested attributes trigger validations on deeply nested children

* code review feedback

Author: jamis

lib/mongoid/association/nested/many.rb

@@ -190,6 +190,8 @@ def update_nested_relation(parent, id, attrs)
             update_document(doc, attrs)
             existing.push(doc) unless destroyable?(attrs)
           end
+
+          parent.children_may_have_changed!
         end
       end
     end

lib/mongoid/association/nested/one.rb

@@ -37,7 +37,7 @@ def build(parent)
             parent.send(association.setter, nil)
           else
             check_for_id_violation!
-          end
+          end.tap { parent.children_may_have_changed! }
         end
 
         # Create the new builder for nested attributes on one-to-one

lib/mongoid/changeable.rb

@@ -15,6 +15,14 @@ def changed
       changed_attributes.keys.select { |attr| attribute_change(attr) }
     end
 
+    # Indicates that the children of this document may have changed, and
+    # ought to be checked when the document is validated.
+    #
+    # @api private
+    def children_may_have_changed!
+      @children_may_have_changed = true
+    end
+
     # Has the document changed?
     #
     # @example Has the document changed?
@@ -31,7 +39,7 @@ def changed?
     #
     # @return [ true | false ] If any children have changed.
     def children_changed?
-      _children.any?(&:changed?)
+      @children_may_have_changed || _children.any?(&:changed?)
     end
 
     # Get the attribute changes.
@@ -69,6 +77,7 @@ def move_changes
       @previous_changes = changes
       @attributes_before_last_save = @previous_attributes
       @previous_attributes = attributes.dup
+      @children_may_have_changed = false
       reset_atomic_updates!
       changed_attributes.clear
     end

spec/integration/associations/embeds_many_spec.rb

@@ -3,6 +3,24 @@
 
 require 'spec_helper'
 
+module EmbedsManySpec
+  class Post
+    include Mongoid::Document
+    field :title, type: String
+    embeds_many :comments, class_name: 'EmbedsManySpec::Comment', as: :container
+    accepts_nested_attributes_for :comments
+  end
+
+  class Comment
+    include Mongoid::Document
+    field :content, type: String
+    validates :content, presence: true
+    embedded_in :container, polymorphic: true
+    embeds_many :comments, class_name: 'EmbedsManySpec::Comment', as: :container
+    accepts_nested_attributes_for :comments
+  end
+end
+
 describe 'embeds_many associations' do
 
   context 're-associating the same object' do
@@ -258,4 +276,55 @@
       expect(klass.new.addresses.build).to be_a Address
     end
   end
+
+  context 'with deeply nested trees' do
+    let(:post) { EmbedsManySpec::Post.create!(title: 'Post') }
+    let(:child) { post.comments.create!(content: 'Child') }
+
+    # creating grandchild will cascade to create the other documents
+    let!(:grandchild) { child.comments.create!(content: 'Grandchild') }
+
+    let(:updated_parent_title) { 'Post Updated' }
+    let(:updated_grandchild_content) { 'Grandchild Updated' }
+
+    context 'with nested attributes' do
+      let(:attributes) do
+        {
+          title: updated_parent_title,
+          comments_attributes: [
+            {
+              # no change for comment1
+              _id: child.id,
+              comments_attributes: [
+                {
+                  _id: grandchild.id,
+                  content: updated_grandchild_content,
+                }
+              ]
+            }
+          ]
+        }
+      end
+
+      context 'when the grandchild is invalid' do
+        let(:updated_grandchild_content) { '' } # invalid value
+
+        it 'will not save the parent' do
+          expect(post.update(attributes)).to be_falsey
+          expect(post.errors).not_to be_empty
+          expect(post.reload.title).not_to eq(updated_parent_title)
+          expect(grandchild.reload.content).not_to eq(updated_grandchild_content)
+        end
+      end
+
+      context 'when the grandchild is valid' do
+        it 'will save the parent' do
+          expect(post.update(attributes)).to be_truthy
+          expect(post.errors).to be_empty
+          expect(post.reload.title).to eq(updated_parent_title)
+          expect(grandchild.reload.content).to eq(updated_grandchild_content)
+        end
+      end
+    end
+  end
 end

spec/integration/associations/has_and_belongs_to_many_spec.rb

@@ -23,6 +23,38 @@ class Attachment
     include Mongoid::Document
     field :file, type: String
   end
+
+  class Item
+    include Mongoid::Document
+
+    field :title, type: String
+
+    has_and_belongs_to_many :colors, class_name: 'HabtmSpec::Color', inverse_of: :items
+
+    accepts_nested_attributes_for :colors
+  end
+
+  class Beam
+    include Mongoid::Document
+
+    field :name, type: String
+    validates :name, presence: true
+
+    has_and_belongs_to_many :colors, class_name: 'HabtmSpec::Color', inverse_of: :beams
+
+    accepts_nested_attributes_for :colors
+  end
+
+  class Color
+    include Mongoid::Document
+
+    field :name, type: String
+    
+    has_and_belongs_to_many :items, class_name: 'HabtmSpec::Item', inverse_of: :colors
+    has_and_belongs_to_many :beams, class_name: 'HabtmSpec::Beam', inverse_of: :colors
+
+    accepts_nested_attributes_for :items, :beams
+  end
 end
 
 describe 'has_and_belongs_to_many associations' do
@@ -59,4 +91,53 @@ class Attachment
       expect { image_block.save! }.not_to raise_error
     end
   end
+
+  context 'with deeply nested trees' do
+    let(:item) { HabtmSpec::Item.create!(title: 'Item') }
+    let(:beam) { HabtmSpec::Beam.create!(name: 'Beam') }
+    let!(:color) { HabtmSpec::Color.create!(name: 'Red', items: [ item ], beams: [ beam ]) }
+
+    let(:updated_item_title) { 'Item Updated' }
+    let(:updated_beam_name) { 'Beam Updated' }
+
+    context 'with nested attributes' do
+      let(:attributes) do
+        {
+          title: updated_item_title,
+          colors_attributes: [
+            {
+              # no change for color
+              _id: color.id,
+              beams_attributes: [
+                {
+                  _id: beam.id,
+                  name: updated_beam_name,
+                }
+              ]
+            }
+          ]
+        }
+      end
+
+      context 'when the beam is invalid' do
+        let(:updated_beam_name) { '' } # invalid value
+
+        it 'will not save the parent' do
+          expect(item.update(attributes)).to be_falsey
+          expect(item.errors).not_to be_empty
+          expect(item.reload.title).not_to eq(updated_item_title)
+          expect(beam.reload.name).not_to eq(updated_beam_name)
+        end
+      end
+
+      context 'when the beam is valid' do
+        it 'will save the parent' do
+          expect(item.update(attributes)).to be_truthy
+          expect(item.errors).to be_empty
+          expect(item.reload.title).to eq(updated_item_title)
+          expect(beam.reload.name).to eq(updated_beam_name)
+        end
+      end
+    end
+  end
 end

spec/integration/associations/has_many_spec.rb

@@ -126,4 +126,60 @@
       end
     end
   end
+
+  context 'with deeply nested trees' do
+    let(:post) { HmmPost.create!(title: 'Post') }
+    let(:child) { post.comments.create!(title: 'Child') }
+
+    # creating grandchild will cascade to create the other documents
+    let!(:grandchild) { child.comments.create!(title: 'Grandchild') }
+
+    let(:updated_parent_title) { 'Post Updated' }
+    let(:updated_grandchild_title) { 'Grandchild Updated' }
+
+    context 'with nested attributes' do
+      let(:attributes) do
+        {
+          title: updated_parent_title,
+          comments_attributes: [
+            {
+              # no change for comment1
+              _id: child.id,
+              comments_attributes: [
+                {
+                  _id: grandchild.id,
+                  title: updated_grandchild_title,
+                  num: updated_grandchild_num,
+                }
+              ]
+            }
+          ]
+        }
+      end
+
+      context 'when the grandchild is invalid' do
+        let(:updated_grandchild_num) { -1 } # invalid value
+
+        it 'will not save the parent' do
+          expect(post.update(attributes)).to be_falsey
+          expect(post.errors).not_to be_empty
+          expect(post.reload.title).not_to eq(updated_parent_title)
+          expect(grandchild.reload.title).not_to eq(updated_grandchild_title)
+          expect(grandchild.num).not_to eq(updated_grandchild_num)
+        end
+      end
+
+      context 'when the grandchild is valid' do
+        let(:updated_grandchild_num) { 1 }
+
+        it 'will save the parent' do
+          expect(post.update(attributes)).to be_truthy
+          expect(post.errors).to be_empty
+          expect(post.reload.title).to eq(updated_parent_title)
+          expect(grandchild.reload.title).to eq(updated_grandchild_title)
+          expect(grandchild.num).to eq(updated_grandchild_num)
+        end
+      end
+    end
+  end
 end

spec/integration/associations/has_one_spec.rb

@@ -224,7 +224,7 @@
     end
 
     context "when explicitly setting the foreign key" do
-      let(:comment2) { HomComment.new(post_id: post.id, content: "2") }
+      let(:comment2) { HomComment.new(container_id: post.id, container_type: post.class.name, content: "2") }
 
       it "persists the new comment" do
         post.comment = comment1
@@ -264,10 +264,62 @@
 
     it "does not overwrite the original value" do
       pending "MONGOID-3999"
-      p1 = comment.post
+      p1 = comment.container
       expect(p1.title).to eq("post 1")
-      comment.post = post2
+      comment.container = post2
       expect(p1.title).to eq("post 1")
     end
   end
+
+  context 'with deeply nested trees' do
+    let(:post) { HomPost.create!(title: 'Post') }
+    let(:child) { post.create_comment(content: 'Child') }
+
+    # creating grandchild will cascade to create the other documents
+    let!(:grandchild) { child.create_comment(content: 'Grandchild') }
+
+    let(:updated_parent_title) { 'Post Updated' }
+    let(:updated_grandchild_content) { 'Grandchild Updated' }
+
+    context 'with nested attributes' do
+      let(:attributes) do
+        {
+          title: updated_parent_title,
+          comment_attributes: {
+            # no change for child
+            _id: child.id,
+            comment_attributes: {
+              _id: grandchild.id,
+              content: updated_grandchild_content,
+              num: updated_grandchild_num,
+            }
+          }
+        }
+      end
+
+      context 'when the grandchild is invalid' do
+        let(:updated_grandchild_num) { -1 } # invalid value
+
+        it 'will not save the parent' do
+          expect(post.update(attributes)).to be_falsey
+          expect(post.errors).not_to be_empty
+          expect(post.reload.title).not_to eq(updated_parent_title)
+          expect(grandchild.reload.content).not_to eq(updated_grandchild_content)
+          expect(grandchild.num).not_to eq(updated_grandchild_num)
+        end
+      end
+
+      context 'when the grandchild is valid' do
+        let(:updated_grandchild_num) { 1 }
+
+        it 'will save the parent' do
+          expect(post.update(attributes)).to be_truthy
+          expect(post.errors).to be_empty
+          expect(post.reload.title).to eq(updated_parent_title)
+          expect(grandchild.reload.content).to eq(updated_grandchild_content)
+          expect(grandchild.num).to eq(updated_grandchild_num)
+        end
+      end
+    end
+  end
 end

spec/mongoid/association/referenced/has_many_models.rb

@@ -96,3 +96,27 @@ class HmmAnimal
 
   belongs_to :trainer, class_name: 'HmmTrainer', scope: -> { where(name: 'Dave') }
 end
+
+class HmmPost
+  include Mongoid::Document
+
+  field :title, type: String
+
+  has_many :comments, class_name: 'HmmComment', as: :container
+
+  accepts_nested_attributes_for :comments, allow_destroy: true
+end
+
+class HmmComment
+  include Mongoid::Document
+
+  field :title, type: String
+  field :num, type: Integer, default: 0
+
+  belongs_to :container, polymorphic: true
+  has_many :comments, class_name: 'HmmComment', as: :container
+
+  accepts_nested_attributes_for :comments, allow_destroy: true
+
+  validates :num, numericality: { greater_than_or_equal_to: 0 }
+end