From 3546cd026f0130330f32e56937da3236dbe7ec4e Mon Sep 17 00:00:00 2001
From: Tommaso Pavese <tommaso.pavese@zendesk.com>
Date: Tue, 4 Oct 2022 11:17:57 +0200
Subject: [PATCH 1/5] Document how to run tests locally

---
 README.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/README.md b/README.md
index a843618..891ce8c 100644
--- a/README.md
+++ b/README.md
@@ -159,6 +159,14 @@ add_index :account_settings, [ :account_id, :name ], :unique => true
 * ActiveRecord
 * ActiveSupport
 
+## Running the tests locally
+
+```shell
+export BUNDLE_GEMFILE="gemfiles/rails5.1.gemfile"
+bundle -j 6
+bundle exec rspec
+```
+
 ## License and copyright
 
 Copyright 2013 Zendesk

From df460f663c9535531b928b8fa3793990008dc821 Mon Sep 17 00:00:00 2001
From: Tommaso Pavese <tommaso.pavese@zendesk.com>
Date: Tue, 4 Oct 2022 11:19:30 +0200
Subject: [PATCH 2/5] Use pry

---
 property_sets.gemspec | 1 +
 spec/spec_helper.rb   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/property_sets.gemspec b/property_sets.gemspec
index 5ef6544..4cd4460 100644
--- a/property_sets.gemspec
+++ b/property_sets.gemspec
@@ -13,6 +13,7 @@ Gem::Specification.new "property_sets", PropertySets::VERSION do |s|
   s.add_runtime_dependency("activerecord", ">= 4.2", "< 6.2")
   s.add_runtime_dependency("json")
 
+  s.add_development_dependency("pry")
   s.add_development_dependency("bump")
   s.add_development_dependency("rake")
   s.add_development_dependency('actionpack')
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 093c1da..823a563 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -10,6 +10,8 @@
 require 'support/account'
 require 'support/thing'
 
+require 'pry'
+
 I18n.enforce_available_locales = false
 
 # http://stackoverflow.com/questions/5490411/counting-the-number-of-queries-performed/43810063#43810063

From ac48c3abefabce769dff4c0338fa5bdaa3795002 Mon Sep 17 00:00:00 2001
From: Tommaso Pavese <tommaso.pavese@zendesk.com>
Date: Sat, 1 Oct 2022 00:11:50 +0200
Subject: [PATCH 3/5] Extract a helper to read a value

---
 lib/property_sets/active_record_extension.rb | 18 +++-----
 lib/property_sets/value_reader.rb            | 47 ++++++++++++++++++++
 2 files changed, 52 insertions(+), 13 deletions(-)
 create mode 100644 lib/property_sets/value_reader.rb

diff --git a/lib/property_sets/active_record_extension.rb b/lib/property_sets/active_record_extension.rb
index 7bb36e8..7107380 100644
--- a/lib/property_sets/active_record_extension.rb
+++ b/lib/property_sets/active_record_extension.rb
@@ -1,5 +1,6 @@
 require 'active_record'
 require 'property_sets/casting'
+require 'property_sets/value_reader'
 require 'set'
 
 module PropertySets
@@ -155,19 +156,10 @@ def lookup_without_default(arg)
       end
 
       def lookup_value(type, key)
-        serialized = property_serialized?(key)
-
-        if instance = lookup_without_default(key)
-          instance.value_serialized = serialized
-          PropertySets::Casting.read(type, instance.value)
-        else
-          value = association_class.default(key)
-          if serialized
-            PropertySets::Casting.deserialize(value)
-          else
-            PropertySets::Casting.read(type, value)
-          end
-        end
+        instance = lookup_without_default(key)
+        PropertySets::ValueReader.new(
+          property_name: key, assoc_instance: instance, assoc_class: association_class
+        ).read
       end
 
       # The finder method which returns the property if present, otherwise a new instance with defaults
