Loading...

Type: Bug
Resolution: Fixed
Priority: Minor - P4
Fix Version/s: 8.0.1
Affects Version/s: 6.4.0
Component/s: Associations, Attributes
Labels:
None
Environment:
mongoid gem on mac/linux with rails backend

Epic Link:
Attribute Improvements

Backwards Compatibility:
Minor Change

At its core, and in summary, the issue is that the clone method is smart enough to check if the object you're calling the method on has setters for it's attribute keys, but it does not treat embedded children the same as it treats itself. With an object's embedded children (and embedded children of those children and so on), #clone does nothing to safely assign attributes and relies on Mongoid::Attributes::Processing#process_attribute to set an embedded child's attributes which throws Mongoid::Errors::UnknownAttribute when it finds an attribute in the database for that child, but does not find a getter/setter for the same child. Instead it recommends via the error message that the "broken" class should have Mongoid::Attributes::Dynamic included.

Tests to demonstrate:

require 'rails_helper'

RSpec.describe "Mongoid::Copyable#clone" do

  before do
    class CloneParent
      include Mongoid::Document
      embeds_one :clone_child
      field :a, type: :string
      field :b, type: :string
    end

    class CloneChild
      include Mongoid::Document
      embedded_in :clone_parent
      field :c, type: :string
      field :d, type: :string
    end
  end
  
  # succeeds as expected
  it "should succeed when calling clone on an object that has a field removed, where the attributes remain in the db" do
    obj = CloneParent.new(a: "1", b: "2")
    obj.save

    Object.send(:remove_const, :CloneParent)

    class CloneParent
      include Mongoid::Document
      embeds_one :clone_child
      field :a, type: :string
    end

    obj = CloneParent.last
    expect\{obj.b}.to raise_error NoMethodError
    expect(obj.attributes).to include(\{"b" => "2"})
    expect(clone = obj.clone).to be_a(CloneParent)
    expect(clone.a).to eq "1"
  end

  # currently fails
  it "should succeed when attempting to clone an object where the child has lost a field, but still has data for that field" do
    obj = CloneParent.new(a: "1", b: "2")
    obj.clone_child = CloneChild.new(c: "3", d: "4")
    obj.save

    # removing constants and redefining classes simulates a deployment, where classes may have removed field

    Object.send(:remove_const, :CloneParent)
    Object.send(:remove_const, :CloneChild)

    class CloneParent
      include Mongoid::Document
      embeds_one :clone_child
      field :a, type: :string
      field :b, type: :string
    end

    class CloneChild
      include Mongoid::Document
      embedded_in :clone_parent
      field :c, type: :string
    end

    obj = CloneParent.last
    expect\{obj.clone_child.d}.to raise_error NoMethodError
    expect(obj.clone_child.attributes).to include("d" => "4")
    expect(obj.clone).to be_truthy
  end
end

First scenario (passes) demonstrates that #clone want's to be safe about attributes set on objects, even when they have no setter methods, but second scenario (fails) demonstrates that #clone doesn't treat embedded children as safely as it treats the calling parent.

I have played around with re-writing clone so that it will check for embedded relation type as it cycles through children to assign attributes – the problem becomes, similarly, that it would need to do this recursively in order to make clone actually safe for any embedded children of embedded children (which I haven't tried to implement) eg:

      self.class.new(attrs).tap do |object|
          dynamic_attrs.each do |attr_name, value|
            if object.embedded_relations.keys.include?(attr_name)
              case object.embedded_relations[attr_name].macro
              when :embeds_one
                object.send("build_#\{attr_name}").tap do |child|
                  value.each do |k, v|
                    child.attributes[k] = v
                  end
                end
              when :embeds_many
                value.each do |child_attrs|
                  object.send("#\{attr_name}").build.tap do |new_child|
                    child_attrs.each do |k, v|
                      new_child.attributes[k] = v
                    end
                  end
                end
              end
            elsif object.respond_to?("#\{attr_name}=")
              object.send("#\{attr_name}=", value)
            else
              object.attributes[attr_name] = value
            end
          end
        end

Another option might be to somehow somehow tell Mongoid::Attributes::Processing#process_attribute not to raise the error in certain cases (such as cloning a document)
right now it does:

      def process_attribute(name, value)
        if !respond_to?("#\{name}=", true) && store_as = aliased_fields.invert[name.to_s]
          name = store_as
        end
        responds = respond_to?("#\{name}=", true)
        raise Errors::UnknownAttribute.new(self.class, name) unless responds
        send("#\{name}=", value)
      end}}

but we could do something like:

      def process_attribute(name, value, strict=false)
        if !respond_to?("#\{name}=", true) && store_as = aliased_fields.invert[name.to_s]
          name = store_as
        end
        responds = respond_to?("#\{name}=", true)
        unless responds
          if strict
            raise Errors::UnknownAttribute.new(self.class, name) 
          else
            attributes[name] = value and return
          end
        end
        send("#\{name}=", value)
      end

The #clone method should handle this situation (unless there are other rationales that I'm missing). I understand if trying to pass in attributes to the `#new` method method normally, you'd wan't it to throw the UnknownAttribute error, but I think of clone as a method whose purpose is to transfer all data from one object to another, regardless of the setters currently defined on the objects class or any embedded objects' classes.

Details

Description

Attachments

Activity

People

Dates