Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: saving association changes #19

Closed
ezubairov opened this issue Jan 17, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@ezubairov
Copy link

commented Jan 17, 2017

What

Sometimes it is really handy to save data describing changes in associated objects, but there is no gem for that. PaperTrail tries to do that, but that's an experimental feature, which is not working very well.

Why

If i had something like:

class User < ActiveRecord::Base
  has_many :attachments

end

class Attachment < ActiveRecord::Base
  belongs_to :user

end

and user uploads a new attachment, there is not going to be meaningful log data that i can use later.
Best case scenario: i'll get log_data with timestamps (which are going to be blacklisted most likely) as the only changed attribute.
But i'd like to have an ability to build a changelog or to revert to any previous version of User including matching versions of associated objects.

How

Unfortunately, i don't have a clear singular solution, hence feature request, instead of pull request.
If i had to implement an ad-hoc solution, i'd just serialize every associated object and treat them as nested attributes, but i am not really sure if that's the best way of doing that.

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 17, 2017

@ezubairov Thanks for the request!

I was thinking about this feature too, but was not sure whether it's useful or not. Now I see – it is.

So, I'll share the task on http://cultofmartians.com with some thoughts on implementation; I think, we'll find someone to handle this)

@palkan

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2017

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2017

@palkan at last I found time to dig through this task

General question:

I read your implementation tips on the Cult of Martians site. Do you want to somehow override ActiveRecord associations getters? Or you meant some other approach?

@palkan

This comment has been minimized.

Copy link
Owner

commented Feb 6, 2017

I read your implementation tips on the Cult of Martians site. Do you want to somehow override ActiveRecord associations getters? Or you meant some other approach?

I think, we can patch load_target method to modify loaded items.

Also, to reduce global monkey-patching, we can patch association instances in-place, prepending some module into an instance of association in association method.

Smth like:

# this method is called on our model object, which `has_logidze`
def association(*args)
  res = super
  # only patch association if we request some past state
  res.singleton_class.prepend Logidze::VersionedAssociation if in_the_past?
  res
end
@charlie-wasp

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2017

@palkan hello, I think, I got it at last. Got it somewhere at least :)

It took me a while to get familiar with Rails inner association logic, I spent some time tinkering with it to just understand the clues you gave :)

Unfortunately, there is a trouble with the collection associations, particularly with the size method. It doesn't call load_target, if target wasn't loaded in memory already, it executes SQL count statement instead (as far as I understand it). So we can't hook into that with load_target.

May I open the pull request, so we could discuss the actual code?

@palkan

This comment has been minimized.

Copy link
Owner

commented Feb 16, 2017

May I open the pull request, so we could discuss the actual code?

Yes, of course.

... particularly with the size method

We can ignore such methods at first (and add a note about it to the README).
But that's an important point, thanks for figuring it out! 👍

@NullVoxPopuli

This comment has been minimized.

Copy link

commented Feb 17, 2017

huh, this could be handy, depending on how well it behaves. Here is what I've come up with in the mean time.

Having issues with weird log_data in tests though... like overly nested

 {"c"=>
         {"h"=>
           [{"c"=>
# frozen_string_literal: true
#
# This is for manually creating versions using logdize's technique
#
# The automatic version on update has been disabled (but could be enabled by
# running a migration that updated the trigger).
# This is to ensure that any version, other than the first version, is
# always created by our code.
# It also means that we can use the same version for every association -- which
# simplifies relations
#
# Note that the latest version on the log_data will always be the most recent
# snapshot, and may not reflect the current state
# So, record.log_version will always be one less than the number of versions.
# E.g.: record.log_size # => 3 -- but the total versions are 3 + [current]
#
# NOTE: gem documentation: https://github.com/palkan/logidze
module Versionable
  extend ActiveSupport::Concern

  included do
    has_logidze

    # set the initial version
    # after_create :save_version!
    # - this happens via SQL TRIGGER
    #
    # increment version
    # before_save :save_version!
    # - this is not automatic, because we need to
    #   have a consistent version across relationships

    # Class Methods
    class << self
      # for retreiving a relationship of this (whatever includes this module)
      # requiring each member to match the provided version number or
      # datetime of the version
      #
      # @param [Number] version
      # @param [Time] time
      def at(version: nil, time: nil)
        # TODO: find a more efficient way to do this
        all.map do |current|
          time ? current.at!(time) : current.at_version!(version)
        end
      end

      private

      # Iterate over the associations, and add a method that will retreive
      # the relationship at the same version as the calling object.
      # That way we don't need to manually carry the version through our
      # relation call chain.
      #
      # More information on reflections
      # http://api.rubyonrails.org/classes/ActiveRecord/Reflection/ClassMethods.html#method-i-reflections
      def build_versioned_reflections!
        reflections.each do |association_name, reflection|
          # is this a versioned reflection?
          next unless reflection.klass.has_attribute?(:log_data)

          define_method("#{association_name}_at_same_version") do
            send(association_name).at(time: time_of_version)
          end
        end
      end
    end

    # ...Metaprogramming... Wooosh
    build_versioned_reflections!
  end

  # @return [NilClass|Time]
  def time_of_version
    ts = log_data&.find_by_version(version)&.data&.fetch('ts')
    Time.at(ts) if ts
  end

  def save_version!
    save_version
    reload
  end

  # For saving logdize historical versions manually.
  # Be sure that logdize has logging turned off globally
  #
  # TODO: have configurable parameter exclusions, such as draft_id
  def save_version
    return false unless has_attribute?(:log_data)
    # return false unless log_data.present?

    klass = self.class
    klass.connection.execute(%(
      SELECT make_snapshot(#{id}, '#{klass.table_name}');
    ))
  end

  # Slightly shorter than log_version, and won't error if there isn't (yet) log_data.
  # log_data is only populated on save, before executing the insert or update statement
  # on the db side
  def version
    log_data ? log_version : 0
  end
end
@charlie-wasp

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

@palkan what we are going to do with this issue?

@charlie-wasp

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

@ezubairov hello!

Today PR, addressing this issue, was merged, you can read about it in the wiki page

@palkan palkan closed this Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.