diff --git a/lib/property_sets/value_reader.rb b/lib/property_sets/value_reader.rb
new file mode 100644
index 0000000..a8ed39d
--- /dev/null
+++ b/lib/property_sets/value_reader.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+module PropertySets
+  class ValueReader
+    # Build a new value reader for a property set record.
+    #
+    # @param property_name [Symbol]
+    # @param assoc_instance [ActiveRecord::Base]
+    # @param assoc_class [Class]
+    #
+    def initialize(property_name:, assoc_instance:, assoc_class:)
+      @name = property_name
+      @instance = assoc_instance
+      @assoc_class = assoc_class
+    end
+
+    # Returns the value of the property that is part of a property set.
+    # It takes into account the property type (e.g. whether it's serialized)
+    # and performs the required casting and parsing.
+    #
+    # @return [Object, nil]
+    #
+    def read
+      serialized = property_serialized?
+
+      if @instance
+        @instance.value_serialized = serialized
+        PropertySets::Casting.read(type, @instance.value)
+      else
+        value = @assoc_class.default(@name)
+        if serialized
+          PropertySets::Casting.deserialize(value)
+        else
+          PropertySets::Casting.read(type, value)
+        end
+      end
+    end
+
+    def property_serialized?
+      type == :serialized
+    end
+
+    def type
+      @type ||= @assoc_class.type(@name)
+    end
+  end
+end

From 437248e38b3214e8deaf7c995b0eb4818aa7f892 Mon Sep 17 00:00:00 2001
From: Tommaso Pavese <tommaso.pavese@zendesk.com>
Date: Tue, 4 Oct 2022 14:14:37 +0200
Subject: [PATCH 4/5] Introduce an association wrapper for the property set
 collection.

---
 lib/property_sets/active_record_extension.rb |  69 ++++-
 lib/property_sets/association_wrapper.rb     | 262 +++++++++++++++++++
 spec/property_sets_spec.rb                   |   6 +-
 3 files changed, 334 insertions(+), 3 deletions(-)
 create mode 100644 lib/property_sets/association_wrapper.rb

diff --git a/lib/property_sets/active_record_extension.rb b/lib/property_sets/active_record_extension.rb
index 7107380..d4193e8 100644
--- a/lib/property_sets/active_record_extension.rb
+++ b/lib/property_sets/active_record_extension.rb
@@ -1,4 +1,5 @@
 require 'active_record'
+require 'property_sets/association_wrapper'
 require 'property_sets/casting'
 require 'property_sets/value_reader'
 require 'set'
@@ -48,8 +49,48 @@ def property_set(association, options = {}, &block)
           has_many association, **hash_opts do
             # keep this damn block! -- creates association_module below
           end
+
+          # Inject our association wrapper so that we can control how the association
+          # is accessed and loaded from the DB. This allows us to not load the entire
+          # dataset from the DB when we only care about a single property set entry.
+          #
+          # Alias the normal has_many association method.
+          #
+          alias_method "#{association}_without_wrapper", association
+          #
+          # An ivar to cache the association wrapper. This has to be scoped with the
+          # association name in order to support multiple uses of property sets
+          # within the same AR model.
+          #
+          association_wrapper_memo_name = prop_set_assoc_wrapper_memo_name(association)
+          #
+          # Redefine the association method.
+          # The AssociationProxy will use the original (aliased) association
+          # method when required.
+          #
+          define_method association do |**opts|
+            memo = instance_variable_get(association_wrapper_memo_name) and return memo
+
+            wrapper = PropertySets::AssociationWrapper.new(
+              owner_instance: self,
+              assoc_name: association,
+              assoc_class: property_class,
+              opts: opts
+            )
+            instance_variable_set(association_wrapper_memo_name, wrapper)
+
+            wrapper
+          end
+
+          # To replicate ":autosave => true" on the has_many association.
+          before_save -> do
+            public_send(association).save
+          end
         end
 
