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

Provide some configuration DSL for custom Strategies and Locks #383

Merged
merged 7 commits into from
Apr 15, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions lib/sidekiq_unique_jobs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@
require "sidekiq_unique_jobs/sidekiq_unique_ext"
require "sidekiq_unique_jobs/on_conflict"

require "sidekiq_unique_jobs/config"
Copy link
Owner

Choose a reason for hiding this comment

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

I like the move to a separate file now that it turned complex again! 👍

require "sidekiq_unique_jobs/sidekiq_unique_jobs"
60 changes: 60 additions & 0 deletions lib/sidekiq_unique_jobs/config.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# frozen_string_literal: true

module SidekiqUniqueJobs
# Shared class for dealing with gem configuration
#
# @author Mauro Berlanda <mauro.berlanda@gmail.com>
class Config < Concurrent::MutableStruct.new(
:default_lock_timeout,
:enabled,
:unique_prefix,
:logger,
:locks,
:strategies,
)
DEFAULT_LOCKS = {
until_and_while_executing: SidekiqUniqueJobs::Lock::UntilAndWhileExecuting,
until_executed: SidekiqUniqueJobs::Lock::UntilExecuted,
until_executing: SidekiqUniqueJobs::Lock::UntilExecuting,
until_expired: SidekiqUniqueJobs::Lock::UntilExpired,
until_timeout: SidekiqUniqueJobs::Lock::UntilExpired,
while_executing: SidekiqUniqueJobs::Lock::WhileExecuting,
while_executing_reject: SidekiqUniqueJobs::Lock::WhileExecutingReject,
}.freeze

DEFAULT_STRATEGIES = {
log: SidekiqUniqueJobs::OnConflict::Log,
raise: SidekiqUniqueJobs::OnConflict::Raise,
reject: SidekiqUniqueJobs::OnConflict::Reject,
replace: SidekiqUniqueJobs::OnConflict::Replace,
reschedule: SidekiqUniqueJobs::OnConflict::Reschedule,
}.freeze

class << self
def default
Copy link
Owner

Choose a reason for hiding this comment

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

The only thing I immediately don't like is class << self. I've avoided it like the plague because I find that additional duplication of self very revealing. Especially in large classes where it is difficult to discern what level of nesting I'm at so please do use self.default. In my opinion not all duplication are to be avoided :)

I like having the default here and using it from the top level though! ❤️

new(
0,
true,
"uniquejobs",
Sidekiq.logger,
DEFAULT_LOCKS,
DEFAULT_STRATEGIES,
)
end
end

def add_lock(name, klass)
raise ArgumentError, "Lock #{name} already defined, please use another name" if locks.key?(name.to_sym)

new_locks = locks.dup.merge(name.to_sym => klass).freeze
self.locks = new_locks
end

def add_strategy(name, klass)
raise ArgumentError, "strategy #{name} already defined, please use another name" if strategies.key?(name.to_sym)

new_strategies = strategies.dup.merge(name.to_sym => klass).freeze
self.strategies = new_strategies
end
end
end
13 changes: 5 additions & 8 deletions lib/sidekiq_unique_jobs/on_conflict.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,14 @@ module SidekiqUniqueJobs
# @author Mikael Henriksson <mikael@zoolutions.se>
#
module OnConflict
STRATEGIES = {
log: OnConflict::Log,
raise: OnConflict::Raise,
reject: OnConflict::Reject,
replace: OnConflict::Replace,
reschedule: OnConflict::Reschedule,
}.freeze
# A convenience method for using the configured strategies
def self.strategies
SidekiqUniqueJobs.strategies
Copy link
Owner

Choose a reason for hiding this comment

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

I've used delegation a lot in many places but I honestly prefer a defined method and was planning on refactoring in this direction in some places 👍

end

# returns OnConflict::NullStrategy when no other could be found
def self.find_strategy(strategy)
STRATEGIES.fetch(strategy.to_s.to_sym) { OnConflict::NullStrategy }
strategies.fetch(strategy.to_s.to_sym) { OnConflict::NullStrategy }
end
end
end
17 changes: 6 additions & 11 deletions lib/sidekiq_unique_jobs/options_with_fallback.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,15 @@ module SidekiqUniqueJobs
# 3. worker_class (required, can be anything)
# @author Mikael Henriksson <mikael@zoolutions.se>
module OptionsWithFallback
LOCKS = {
until_and_while_executing: SidekiqUniqueJobs::Lock::UntilAndWhileExecuting,
until_executed: SidekiqUniqueJobs::Lock::UntilExecuted,
until_executing: SidekiqUniqueJobs::Lock::UntilExecuting,
until_expired: SidekiqUniqueJobs::Lock::UntilExpired,
until_timeout: SidekiqUniqueJobs::Lock::UntilExpired,
while_executing: SidekiqUniqueJobs::Lock::WhileExecuting,
while_executing_reject: SidekiqUniqueJobs::Lock::WhileExecutingReject,
}.freeze

def self.included(base)
base.send(:include, SidekiqUniqueJobs::SidekiqWorkerMethods)
end

# A convenience method for using the configured locks
def locks
SidekiqUniqueJobs.locks
end

# Check if unique has been enabled
# @return [true, false] indicate if the gem has been enabled
def unique_enabled?
Expand All @@ -49,7 +44,7 @@ def lock
# @return [SidekiqUniqueJobs::Lock::BaseLock] an instance of a child class
def lock_class
@lock_class ||= begin
LOCKS.fetch(lock_type.to_sym) do
locks.fetch(lock_type.to_sym) do
raise UnknownLock, "No implementation for `lock: :#{lock_type}`"
end
end
Expand Down
26 changes: 13 additions & 13 deletions lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,22 @@ module SidekiqUniqueJobs

