Field definition should remove any existing accessors before setting its own ones

[cperezabo]

Hi, based on what we have been discussing in #5851 I made a first attempt.

I was thinking that we should not take care about warnings. Instead, the ORM/ODM should do whatever necessary to map things correctly.

Tell me if I forgot something.

Thanks!

[cperezabo]

Hi, any update on this? Remember I can do the job of changing or adding whatever needed

[jamis]

@cperezabo -- Sorry for the silence on our end. There were several members of the team that were out of office yesterday when we were going to discuss it. It's still scheduled for discussion, though, and I'll let you know if we need any changes made. 👍

[jamis]

Hey @cperezabo -- we talked about it and think it would be better to warn, than to auto-remove the other method. It might be that the programmer is including a module that adds the field, and the host model happens to have a method with the same name that should not be clobbered. It's better to warn about the collision and let the programmer do what needs to be done, than to assume the existing method is safe to be overwritten.

[johnnyshields]

I agree with @jamis here. It could be intentional.

[cperezabo]

Hi! I've thinking on this and further more whether this change will be useful for pure-Mongoid users.

What do I mean?

We develop with POROs, we don't have the DB in mind while developing (contradictorily to what most Mongoid users do).
When a feature is done we just change some objects with polymorphic versions that touch the DB.
All this with the 100% of coverage from the test suite generated by doing TDD and the plain old objects that model the domain.

We have been working in other projects with SQLAlchemy and Doctrine, but now in Ruby we have the problem of Mongoid (the best Ruby ODM by far) being an ActiveRecord-like.

We have been working until now by sub-classifying things. For example, if we have an Invoice class, we add an InvoiceMongo < Invoice subclass and put the mapping stuff there.

We also made a tradeoff by using accessors instead of instance variables (what is natural for plain objects) so we can use Mongoid to help us mapping our objects to the DB.
By using accessors, our objects are already "prepared" for the accessors that Mongoid will add after fields definition.

The problem comes when you have an A class with 2 subtypes B and C. If you add an AMongo < A subclass, then you have to add BMongo < AMongo and CMongo < AMongo, and now, BMongo is not a B anymore, which breaks the model.

So here we are, we need to "transform" existent classes into Document so we can preserve inheritance and simplify things.

Nowadays our project is working like this:

Suppose a dummy class like the following, using accessors instead of simply instance variables (the tradeoff we made):

class Person
  def initialize(name:)
    @name = name
  end

  def rename_myself
    self.name = "#{name} renamed"
  end

  def present
    "Hi, I'm #{name}"
  end

  private

  attr_accessor :name
end

now we conditionally open the class and configure it like this:

class Person
  include MongoDocument

  field :name

  autosave_on :rename_myself
end

by using the following MongoDocument module:

module MongoDocument
  extend ActiveSupport::Concern
  include Mongoid::Document

  included do
    class << self
      def field(name, options = {})
        remove_method name if method_defined?(name, false) || private_method_defined?(name, false)
        remove_method "#{name}=" if method_defined?("#{name}=", false) || private_method_defined?("#{name}=", false)
        super
      end

      def autosave_on(method_name)
        old_method = instance_method(method_name)
        define_method method_name do |*args, **kwargs|
          old_method.bind(self).call(*args, **kwargs).tap { save! }
        end
      end
    end

    def initialize(*, **)
      super
    end

    subclasses.each(&method(:inherited))
  end
end

The only thing we have custom made is the autosave_on, so we don't have to send save! after methods that mutate the object.

I wanted to contribute so we can take some of this functionality inside Mongoid and make things easier.

That's the reason of this PR and the original discussion thread.

I hope this has been enlightening

Cheers!

[jamis]

Hey, I appreciate the extra context. As you said, most Mongoid users won't ever encounter this issue, and it looks like the automatic removal of existing methods is very specific to your use-case (where you actually expect specific name collisions when you add the module). I definitely appreciate you reaching out and wanting to contribute to Mongoid--and I hope you'll do so again!--but yeah, in this instance, I don't think it makes sense to add this functionality to Mongoid itself.

As I said, we'd be open to a PR that adds a warning in this case, but I can also see how that's not functionality that will be helpful in your usage of Mongoid.

[cperezabo]

Yeah, we completely agree and of course, I'll contribute to the project whenever possible!

Also, I'll keep up the work of creating a datamapper-like layer around Mongoid and then publish it as an independent gem so Mongoid stays as-is.

I hope I can reach you in the discussions area if I encounter some issue. 😜

Cheers! 🙂