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

Loading associated temporal records only ever works once #25

Open
Zyndoras opened this issue Feb 3, 2023 · 3 comments
Open

Loading associated temporal records only ever works once #25

Zyndoras opened this issue Feb 3, 2023 · 3 comments

Comments

@Zyndoras
Copy link

Zyndoras commented Feb 3, 2023

... per table, I guess.

In a one-to-many association of two temporal tables, loading the parent (history) record of the child (history) record works exactly once (per runtime) for me, and never again.

Example:

task demo: :environment do
  ActiveRecord::Base.logger = Logger.new STDOUT

  puts 'POST 1:', post_of_comment.inspect # => Outputs the correct PostHistory
  puts 'POST 2:', post_of_comment.inspect # => Outputs nil
end

def post_of_comment
  post = Post.create
  comm = Comment.create(post: post)

  comm.history.at(Time.current).first.post
end

Expectation:
Each call of post_of_comment should find a PostHistory.

Current behavior:
The first attempt succeeds, all other attempts return nil.

This reproduction case is on Github at Zyndoras/temporal_tables_demo (see lib/tasks/demo.rake)

It seems to me that the timestamp used to load the PostHistory is somewhat arbitrary and does not necessarily match anything related to the records created or the timestamp passed to history.at.

What I do not understand though, is why it consistently works the first time the function is called, but then never again.

@evman182
Copy link
Collaborator

evman182 commented Feb 5, 2023

I've run into a likely related issue. There's something weird going on with the activerecord statement cache where it can't tell the difference between history queries of the same model. I haven't had a chance to dig in deep yet, but my solution so far has been a monkeypatch to disable it for History records

module TemporalPatch
    class << self
      def apply_patch
        puts 'asdfasdf'
        const = find_const
        mtd = find_method(const)
  
        # make sure the class we want to patch exists;
        # make sure the #filter method exists and accepts exactly one argument
        unless const && mtd&.arity == 1
          raise 'Could not find class or method when patching ' \
                "ActiveRecord::Associations:Association's skip_statement_cache method. Please investigate."
        end
        const.prepend(InstanceMethods)
      end
  
      private
  
      def find_const
        Kernel.const_get('ActiveRecord::Associations::Association')
      rescue NameError
        # return nil if the constant doesn't exist
      end
  
      def find_method(const)
        return unless const
  
        const.instance_method(:skip_statement_cache?)
      rescue NameError
        # return nil if the constant doesn't exist
      end
    end
  
    module InstanceMethods
      private
  
      def skip_statement_cache?(scope)
        klass.name.end_with?('History') ||
          reflection.has_scope? ||
          scope.eager_loading? ||
          klass.scope_attributes? ||
          reflection.source_reflection.active_record.default_scopes.any?
      end
    end
  end
  
  TemporalPatch.apply_patch
  

@Zyndoras
Copy link
Author

Zyndoras commented Feb 6, 2023

Definitely related. Your patch makes my tests green.

I'll go with this for now:

def skip_statement_cache?(scope)
  klass.is_a?(TemporalTables::TemporalClass::ClassMethods) || super(scope)
end

@evman182
Copy link
Collaborator

monkey-patch implemented in v3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants