Fix autosave for accepts_nested_attributes_for

[kodawill]

Summary

This PR attempts to automatically save associated documents for referenced (i.e., non-embedded) associations when the parent is saved even if autosave option set to false as long as associated documents are declared as nested attributes via accepts_nested_attributes_for and autosave is set to true.

class Foo
  include Mongoid::Document
 
  has_many :bars, autosave: false

  accepts_nested_attributes_for :bars, autosave: true # This should autosave :bars association
                                                      # even if autosave is set to false in has_many
end
 
class Bar
  include Mongoid::Document
 
  belongs_to :foo
end
 
foo = Foo.new(bars: [Bar.new])
foo.save # must save associated bars documents

[kodawill]

This is now ready for review.

[johnnyshields]

@kodawill two points here.

First, please make a test case which demonstrates why this change is needed. For example:

1. Start with a test model (such as Person or Car)
2. Assign nested attributes of a has_many related model
3. Make a test which asserts the related model should be persisted. This test should fail on master, and succeed after applying your patch.

Second, (assuming this is a legitimate issue per #1) I don't think that specifying accepts_nested_attributes_for should set auto-save to true, because that would also affect cases where nested attributes are not passed-in. Instead, if autosave: false, an ideal fix IMHO would only invoke auto-save logic if the user passed in nested attributes for the relation, but otherwise would not autosave. If we go this route, there should be tests for all of this as well.

[kodawill]

@johnnyshields first of all thank you for your feedback. I have reverted previous changes and instead, as you suggested, wrote a failing spec demonstrating the possible bug for autosaving. If I understand you correctly, auto-save logic should only be invoked if autosave: true in accepts_nested_attributes_for even if association is set as autosave: false. Please confirm if that's what you have in mind. Please refer in description as I have updated the model definition in it for illustration.

[johnnyshields]

@kodawill re: your comment:

If I understand you correctly, auto-save logic should only be invoked if autosave: true in accepts_nested_attributes_for even if association is set as autosave: false

Let's start with specs that describe the end-result behavior when setting nested attributes, i.e., "is the nested model actually being persisted in the database under X conditions." The specs you have currently make assertions around whether autosave is coerced to be true, but what we are really interested in is whether NestedLike objects are saved in the DB.

After we describe the desired behavior, then we can consider what the ideal approach is to achieve it (i.e. use autosave, or use some other logic.)

[kodawill]

@johnnyshields Added another test (failing) to describe the desired behavior.

[kodawill]

And here is my proposal for the fix:

--- a/lib/mongoid/attributes/nested.rb
+++ b/lib/mongoid/attributes/nested.rb
@@ -47,6 +47,7 @@ module Mongoid
         #   enable it.
         def accepts_nested_attributes_for(*args)
           options = args.extract_options!.dup
+          autosave_via_nested_attributes = options[:autosave]
           options[:autosave] = true if options[:autosave].nil?

           options[:reject_if] = REJECT_ALL_BLANK_PROC if options[:reject_if] == :all_blank
@@ -55,7 +56,7 @@ module Mongoid
             self.nested_attributes["#{name}_attributes"] = meth
             association = relations[name.to_s]
             raise Errors::NestedAttributesMetadataNotFound.new(self, name) unless association
-            autosave_nested_attributes(association) if options[:autosave]
+            autosave_nested_attributes(association, autosave_via_nested_attributes) if options[:autosave]

             re_define_method(meth) do |attrs|
               _assigning do
@@ -78,12 +79,13 @@ module Mongoid
         #   Person.autosave_nested_attributes(metadata)
         #
         # @param [ Mongoid::Association::Relatable ] association The existing association metadata.
-        def autosave_nested_attributes(association)
+        # @param [ true | false ] autosave_via_nested_attributes Autosave once options[:autosave] is true in accepts_nested_attributes_for
+        def autosave_nested_attributes(association, autosave_via_nested_attributes = false)
           # In order for the autosave functionality to work properly, the association needs to be
           # marked as autosave despite the fact that the option isn't present. Because the method
           # Association#autosave? is implemented by checking the autosave option, this is the most
           # straightforward way to mark it.
-          if association.autosave? || (association.options[:autosave].nil? && !association.embedded?)
+          if association.autosave? || ((association.options[:autosave].nil? || autosave_via_nested_attributes) && !association.embedded?)
             association.options[:autosave] = true
             Association::Referenced::AutoSave.define_autosave!(association)
           end

[kodawill]

Another option by using another (additional) method:

--- a/lib/mongoid/attributes/nested.rb
+++ b/lib/mongoid/attributes/nested.rb
@@ -46,6 +46,7 @@ module Mongoid
         #   association. Note that since the default is true, setting autosave to nil will still
         #   enable it.
         def accepts_nested_attributes_for(*args)
+          autosave_nested_attrs = autosave_nested_attribute?(args.dup)
           options = args.extract_options!.dup
           options[:autosave] = true if options[:autosave].nil?

@@ -55,7 +56,7 @@ module Mongoid
             self.nested_attributes["#{name}_attributes"] = meth
             association = relations[name.to_s]
             raise Errors::NestedAttributesMetadataNotFound.new(self, name) unless association
-            autosave_nested_attributes(association) if options[:autosave]
+            autosave_nested_attributes(association, autosave_nested_attrs) if options[:autosave]

             re_define_method(meth) do |attrs|
               _assigning do
@@ -78,16 +79,32 @@ module Mongoid
         #   Person.autosave_nested_attributes(metadata)
         #
         # @param [ Mongoid::Association::Relatable ] association The existing association metadata.
-        def autosave_nested_attributes(association)
+        # @param [ true | false ] autosave_nested_attrs Autosave once options[:autosave] is true in accepts_nested_attributes_for
+        def autosave_nested_attributes(association, autosave_nested_attrs = false)
           # In order for the autosave functionality to work properly, the association needs to be
           # marked as autosave despite the fact that the option isn't present. Because the method
           # Association#autosave? is implemented by checking the autosave option, this is the most
           # straightforward way to mark it.
-          if association.autosave? || (association.options[:autosave].nil? && !association.embedded?)
+          if association.autosave? || ((association.options[:autosave].nil? || autosave_nested_attrs) && !association.embedded?)
             association.options[:autosave] = true
             Association::Referenced::AutoSave.define_autosave!(association)
           end
         end
+
+
+
+        # This method should be used to detect whether a nested attribute association
+        # is autosave or not when parent document is saved. Nested attribute association
+        # is autosave if `autosave` option in #accepts_nested_attributes_for is explicitly set to true.
+        #
+        # @param [ Hash ] args The options being passed in accepts_nested_attributes_for.
+        #
+        # @return [ true | false ] Whether nested attribute is to be autosave
+        # on parent document save or not.
+        def autosave_nested_attribute?(args)
+          options = args.extract_options!
+          options[:autosave] == true
+        end
       end
     end
   end

[johnnyshields]

@jamis it's fine to close this PR. I will take it over and re-raise.