balvig/spyke

Dirty model check

Closed this issue · 5 comments

johan-smits commented

I want to be able to check if the model has changed before hitting save. This can significant improve performance of doing batch jobs (import) that needs to update the source. If nothing has changed then don't save it.
Like https://api.rubyonrails.org/classes/ActiveModel/Dirty.html

balvig commented

@johan-smits sounds good, I think #104 has enabled you to include and use https://api.rubyonrails.org/classes/ActiveModel/Dirty.html as you see fit :)

johan-smits commented

I will try to apply this, if needed I'll reopen the issue.

paulvt commented

The question is whether it is desired that ActiveModel::Dirty is to be part of the Base class by default, as is the case for the Model class in Her. I see that #104 provides a minimal fix as not to clash with ActiveModel::Dirty, but could we bring these features into the base model?

Our current implementation uses an generic class/model to arrange these things:

class Model < Spyke::Base 
  include ActiveModel::Dirty

  def self.attribute(name)
    define_attribute_methods(name) # ActiveModel::Dirty
    attributes(name) # Spyke
    define_method(:"#{name}=") do |value|
      send(:"#{name}_will_change!") unless value == attribute(name) # ActiveModel::Dirty
      super(value) # Spyke
     end
   end

  def initialize(*args)
    super
    clear_changes_information
  end

  def reload
    result = super
    clear_changes_information if result
    result
  end  
       
  def rollback
    restore_attributes
  end
     
  def save
    return true unless changed?
    result = super
    changes_applied if result
    result
  end

end

class Foo < Model
  attribute :bar
  attribute :baz
end

foo = Foo.find(1)
foo.bar = "test"
foo.rollback
foo.save # Does nothing

foo.baz = "baz"
foo.reload
foo.save # Does nothing
# etc...

Having this would enormously boost performance because #save would only really go via HTTP if attribute are changed, and a rollback is possible without doing a reload over HTTP.

Is the above implementation all right or do we miss some corner cases? Would you like us to draw up a PR?

  • 👍1
  • ❤️1
balvig commented

@paulvt awesome that you've gotten it to work!

I'm not entirely sure this should be built in to Spyke though... 🙇

I believe depending on the API there may be cases where you do want an HTTP request to be triggered, even if no attributes changed, and we can't universally say that we would always prefer the one less request (say if for example there is some logic on the API side that updates a timestamp whenever a record is saved).

Similarly, for reload, the ActiveRecord equivalent would load the record from the DB, where it might have changed due to others interacting with the app, so depending on the use case, not getting the latest changes from the API might come as a surprise 😓.

johan-smits commented

We added the implementation that @paulvt suggested.

  • 👍1