Skip to content

Commit

Permalink
Fix call context when a preference default is a proc
Browse files Browse the repository at this point in the history
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.
Roddoric committed Dec 2, 2022
1 parent efe2b3a commit 67bd03d
Showing 3 changed files with 121 additions and 99 deletions.
5 changes: 5 additions & 0 deletions core/lib/spree/preferences/preferable.rb
Original file line number Diff line number Diff line change
@@ -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|
4 changes: 2 additions & 2 deletions core/lib/spree/preferences/preferable_class_methods.rb
Original file line number Diff line number Diff line change
@@ -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
211 changes: 114 additions & 97 deletions core/spec/models/spree/preferences/preferable_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 67bd03d

Please sign in to comment.