From 67bd03d0e3af05a325787e840a858c0ac8948a3c Mon Sep 17 00:00:00 2001
From: Beber <j.garibotti@epicery.fr>
Date: Wed, 16 Nov 2022 17:34:13 +0100
Subject: [PATCH] Fix call context when a preference default is a proc

Before v3.1, the proc given as the default value in a preference
had its context bound to the initialized instance.
That allowed, for example, referencing other preferences.

From v3.1., the context is bound to the class.

We are coming back to the behavior by calling the proc with
`instance_exec`

The test changed because initializing `preferences` with
`default_preferences` cause an infinite loop.

This is not a regression as it's the expected behavior on anterior
versions but added some information on the comments.

Change Preference

Context:
- `class A` is declared once in a `before(:all)` so the class WILL NOT be destroyed/recreate for each test
- `@a = A.new` is in a `before(:each)` therefore the variable is initialized before the test start
- The 4 examples from l.285 define the same preference `:secret` but the last one is the only using a default

If the test run first:
- `@a` is instantiated without `preference :secret`
- `@a.preferences` looks like `{:color=>"green"}`
- We define the `preference :secret` with a default
- Calling `@a.get_preference(:secret)` will work because it will look for the default

If the test doesn't run first:
- `@a` is instantiated with a `preference :secret` without `:default`
- `@a.preferences` looks like `{:color=>"green", :secret=>"rJ0s--jPH2d2iyyB9KseLU--HbYfXFtRB36+yj9L3fSDFA=="}
` (`:secret` value is `nil` but encrypted)
- We override `preference :secret` with a default
- `@a.get_preference(:secret)` returns `nil` as it's initialized and does not look for the default

Setting `@a = A.new` after the override works because it'll be initialized with the new default

To fix it we declared the class as anonymous and create them only when needed.
---
 core/lib/spree/preferences/preferable.rb      |   5 +
 .../preferences/preferable_class_methods.rb   |   4 +-
 .../spree/preferences/preferable_spec.rb      | 211 ++++++++++--------
 3 files changed, 121 insertions(+), 99 deletions(-)

diff --git a/core/lib/spree/preferences/preferable.rb b/core/lib/spree/preferences/preferable.rb
index 5753fdbc7f..86da3aa2f4 100644
--- a/core/lib/spree/preferences/preferable.rb
+++ b/core/lib/spree/preferences/preferable.rb
@@ -10,6 +10,9 @@ module Preferences
     #
     # A class including Preferable must implement #preferences which should return
     # an object responding to .fetch(key), []=(key, val), and .delete(key).
+    # If #preferences is initialized with `default_preferences` and one of the
+    # preferences is another preference, it will cause a stack level too deep error.
+    # To avoid it do not memoize #preferences.
     #
     # It may also define a `#context_for_default` method. It should return an
     # array with the arguments to be provided to a proc used as the `default:`
@@ -111,6 +114,8 @@ def defined_preferences
       end
 
       # @return [Hash{Symbol => Object}] Default for all preferences defined on this class