module_function

Config = Concurrent::MutableStruct.new(
:default_lock_timeout,
:enabled,
:unique_prefix,
:logger,
)

# The current configuration (See: {.configure} on how to configure)
def config
# Arguments here need to match the definition of the new class (see above)
@config ||= Config.new(
0,
true,
"uniquejobs",
Sidekiq.logger,
)
@config ||= SidekiqUniqueJobs::Config.default
end

# The current strategies
# @return [Hash] the configured strategies
def strategies
config.strategies
end

# The current locks
# @return [Hash] the configured locks
def locks
config.locks
end

# The current logger
Expand Down
116 changes: 116 additions & 0 deletions spec/unit/sidekiq_unique_jobs/config_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
# frozen_string_literal: true

require "spec_helper"

RSpec.describe SidekiqUniqueJobs::Config do
describe ".locks" do
let(:config) { described_class.default }

context "when using default config" do
it "falls back on default option" do
expect(config.locks).to eq(SidekiqUniqueJobs::Config::DEFAULT_LOCKS)
end
end

context "when trying to add an already existing lock" do
it "raises an ArgumentError exception" do
name = "while_executing"
expect do
config.add_lock name, Class
end.to raise_exception(ArgumentError, /#{name} already defined/)
end
end

context "when adding a new lock" do
it "preserves it in the config instance" do
name = "some_lock"
klass = Class

original_locks_id = config.locks.object_id
config.add_lock name, klass

expect(config.locks.frozen?).to be(true)
expect(config.locks.keys).to include(:some_lock)
expect(config.locks.fetch(:some_lock)).to eq(Class)
expect(config.locks.object_id).not_to eq(original_locks_id)
end

it "accepts as many locks as you want" do
CustomLock1 = Class.new
CustomLock2 = Class.new

config.add_lock :custom_lock1, CustomLock1
config.add_lock :custom_lock2, CustomLock2

expect(config.locks.frozen?).to be(true)
expect(config.locks.keys).to include(:custom_lock1, :custom_lock2)
expect(config.locks.fetch(:custom_lock1)).to eq(CustomLock1)
expect(config.locks.fetch(:custom_lock2)).to eq(CustomLock2)
end
end
end

describe ".strategies" do
let(:config) { described_class.default }

context "when using default config" do
it "falls back on default option" do
expect(config.strategies).to eq(SidekiqUniqueJobs::Config::DEFAULT_STRATEGIES)
end
end

context "when trying to add an already existing lock" do
it "raises an ArgumentError exception" do
name = "log"
expect do
config.add_strategy name, Class
end.to raise_exception(ArgumentError, /#{name} already defined/)
end
end

context "when adding a new strategy" do
it "preserves it in the config instance" do
name = "some_strategy"
klass = Class

original_strategies_id = config.strategies.object_id
config.add_strategy name, klass

expect(config.strategies.frozen?).to be(true)
expect(config.strategies.keys).to include(:some_strategy)
expect(config.strategies.fetch(:some_strategy)).to eq(Class)
expect(config.strategies.object_id).not_to eq(original_strategies_id)
end

it "accepts as many strategies as you want" do
CustomStrategy1 = Class.new
CustomStrategy2 = Class.new

config.add_strategy "custom_strategy1", CustomStrategy1
config.add_strategy :custom_strategy2, CustomStrategy2

expect(config.strategies.frozen?).to be(true)
expect(config.strategies.keys).to include(:custom_strategy1, :custom_strategy2)
expect(config.strategies.fetch(:custom_strategy1)).to eq(CustomStrategy1)
expect(config.strategies.fetch(:custom_strategy2)).to eq(CustomStrategy2)
end
end
end

# Test backported from spec/unit/on_conflict_spec.rb
describe "::DEFAULT_STRATEGIES" do
subject { described_class::DEFAULT_STRATEGIES }

let(:expected) do
{
log: SidekiqUniqueJobs::OnConflict::Log,
raise: SidekiqUniqueJobs::OnConflict::Raise,
reject: SidekiqUniqueJobs::OnConflict::Reject,
replace: SidekiqUniqueJobs::OnConflict::Replace,
reschedule: SidekiqUniqueJobs::OnConflict::Reschedule,
}
end

it { is_expected.to eq(expected) }
end
end
32 changes: 21 additions & 11 deletions spec/unit/sidekiq_unique_jobs/on_conflict_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,29 @@

require "spec_helper"
RSpec.describe SidekiqUniqueJobs::OnConflict do
describe "::STRAGEGIES" do
subject { described_class::STRATEGIES }
it { expect(described_class.strategies).to eq(SidekiqUniqueJobs.strategies) }

let(:expected) do
{
log: described_class::Log,
raise: described_class::Raise,
reject: described_class::Reject,
replace: described_class::Replace,
reschedule: described_class::Reschedule,
}
describe "#find_strategy" do
before do
allow(SidekiqUniqueJobs).to receive(:strategies).and_return(
log: SidekiqUniqueJobs::OnConflict::Log,
)
end

it { is_expected.to eq(expected) }
context "when a strategy is found" do
it "returns the given strategy" do
expect(described_class.find_strategy("log")).to eq(SidekiqUniqueJobs::OnConflict::Log)
end
end

context "when a strategy is not found" do
it "does not raise any exception" do
expect { described_class.find_strategy(:foo) }.not_to raise_exception
end

it "returns an OnConflict::NullStrategy" do
expect(described_class.find_strategy(:foo)).to eq(SidekiqUniqueJobs::OnConflict::NullStrategy)
end
end
end
end