Skip to content

Add default recovery window option to acts as paranoid #4

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: core
Choose a base branch
from
Open

Add default recovery window option to acts as paranoid #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3af4d74
Add: add-default-recovery-window-option-to-acts-as-paranoid
nathanbertram Aug 23, 2017
5736b02
Overwrite internal default_recovery_window and use paranoia_default_r…
nathanbertram Aug 23, 2017
23e9e1a
Add test_default_recovery_window to set default
nathanbertram Aug 23, 2017
e120e68
test_
nathanbertram Aug 23, 2017
da2dcf2
Drop the default
nathanbertram Aug 23, 2017
acee95b
Specs for paranoia_recovery_window
nathanbertram Aug 23, 2017
64b47db
Add spec
nathanbertram Aug 23, 2017
14780a0
eh
nathanbertram Aug 23, 2017
cf512ba
Fix ParentModelWithRecoveryWindow spec
nathanbertram Aug 31, 2017
de32e53
Binding .pry
nathanbertram Sep 26, 2017
388587b
Options any
nathanbertram Sep 26, 2017
811218c
Remove pry debug code
nathanbertram Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion lib/paranoia.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

module Paranoia
@@default_sentinel_value = nil
@@default_recovery_window = nil
Copy link
Member

@aks aks Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does lib/paranoia.rb ever get run in a thread? If so, the use of class variables will not work. Each thread can independently set the class variable, but they all share the same class variable.

In a multithreaded environment, such as in sidekiq, use thread-local variables:

    Thread.current[:default_sentinel_value] = nil
    Thread.current[:default_recovery_window] = nil

Copy link

@sudara sudara Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I understand this concern. My thoughts:

  1. This is following convention in the existing gem (class var getter/setter) https://github.com/rubysherpas/paranoia/blob/core/lib/paranoia.rb

  2. We are hoping to get this merged upstream (hence wanting to follow their convention)

  3. The class variables are only ever set in a rails initializer.

In practice these variables are more than thread safe — they are just "hooks" to give gem users the ability to set pseudo-constants for the gem once, when the app loads.

But in theory, you are 100% right, it is not thread-safe if someone chose to dynamically change these values. I personally can't imagine it ever being an issue. If you feel strongly about it we could re-strategize about what to do re: upstream.


# Change default_sentinel_value in a rails initializer
def self.default_sentinel_value=(val)
Expand All @@ -12,6 +13,14 @@ def self.default_sentinel_value
@@default_sentinel_value
end

def self.default_recovery_window=(val)
@@default_recovery_window = val
Copy link
Member

@aks aks Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in a multithreaded environment. Use thread-local variables:

    Thread.current[:default_recovery_window] = val

end

def self.default_recovery_window
@@default_recovery_window
Copy link
Member

@aks aks Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use thread-local variables to enable usage in threaded code (eg: within a sidekiq job):

    Thread.current[:default_recovery_window]

end

def self.included(klazz)
klazz.extend Query
klazz.extend Callbacks
Expand Down Expand Up @@ -132,6 +141,7 @@ def restore!(opts = {})

def get_recovery_window_range(opts)
return opts[:recovery_window_range] if opts[:recovery_window_range]
opts[:recovery_window] = paranoia_recovery_window if opts[:recovery_window].blank? && paranoia_recovery_window.present?
return unless opts[:recovery_window]
(deleted_at - opts[:recovery_window]..deleted_at + opts[:recovery_window])
end
Expand Down Expand Up @@ -246,10 +256,12 @@ def self.acts_as_paranoid(options={})
alias_method :destroy_without_paranoia, :destroy

include Paranoia
class_attribute :paranoia_column, :paranoia_sentinel_value
class_attribute :paranoia_column, :paranoia_sentinel_value, :paranoia_recovery_window

self.paranoia_column = (options[:column] || :deleted_at).to_s
self.paranoia_sentinel_value = options.fetch(:sentinel_value) { Paranoia.default_sentinel_value }
self.paranoia_recovery_window = options.fetch(:recovery_window) { Paranoia.default_recovery_window }

