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

default_settings nested Arrays and Hashes are mutable #108

Closed
farkmarnum opened this issue Feb 28, 2022 · 0 comments · Fixed by #109
Closed

default_settings nested Arrays and Hashes are mutable #108

farkmarnum opened this issue Feb 28, 2022 · 0 comments · Fixed by #109

Comments

@farkmarnum
Copy link
Contributor

Bug

It's easy to accidentally modify the value of Model.default_settings when there are nested Arrays and/or Hashes.

How to reproduce

For example:

class User < ActiveRecord::Base
  has_settings do |s|
    s.key :promotions, defaults: { dismissed: [] }
  end

  def dismiss_promotion(promotion_to_dismiss)
    setting_value = settings(:promotions).dismissed
    setting_value.push(promotion_to_dismiss)
    settings(:promotions).update!(dismissed: setting_value)
  end
end

user = User.new

# Clear any existing settings for the user:
RailsSettings::SettingObject.where(target: user).destroy_all

# See that the initial value of `User.default_settings` is correct:
User.default_settings[:promotions]
# => {"dismissed"=>[]}

# Update a setting:
user.dismiss_promotion('test')

# Now, `User.default_settings` has been modified!
User.default_settings[:promotions]
# => {"dismissed"=>["test"]}

This is a pretty big issue, since code that uses the above approach will result in the behavior of one user (updating a setting) impact the behavior of all other users (their default_settings will be modified).

Also, class attributes are not thread safe, so this will potentially impact all requests made to the Rails process until it is restarted.

Why is it happening?

Interestingly, while rails-settings does ensure that each value of the default_settings Hash is frozen, it doesn't "deep freeze", i.e. the values within the children Hashes are not frozen.

This means that changing the value of an inner Hash key will raise a FrozenError, but modifying it (if it's an Array/Hash/non-scalar) will not:

User.default_settings[:promotions]['dismissed'] = ['new value']
# FrozenError (can't modify frozen Hash)

User.default_settings[:promotions]['dismissed'].push('new value')
# => ["new value"]

How to fix?

There are two ways to fix this, and they both involve modifying RailsSettings::Configuration:

  1. Make default_settings "deep frozen", so that the nested Arrays/Hashes will also be frozen.
  2. Make default_settings a method that returns a copy of its value instead of a class_attribute.
    With the first option, the consumer of a RailsSettings::SettingObject would have to add .dup because sometimes the value returned would be a default value and would be frozen. So, I think the second option is better, since that way the returned value will always be mutable but will be a copy of the original value, and so the default_settings will not ever be modified.
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

Successfully merging a pull request may close this issue.

1 participant