+        # Open the association module defined with the has_many declaration (above)
+        # and define custom accessors.
+
         # eg 5: AccountSettingsAssociationExtension
         # eg 6: Account::SettingsAssociationExtension
 
@@ -97,6 +138,11 @@ def property_set(association, options = {}, &block)
           end
         end
       end
+
+
+      def prop_set_assoc_wrapper_memo_name(association)
+        "@prop_set_assoc_wrapper_#{association}"
+      end
     end
 
     module AssociationExtensions
@@ -155,7 +201,7 @@ def lookup_without_default(arg)
         detect { |property| property.name.to_sym == arg.to_sym }
       end
 
-      def lookup_value(type, key)
+      def lookup_value(_type, key)
         instance = lookup_without_default(key)
         PropertySets::ValueReader.new(
           property_name: key, assoc_instance: instance, assoc_class: association_class
@@ -176,6 +222,13 @@ def lookup(arg)
 
       # This finder method returns the property if present, otherwise a new instance with the default value.
       # It does not have the side effect of adding a new setting object.
+      #
+      # Among the "lookup" methods, this alone is not used internally in the gem.
+      # It's part on the undocumented public API, and application code can make
+      # use of it to try to fetch a property record or an unpersisted default
+      # record, _without adding it to the association collection_, which means
+      # that the returned default will not risk to be saved.
+      #
       def lookup_or_default(arg)
         instance = lookup_without_default(arg)
         instance ||= association_class.new(:value => association_class.raw_default(arg))
@@ -219,8 +272,22 @@ def update_columns(attributes)
         super attributes
       end
 
+      # Remove all the memoized Association Proxies
+      def reload(*args)
+        clear_all_property_set_caches
+        super(*args)
+      end
+
       private
 
+      def clear_all_property_set_caches
+        self.class.property_set_index.each do |association|
+          memo_ivar = self.class.prop_set_assoc_wrapper_memo_name(association)
+          next unless instance_variable_defined?(memo_ivar)
+          remove_instance_variable(memo_ivar)
+        end
+      end
+
       def delegated_property_sets?
         self.class.respond_to?(:delegated_property_set_attributes)
       end
diff --git a/lib/property_sets/association_wrapper.rb b/lib/property_sets/association_wrapper.rb
new file mode 100644
index 0000000..1037f28
--- /dev/null
+++ b/lib/property_sets/association_wrapper.rb
@@ -0,0 +1,262 @@
+# frozen_string_literal: true
+
+require "property_sets/casting"
+require "property_sets/value_reader"
+
+module PropertySets
+  # A wrapper for the underlying has_many ActiveRecord association that represents
+  # the property set. This class intercepts query methods that would be injected
+  # into the association, and implements that interface contract with its own
+  # implementation. Its goal is to provide a more lightweight form of DB access.
+  #
+  class AssociationWrapper
+    # Build a new association wrapper for the property set.
+    #
+    # @param owner_instance [ActiveRecord::Base]
+    # @param assoc_name [Symbol]
+    # @param assoc_class [Class]
+    # @param opts [Hash]
+    #
+    def initialize(owner_instance:, assoc_name:, assoc_class:, opts:)
+      @owner_instance = owner_instance
+      @assoc_name = assoc_name
+      @assoc_class = assoc_class
+      @opts = opts
+      @loaded_records = {}
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    # Accepts an array of names as strings or symbols and returns a hash.
+    #
+    # @param keys [Array<Symbol, String>]
+    # @return [HashWithIndifferentAccess]
+    #
+    def get(keys = [])
+      property_keys = if keys.empty?
+        @assoc_class.keys
+      else
+        @assoc_class.keys & keys.map(&:to_s)
+      end
+
+      property_pairs = property_keys.map do |name|
+        value = _lookup_value(name)
+        [name, value]
+      end.flatten(1)
+
+      HashWithIndifferentAccess[*property_pairs]
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    # Accepts a name value pair hash { :name => 'value', :pairs => true } and builds a property for each key
+    #
+    # @param property_pairs [Hash]
+    # @param with_protection [Boolean]
+    # @return [Array<Symbol]
+    #
+    def set(property_pairs, with_protection = false)
+      property_pairs.keys.each do |name|
+        record = lookup(name)
+        if with_protection && record.protected?
+          @assoc_class.logger.warn("Someone tried to update the protected #{name} property to #{property_pairs[name]}")
+        else
+          set_property(name, property_pairs[name])
+        end
+      end
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def save(*args)
+      @loaded_records.each do |prop_name, instance|
+        instance.save(*args) if instance&.changed?
+      end
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def save!(*args)
+      @loaded_records.each do |prop_name, instance|
+        instance.save!(*args) if instance&.changed?
+      end
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def protected?(arg)
+      lookup(arg).protected?
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def enable(arg)
+      set_property(arg, "1")
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def disable(arg)
+      set_property(arg, "0")
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    # Returns a record
+    #
+    def lookup_without_default(name)
+      @loaded_records[name.to_sym] ||= underlying_association.find_by(name: name)
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def lookup_value(_type, name)
+      _lookup_value(name)
+    end
+
+    # The signature of the original `lookup_value` expects the property type, which
+    # is not required with this implementation (it's retrieved in a different way).
+    #
+    def _lookup_value(name)
+      PropertySets::ValueReader.new(
+        property_name: name,
+        assoc_instance: lookup_without_default(name),
+        assoc_class: @assoc_class
+      ).read
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    def lookup(name)
+      instance = lookup_without_default(name)
+
+      instance ||= underlying_association.build(
+        name: name.to_sym,
+        value: @assoc_class.raw_default(name)
+      )
+
+      instance.value_serialized = property_serialized?(name)
+      instance.public_send("#{@assoc_class.owner_class_sym}=", @owner_instance) if @owner_instance.new_record?
+
+      @loaded_records[name.to_sym] ||= instance
+
+      instance
+    end
+
+    # Adopted from `PropertySets::ActiveRecordExtension::AssociationExtensions`.
+    #
+    # This finder method returns the property if present, otherwise a new instance with the default value.
+    # It does not have the side effect of adding a new setting object.
+    #
+    # Among the "lookup" methods, this alone is not used internally in the gem.
+    # It's part on the undocumented public API, and application code can make
+    # use of it to try to fetch a property record or an unpersisted default
+    # record, _without adding it to the association collection_, which means
+    # that the returned default will not risk to be saved.
+    #
+    def lookup_or_default(name)
+      instance = lookup_without_default(arg)
+      instance ||= @assoc_class.new(value: @assoc_class.raw_default(arg))
+      instance.value_serialized = property_serialized?(arg)
+      instance
+    end
+
+    # Required to make the change-tracking Delegator methods work.
+    #
+    def loaded?
+      @loaded_records.any?
+    end
+
+    private
+
+    # The main entry point for this class.
+    # It's meant to intercept attempts to access the property set collection by
+    # key (property name), and in that case try to only load the relevant records.
+    #
+    # It delegates anything else to the underlying association.
+    #
+    def method_missing(meth_name, *meth_args, &block)
+      prop_name, suffix = recognize_property_method(meth_name)
+
+      if prop_name
+        handle_property_access(prop_name, suffix, meth_args)
+      else
+        # Delegate to the underlying association.
+        underlying_association.public_send(meth_name, *meth_args, &block)
+      end
+    end
+
+    # The underlying ActiveRecord has_many association.
+    #
+    def underlying_association
+      @underlying_association ||= @owner_instance.public_send(
+        "#{@assoc_name}_without_wrapper", **@opts
+      )
+    end
+
+    def properties
+      @properties ||= @assoc_class.properties
+    end
+
+    def is_property?(name)
+      properties.include? name
+    end
+
+    def check_predicate(name)
+      !PropertySets::Casting::FALSE.include?(
+        lookup_value(nil, name).to_s.downcase
+      )
+    end
+
+    def set_property(name, value)
+      instance = lookup(name)
+      instance.value = PropertySets::Casting.write(@assoc_class.type(name), value)
+
+      @loaded_records[name.to_sym] = instance
+
+      instance.value
+    end
+
+    def handle_property_access(name, suffix, args)
+      case suffix
+      when "="      then set_property(name, args[0])
+      when "?"      then check_predicate(name)
+      when "record" then lookup(name)
+      when nil      then _lookup_value(name)
+      else
+        raise NoMethodError, "unrecognized property access (#{name}#{suffix})"
+      end
+    end
+
+    def respond_to_missing?(meth_name, include_private = false)
+      underlying_association.respond_to?(meth_name) or super
+    end
+
+    PROPERTY_ACCESS_REGEX = %r{\A(\w+)(\=|\?)?\z}.freeze
+    PROPERTY_RECORD_REGEX = %r{_record\z}.freeze
+
+    # Takes in a method selector and checks whether it's a property set access
+    # attempt. If it is, it returns the property name and the method suffix, e.g.
+    # "?", "=" or "_record." A missing suffix means that the caller wants to read
+    # the property set value for this key.
+    #
+    def recognize_property_method(meth)
+      md = PROPERTY_ACCESS_REGEX.match(meth.to_s)
+      return nil unless md
+
+      prop_name, suffix = md[1], md[2]
+      return nil unless is_property?(prop_name)
+
+      if suffix.nil? && PROPERTY_RECORD_REGEX === prop_name
+        suffix = "record"
+        prop_name.sub!(PROPERTY_RECORD_REGEX, "")
+      end
+
+      [prop_name, suffix]
+    end
+
+    def property_serialized?(name)
+      @assoc_class.type(name) == :serialized
+    end
+  end
+end
diff --git a/spec/property_sets_spec.rb b/spec/property_sets_spec.rb
index 6c9788a..747457b 100644
--- a/spec/property_sets_spec.rb
+++ b/spec/property_sets_spec.rb
@@ -97,12 +97,12 @@ class AnotherThing < ActiveRecord::Base
 
   describe 'querying for a setting that does not exist' do
     before do
-      expect(account.settings).to eq([])
+      expect(account.settings.to_a).to eq([])
       expect(account.settings.hep?).to be true
     end
 
     it 'not add a new setting' do
-      expect(account.settings).to eq([])
+      expect(account.settings.to_a).to eq([])
     end
 
     it 'give back the default value' do
@@ -263,6 +263,8 @@ class AnotherThing < ActiveRecord::Base
         { :id => account.texts.lookup(:bar).id, :name => "bar", :value => "1" }
       ])
 
+      account.reload
+
       expect(account.texts.foo).to eq("0")
       expect(account.texts.bar).to eq("1")
     end

From a04c7e44ac273bfeaaafd6c841b09fe778ede10e Mon Sep 17 00:00:00 2001
From: Tommaso Pavese <tommaso.pavese@zendesk.com>
Date: Tue, 4 Oct 2022 17:44:32 +0200
Subject: [PATCH 5/5] Fix issue with handling of _record suffix in prop set
 access

---
 lib/property_sets/association_wrapper.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/property_sets/association_wrapper.rb b/lib/property_sets/association_wrapper.rb
index 1037f28..ad3bbc1 100644
--- a/lib/property_sets/association_wrapper.rb
+++ b/lib/property_sets/association_wrapper.rb
@@ -245,13 +245,15 @@ def recognize_property_method(meth)
       return nil unless md
 
       prop_name, suffix = md[1], md[2]
-      return nil unless is_property?(prop_name)
+      return nil if prop_name.nil?
 
       if suffix.nil? && PROPERTY_RECORD_REGEX === prop_name
         suffix = "record"
         prop_name.sub!(PROPERTY_RECORD_REGEX, "")
       end
 
+      return nil unless is_property?(prop_name)
+
       [prop_name, suffix]
     end