def self.paranoia_scope
where(paranoia_column => paranoia_sentinel_value)
end
Expand Down Expand Up @@ -290,6 +302,10 @@ def paranoia_column
def paranoia_sentinel_value
self.class.paranoia_sentinel_value
end

def paranoia_recovery_window
self.class.paranoia_recovery_window
end
end
end

Expand Down
65 changes: 64 additions & 1 deletion test/paranoia_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,26 @@ def test_default_sentinel_value
assert_nil ParanoidModel.paranoia_sentinel_value
end

def test_default_sentinel_value_setter
ParanoidModel.paranoia_sentinel_value = 5
assert_equal 5, ParanoidModel.paranoia_sentinel_value
ParanoidModel.paranoia_sentinel_value = nil
end

def test_default_sentinel_value
assert_nil ParanoidModel.paranoia_sentinel_value
end

def test_default_recovery_window_setter
ParanoidModel.paranoia_recovery_window = 5
assert_equal 5, ParanoidModel.paranoia_recovery_window
ParanoidModel.paranoia_recovery_window = nil
end

def test_default_recovery_window_value
assert_nil ParanoidModel.paranoia_recovery_window
end

def test_without_default_scope_option
model = WithoutDefaultScopeModel.create
model.destroy
Expand Down Expand Up @@ -613,6 +633,38 @@ def test_restore_with_associations_using_recovery_window
assert_equal true, second_child.reload.deleted_at.nil?
end

def test_restore_with_associations_using_default_recovery_window
parent = ParentModelWithRecoveryWindow.create
first_child = parent.very_related_models.create
second_child = parent.very_related_models.create

parent.destroy
second_child.update(deleted_at: parent.deleted_at + 11.minutes)

parent.restore!(:recursive => true, recovery_window: 12.minutes)
assert_equal true, parent.deleted_at.nil?
assert_equal true, first_child.reload.deleted_at.nil?
assert_equal true, second_child.reload.deleted_at.nil?

parent.destroy
second_child.update(deleted_at: parent.deleted_at + 11.minutes)

parent.restore(:recursive => true)
assert_equal true, parent.deleted_at.nil?
assert_equal true, first_child.reload.deleted_at.nil?
assert_equal false, second_child.reload.deleted_at.nil?

second_child.restore
parent.destroy
first_child.update(deleted_at: parent.deleted_at - 11.minutes)
second_child.update(deleted_at: parent.deleted_at - 9.minutes)

ParentModelWithRecoveryWindow.restore(parent.id, :recursive => true)
assert_equal true, parent.reload.deleted_at.nil?
assert_equal false, first_child.reload.deleted_at.nil?
assert_equal true, second_child.reload.deleted_at.nil?
end

def test_restore_with_associations
parent = ParentModel.create
first_child = parent.very_related_models.create
Expand Down Expand Up @@ -1050,7 +1102,6 @@ def get_featureful_model
end

# Helper classes

class ParanoidModel < ActiveRecord::Base
belongs_to :parent_model
acts_as_paranoid
Expand Down Expand Up @@ -1120,6 +1171,18 @@ class ParentModel < ActiveRecord::Base
has_one :polymorphic_model, as: :parent, dependent: :destroy
end

class ParentModelWithRecoveryWindow < ActiveRecord::Base
self.table_name = 'parent_models'
acts_as_paranoid recovery_window: 10.minutes
has_many :paranoid_models, foreign_key: 'parent_model_id'
has_many :related_models, foreign_key: 'parent_model_id'
has_many :very_related_models, class_name: 'RelatedModel', foreign_key: 'parent_model_id', dependent: :destroy
has_many :non_paranoid_models, foreign_key: 'parent_model_id', dependent: :destroy
has_one :non_paranoid_model, foreign_key: 'parent_model_id', dependent: :destroy
has_many :asplode_models, foreign_key: 'parent_model_id', dependent: :destroy
has_one :polymorphic_model, as: :parent, dependent: :destroy
end

class ParentModelWithCounterCacheColumn < ActiveRecord::Base
has_many :related_models
end
Expand Down