+      # This may raise an infinite loop error if any of the defaults are
+      # dependent on other preferences defaults.
       def default_preferences
         Hash[
           defined_preferences.map do |preference|
diff --git a/core/lib/spree/preferences/preferable_class_methods.rb b/core/lib/spree/preferences/preferable_class_methods.rb
index 4da470c0c9..2541619763 100644
--- a/core/lib/spree/preferences/preferable_class_methods.rb
+++ b/core/lib/spree/preferences/preferable_class_methods.rb
@@ -75,7 +75,7 @@ def preference(name, type, options = {})
       # is a pending preference before going to default
       define_method preference_getter_method(name) do
         value = preferences.fetch(name) do
-          default.call(*context_for_default)
+          instance_exec(*context_for_default, &default)
         end
         value = preference_encryptor.decrypt(value) if preference_encryptor.present?
         value
@@ -92,7 +92,7 @@ def preference(name, type, options = {})
       end
 
       define_method preference_default_getter_method(name) do
-        default.call(*context_for_default)
+        instance_exec(*context_for_default, &default)
       end
 
       define_method preference_type_getter_method(name) do
diff --git a/core/spec/models/spree/preferences/preferable_spec.rb b/core/spec/models/spree/preferences/preferable_spec.rb
index 25addfa03f..1064ae9246 100644
--- a/core/spec/models/spree/preferences/preferable_spec.rb
+++ b/core/spec/models/spree/preferences/preferable_spec.rb
@@ -3,105 +3,110 @@
 require 'rails_helper'
 
 RSpec.describe Spree::Preferences::Preferable, type: :model do
-  before :all do
-    class A
+  let(:config_class_a) do
+    Class.new do
       include Spree::Preferences::Preferable
       attr_reader :id
 
       def initialize
         @id = rand(999)
+        initialize_preference_defaults
       end
 
       def preferences
-        @preferences ||= default_preferences
+        @preferences ||= {}
+      end
+
+      def initialize_preference_defaults
+        @preferences = default_preferences
       end
 
       preference :color, :string, default: 'green'
     end
+  end
 
-    class B < A
+  let(:config_class_b) do
+    Class.new(config_class_a) do
       preference :flavor, :string
     end
   end
 
-  before :each do
-    @a = A.new
-    @b = B.new
-  end
+  let(:a) { config_class_a.new }
+  let(:b) { config_class_b.new }
 
   describe "preference definitions" do
     it "parent should not see child definitions" do
-      expect(@a.has_preference?(:color)).to be true
-      expect(@a.has_preference?(:flavor)).not_to be true
+      expect(a.has_preference?(:color)).to be true
+      expect(a.has_preference?(:flavor)).not_to be true
     end
 
     it "child should have parent and own definitions" do
-      expect(@b.has_preference?(:color)).to be true
-      expect(@b.has_preference?(:flavor)).to be true
+      expect(b.has_preference?(:color)).to be true
+      expect(b.has_preference?(:flavor)).to be true
     end
 
     it "instances have defaults" do
-      expect(@a.preferred_color).to eq 'green'
-      expect(@b.preferred_color).to eq 'green'
-      expect(@b.preferred_flavor).to be_nil
+      expect(a.preferred_color).to eq 'green'
+      expect(b.preferred_color).to eq 'green'
+      expect(b.preferred_flavor).to be_nil
     end
 
     it "can be asked if it has a preference definition" do
-      expect(@a.has_preference?(:color)).to be true
-      expect(@a.has_preference?(:bad)).to be false
+      expect(a.has_preference?(:color)).to be true
+      expect(a.has_preference?(:bad)).to be false
     end
 
     it "can be asked and raises" do
       expect {
-        @a.has_preference! :flavor
+        a.has_preference! :flavor
       }.to raise_error(NoMethodError, "flavor preference not defined")
     end
 
     it "has a type" do
-      expect(@a.preferred_color_type).to eq :string
-      expect(@a.preference_type(:color)).to eq :string
+      expect(a.preferred_color_type).to eq :string
+      expect(a.preference_type(:color)).to eq :string
     end
 
     it "has a default" do
-      expect(@a.preferred_color_default).to eq 'green'
-      expect(@a.preference_default(:color)).to eq 'green'
+      expect(a.preferred_color_default).to eq 'green'
+      expect(a.preference_default(:color)).to eq 'green'
     end
 
     it "raises if not defined" do
       expect {
-        @a.get_preference :flavor
+        a.get_preference :flavor
       }.to raise_error(NoMethodError, "flavor preference not defined")
     end
   end
 
   describe "preference access" do
     it "handles ghost methods for preferences" do
-      @a.preferred_color = 'blue'
-      expect(@a.preferred_color).to eq 'blue'
+      a.preferred_color = 'blue'
+      expect(a.preferred_color).to eq 'blue'
     end
 
     it "parent and child instances have their own prefs" do
-      @a.preferred_color = 'red'
-      @b.preferred_color = 'blue'
+      a.preferred_color = 'red'
+      b.preferred_color = 'blue'
 
-      expect(@a.preferred_color).to eq 'red'
-      expect(@b.preferred_color).to eq 'blue'
+      expect(a.preferred_color).to eq 'red'
+      expect(b.preferred_color).to eq 'blue'
     end
 
     it "raises when preference not defined" do
       expect {
-        @a.set_preference(:bad, :bone)
+        a.set_preference(:bad, :bone)
       }.to raise_exception(NoMethodError, "bad preference not defined")
     end
 
     it "builds a hash of preferences" do
-      @b.preferred_flavor = :strawberry
-      expect(@b.preferences[:flavor]).to eq 'strawberry'
-      expect(@b.preferences[:color]).to eq 'green' # default from A
+      b.preferred_flavor = :strawberry
+      expect(b.preferences[:flavor]).to eq 'strawberry'
+      expect(b.preferences[:color]).to eq 'green' # default from A
     end
 
     it "builds a hash of preference defaults" do
-      expect(@b.default_preferences).to eq({
+      expect(b.default_preferences).to eq({
         flavor: nil,
         color: 'green'
       })
@@ -151,158 +156,170 @@ def self.allowed_admin_form_preference_types
       end
     end
 
+    context "when a default is a proc" do
+      before do
+        config_class_a.preference(:context, :string, default: -> { preferred_color })
+      end
+
+      it "calls it from the instance" do
+        expect(a.preferred_context).to eq("green")
+      end
+    end
+
     context "converts integer preferences to integer values" do
       before do
-        A.preference :is_integer, :integer
+        config_class_a.preference :is_integer, :integer
       end
 
       it "with strings" do
-        @a.set_preference(:is_integer, '3')
-        expect(@a.preferences[:is_integer]).to eq(3)
+        a.set_preference(:is_integer, '3')
+        expect(a.preferences[:is_integer]).to eq(3)
 
-        @a.set_preference(:is_integer, '')
-        expect(@a.preferences[:is_integer]).to eq(0)
+        a.set_preference(:is_integer, '')
+        expect(a.preferences[:is_integer]).to eq(0)
       end
 
       it 'does not convert if value is nil' do
-        @a.set_preference(:is_integer, nil)
-        expect(@a.preferences[:is_integer]).to be_nil
+        a.set_preference(:is_integer, nil)
+        expect(a.preferences[:is_integer]).to be_nil
       end
     end
 
     context "converts decimal preferences to BigDecimal values" do
       before do
-        A.preference :if_decimal, :decimal
+        config_class_a.preference :if_decimal, :decimal
       end
 
       it "returns a BigDecimal" do
-        @a.set_preference(:if_decimal, 3.3)
-        expect(@a.preferences[:if_decimal].class).to eq(BigDecimal)
+        a.set_preference(:if_decimal, 3.3)
+        expect(a.preferences[:if_decimal].class).to eq(BigDecimal)
       end
 
       it "with strings" do
-        @a.set_preference(:if_decimal, '3.3')
-        expect(@a.preferences[:if_decimal]).to eq(3.3)
+        a.set_preference(:if_decimal, '3.3')
+        expect(a.preferences[:if_decimal]).to eq(3.3)
 
-        @a.set_preference(:if_decimal, '')
-        expect(@a.preferences[:if_decimal]).to eq(0.0)
+        a.set_preference(:if_decimal, '')
+        expect(a.preferences[:if_decimal]).to eq(0.0)
       end
     end
 
     context "converts boolean preferences to boolean values" do
       before do
-        A.preference :is_boolean, :boolean, default: true
+        config_class_a.preference :is_boolean, :boolean, default: true
       end
 
       it "with strings" do
-        @a.set_preference(:is_boolean, '0')
-        expect(@a.preferences[:is_boolean]).to be false
-        @a.set_preference(:is_boolean, 'f')
-        expect(@a.preferences[:is_boolean]).to be false
-        @a.set_preference(:is_boolean, 't')
-        expect(@a.preferences[:is_boolean]).to be true
+        a.set_preference(:is_boolean, '0')
+        expect(a.preferences[:is_boolean]).to be false
+        a.set_preference(:is_boolean, 'f')
+        expect(a.preferences[:is_boolean]).to be false
+        a.set_preference(:is_boolean, 't')
+        expect(a.preferences[:is_boolean]).to be true
       end
 
       it "with integers" do
-        @a.set_preference(:is_boolean, 0)
-        expect(@a.preferences[:is_boolean]).to be false
-        @a.set_preference(:is_boolean, 1)
-        expect(@a.preferences[:is_boolean]).to be true
+        a.set_preference(:is_boolean, 0)
+        expect(a.preferences[:is_boolean]).to be false
+        a.set_preference(:is_boolean, 1)
+        expect(a.preferences[:is_boolean]).to be true
       end
 
       it "with an empty string" do
-        @a.set_preference(:is_boolean, '')
-        expect(@a.preferences[:is_boolean]).to be false
+        a.set_preference(:is_boolean, '')
+        expect(a.preferences[:is_boolean]).to be false
       end
 
       it "with an empty hash" do
-        @a.set_preference(:is_boolean, [])
-        expect(@a.preferences[:is_boolean]).to be false
+        a.set_preference(:is_boolean, [])
+        expect(a.preferences[:is_boolean]).to be false
       end
     end
 
     context "converts array preferences to array values" do
       before do
-        A.preference :is_array, :array, default: []
+        config_class_a.preference :is_array, :array, default: []
       end
 
       it "with arrays" do
-        @a.set_preference(:is_array, [])
-        expect(@a.preferences[:is_array]).to eq []
+        a.set_preference(:is_array, [])
+        expect(a.preferences[:is_array]).to eq []
       end
     end
 
     context "converts hash preferences to hash values" do
       before do
-        A.preference :is_hash, :hash, default: {}
+        config_class_a.preference :is_hash, :hash, default: {}
       end
 
       it "with hash" do
-        @a.set_preference(:is_hash, {})
-        expect(@a.preferences[:is_hash]).to be_is_a(Hash)
+        a.set_preference(:is_hash, {})
+        expect(a.preferences[:is_hash]).to be_is_a(Hash)
       end
 
       it "with hash and keys are integers" do
-        @a.set_preference(:is_hash, { 1 => 2, 3 => 4 })
-        expect(@a.preferences[:is_hash]).to eql({ 1 => 2, 3 => 4 })
+        a.set_preference(:is_hash, { 1 => 2, 3 => 4 })
+        expect(a.preferences[:is_hash]).to eql({ 1 => 2, 3 => 4 })
       end
     end
 
     context "converts any preferences to any values" do
-      before do
-        A.preference :product_ids, :any, default: []
-        A.preference :product_attributes, :any, default: {}
-      end
-
       it "with array" do
-        expect(@a.preferences[:product_ids]).to eq([])
-        @a.set_preference(:product_ids, [1, 2])
-        expect(@a.preferences[:product_ids]).to eq([1, 2])
+        config_class_a.preference :product_ids, :any, default: []
+        config_class_a.preference :product_attributes, :any, default: {}
+
+        expect(a.preferences[:product_ids]).to eq([])
+        a.set_preference(:product_ids, [1, 2])
+        expect(a.preferences[:product_ids]).to eq([1, 2])
       end
 
       it "with hash" do
-        expect(@a.preferences[:product_attributes]).to eq({})
-        @a.set_preference(:product_attributes, { id: 1, name: 2 })
-        expect(@a.preferences[:product_attributes]).to eq({ id: 1, name: 2 })
+        config_class_a.preference :product_ids, :any, default: []
+        config_class_a.preference :product_attributes, :any, default: {}
+
+        expect(a.preferences[:product_attributes]).to eq({})
+        a.set_preference(:product_attributes, { id: 1, name: 2 })
+        expect(a.preferences[:product_attributes]).to eq({ id: 1, name: 2 })
       end
     end
 
     context "converts encrypted_string preferences to encrypted values" do
       it "with string, encryption key provided as option" do
-        A.preference :secret, :encrypted_string,
+        config_class_a.preference :secret, :encrypted_string,
                      encryption_key: 'VkYp3s6v9y$B?E(H+MbQeThWmZq4t7w!'
 
-        @a.set_preference(:secret, 'secret_client_id')
-        expect(@a.get_preference(:secret)).to eq('secret_client_id')
-        expect(@a.preferences[:secret]).not_to eq('secret_client_id')
+        a.set_preference(:secret, 'secret_client_id')
+        expect(a.get_preference(:secret)).to eq('secret_client_id')
+        expect(a.preferences[:secret]).not_to eq('secret_client_id')
       end
 
       it "with string, encryption key provided as env variable" do
         expect(ENV).to receive(:[]).with("SOLIDUS_PREFERENCES_MASTER_KEY").and_return("VkYp3s6v9y$B?E(H+MbQeThWmZq4t7w!")
 
-        A.preference :secret, :encrypted_string
+        config_class_a.preference :secret, :encrypted_string
 
-        @a.set_preference(:secret, 'secret_client_id')
-        expect(@a.get_preference(:secret)).to eq('secret_client_id')
-        expect(@a.preferences[:secret]).not_to eq('secret_client_id')
+        a.set_preference(:secret, 'secret_client_id')
+        expect(a.get_preference(:secret)).to eq('secret_client_id')
+        expect(a.preferences[:secret]).not_to eq('secret_client_id')
       end
 
       it "with string, encryption key provided as option, set using syntactic sugar method" do
-        A.preference :secret, :encrypted_string,
+        config_class_a.preference :secret, :encrypted_string,
                      encryption_key: 'VkYp3s6v9y$B?E(H+MbQeThWmZq4t7w!'
 
-        @a.preferred_secret = 'secret_client_id'
-        expect(@a.preferred_secret).to eq('secret_client_id')
-        expect(@a.preferences[:secret]).not_to eq('secret_client_id')
+        a.preferred_secret = 'secret_client_id'
+        expect(a.preferred_secret).to eq('secret_client_id')
+        expect(a.preferences[:secret]).not_to eq('secret_client_id')
       end
 
       it "with string, default value" do
-        A.preference :secret, :encrypted_string,
+        config_class_a.preference :secret, :encrypted_string,
                      default: 'my_default_secret',
                      encryption_key: 'VkYp3s6v9y$B?E(H+MbQeThWmZq4t7w!'
 
-        expect(@a.get_preference(:secret)).to eq('my_default_secret')
-        expect(@a.preferences[:secret]).not_to eq('my_default_secret')
+        a = config_class_a.new
+        expect(a.get_preference(:secret)).to eq('my_default_secret')
+        expect(a.preferences[:secret]).not_to eq('my_default_secret')
       end
     end
   end