mirror of
https://github.com/discourse/discourse.git
synced 2025-06-06 13:06:56 +08:00
DEV: Follow-up to the lock
step for services
This patch adds two things: 1. An outcome matcher (`on_lock_not_acquired`), allowing to react when there was a problem with the lock. 2. Compatibility with the steps inspector, allowing to display properly the steps of a service containing locks.
This commit is contained in:

committed by
Loïc Guitaut

parent
997a9e3de9
commit
f057c71fc8
@ -265,10 +265,11 @@ module Service
|
|||||||
class LockStep < Step
|
class LockStep < Step
|
||||||
include StepsHelpers
|
include StepsHelpers
|
||||||
|
|
||||||
attr_reader :steps
|
attr_reader :steps, :keys
|
||||||
|
|
||||||
def initialize(*keys, &block)
|
def initialize(*keys, &block)
|
||||||
@keys = keys
|
@keys = keys
|
||||||
|
@name = keys.join(":")
|
||||||
@steps = []
|
@steps = []
|
||||||
instance_exec(&block)
|
instance_exec(&block)
|
||||||
end
|
end
|
||||||
@ -290,10 +291,12 @@ module Service
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
def lock_name
|
def lock_name
|
||||||
[
|
[
|
||||||
context.__service_class__.to_s.underscore,
|
context.__service_class__.to_s.underscore,
|
||||||
*@keys.flat_map { |key| [key, context[:params].send(key)] },
|
*keys.flat_map { |key| [key, context[:params].public_send(key)] },
|
||||||
].join(":")
|
].join(":")
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -25,6 +25,8 @@
|
|||||||
# * +on_exceptions(*exceptions)+: will execute the provided block if any
|
# * +on_exceptions(*exceptions)+: will execute the provided block if any
|
||||||
# exceptions were caught by the `try` block. One or more exception classes
|
# exceptions were caught by the `try` block. One or more exception classes
|
||||||
# can be provided to specifically handle those exceptions.
|
# can be provided to specifically handle those exceptions.
|
||||||
|
# * +on_lock_not_acquired(*keys)+: will execute the provided block if the lock
|
||||||
|
# using `keys` wasn’t acquired successfully.
|
||||||
#
|
#
|
||||||
# All the specialized steps receive the failing step result object as an
|
# All the specialized steps receive the failing step result object as an
|
||||||
# argument to their block. `on_model_errors` receives the actual model so it’s
|
# argument to their block. `on_model_errors` receives the actual model so it’s
|
||||||
@ -105,6 +107,10 @@ class Service::Runner
|
|||||||
name: "default",
|
name: "default",
|
||||||
property: :exception,
|
property: :exception,
|
||||||
},
|
},
|
||||||
|
on_lock_not_acquired: {
|
||||||
|
condition: ->(*keys) { failure_for?("result.lock.#{keys.join(":")}") },
|
||||||
|
key: [],
|
||||||
|
},
|
||||||
}.with_indifferent_access.freeze
|
}.with_indifferent_access.freeze
|
||||||
|
|
||||||
# @!visibility private
|
# @!visibility private
|
||||||
|
@ -123,6 +123,17 @@ class Service::StepsInspector
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# @!visibility private
|
||||||
|
class Lock < Transaction
|
||||||
|
def inspect
|
||||||
|
"#{" " * nesting_level}[#{inspect_type}] #{name}#{runtime} #{emoji}".rstrip
|
||||||
|
end
|
||||||
|
|
||||||
|
def error
|
||||||
|
"Lock '#{name}' was not acquired."
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
attr_reader :steps, :result
|
attr_reader :steps, :result
|
||||||
|
|
||||||
def initialize(result)
|
def initialize(result)
|
||||||
|
@ -487,6 +487,31 @@ RSpec.describe Service::Runner do
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "when using the on_lock_not_acquired action" do
|
||||||
|
let(:service) { LockService }
|
||||||
|
let(:dependencies) { { params: { post_id: 123, user_id: 456 } } }
|
||||||
|
let(:actions) { <<-BLOCK }
|
||||||
|
proc do
|
||||||
|
on_success { :success }
|
||||||
|
on_lock_not_acquired(:post_id, :user_id) { :lock_not_acquired }
|
||||||
|
end
|
||||||
|
BLOCK
|
||||||
|
|
||||||
|
context "when the service fails" do
|
||||||
|
before { allow(DistributedMutex).to receive(:synchronize) }
|
||||||
|
|
||||||
|
it "runs the provided block" do
|
||||||
|
expect(runner).to eq :lock_not_acquired
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context "when the service does not fail" do
|
||||||
|
it "does not run the provided block" do
|
||||||
|
expect(runner).to eq :success
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context "when using several actions together" do
|
context "when using several actions together" do
|
||||||
let(:service) { FailureService }
|
let(:service) { FailureService }
|
||||||
let(:actions) { <<-BLOCK }
|
let(:actions) { <<-BLOCK }
|
||||||
@ -515,36 +540,5 @@ RSpec.describe Service::Runner do
|
|||||||
expect(runner).to eq :success
|
expect(runner).to eq :success
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context "when aquiring a lock" do
|
|
||||||
let(:service) { LockService }
|
|
||||||
let(:dependencies) { { params: { post_id: 123, user_id: 456 } } }
|
|
||||||
let(:actions) { <<-BLOCK }
|
|
||||||
proc do
|
|
||||||
on_success { :success }
|
|
||||||
on_failure { :failure }
|
|
||||||
end
|
|
||||||
BLOCK
|
|
||||||
|
|
||||||
it "runs successfully" do
|
|
||||||
expect(runner).to eq :success
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
context "when failing to acquire a lock" do
|
|
||||||
let(:service) { LockService }
|
|
||||||
let(:dependencies) { { params: { post_id: 123, user_id: 456 } } }
|
|
||||||
let(:actions) { <<-BLOCK }
|
|
||||||
proc do
|
|
||||||
on_success { :success }
|
|
||||||
on_failure { :failure }
|
|
||||||
end
|
|
||||||
BLOCK
|
|
||||||
|
|
||||||
it "fails the service" do
|
|
||||||
DistributedMutex.stubs(:synchronize).returns
|
|
||||||
expect(runner).to eq :failure
|
|
||||||
end
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -14,14 +14,17 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
params do
|
params do
|
||||||
attribute :parameter
|
attribute :parameter
|
||||||
|
attribute :other_param, :integer
|
||||||
|
|
||||||
validates :parameter, presence: true
|
validates :parameter, presence: true
|
||||||
end
|
end
|
||||||
|
|
||||||
|
lock(:parameter, :other_param) do
|
||||||
transaction do
|
transaction do
|
||||||
step :in_transaction_step_1
|
step :in_transaction_step_1
|
||||||
step :in_transaction_step_2
|
step :in_transaction_step_2
|
||||||
end
|
end
|
||||||
|
end
|
||||||
|
|
||||||
try { step :might_raise }
|
try { step :might_raise }
|
||||||
step :final_step
|
step :final_step
|
||||||
@ -51,16 +54,17 @@ RSpec.describe Service::StepsInspector do
|
|||||||
context "when service runs without error" do
|
context "when service runs without error" do
|
||||||
it "outputs all the steps of the service" do
|
it "outputs all the steps of the service" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅
|
[ 3/11] [policy] policy ✅
|
||||||
[ 4/10] [params] default ✅
|
[ 4/11] [params] default ✅
|
||||||
[ 5/10] [transaction]
|
[ 5/11] [lock] parameter:other_param ✅
|
||||||
[ 6/10] [step] in_transaction_step_1 ✅
|
[ 6/11] [transaction]
|
||||||
[ 7/10] [step] in_transaction_step_2 ✅
|
[ 7/11] [step] in_transaction_step_1 ✅
|
||||||
[ 8/10] [try]
|
[ 8/11] [step] in_transaction_step_2 ✅
|
||||||
[ 9/10] [step] might_raise ✅
|
[ 9/11] [try]
|
||||||
[10/10] [step] final_step ✅
|
[10/11] [step] might_raise ✅
|
||||||
|
[11/11] [step] final_step ✅
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
|
|
||||||
@ -80,10 +84,10 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "shows the failing step" do
|
it "shows the failing step" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ❌
|
[ 2/11] [model] model ❌
|
||||||
|
|
||||||
(8 more steps not shown as the execution flow was stopped before reaching them)
|
(9 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@ -99,11 +103,11 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "shows the failing step" do
|
it "shows the failing step" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ❌
|
[ 3/11] [policy] policy ❌
|
||||||
|
|
||||||
(7 more steps not shown as the execution flow was stopped before reaching them)
|
(8 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@ -113,12 +117,12 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "shows the failing step" do
|
it "shows the failing step" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅
|
[ 3/11] [policy] policy ✅
|
||||||
[ 4/10] [params] default ❌
|
[ 4/11] [params] default ❌
|
||||||
|
|
||||||
(6 more steps not shown as the execution flow was stopped before reaching them)
|
(7 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@ -134,13 +138,14 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "shows the failing step" do
|
it "shows the failing step" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅
|
[ 3/11] [policy] policy ✅
|
||||||
[ 4/10] [params] default ✅
|
[ 4/11] [params] default ✅
|
||||||
[ 5/10] [transaction]
|
[ 5/11] [lock] parameter:other_param ✅
|
||||||
[ 6/10] [step] in_transaction_step_1 ✅
|
[ 6/11] [transaction]
|
||||||
[ 7/10] [step] in_transaction_step_2 ❌
|
[ 7/11] [step] in_transaction_step_1 ✅
|
||||||
|
[ 8/11] [step] in_transaction_step_2 ❌
|
||||||
|
|
||||||
(3 more steps not shown as the execution flow was stopped before reaching them)
|
(3 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
@ -158,37 +163,55 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "shows the failing step" do
|
it "shows the failing step" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅
|
[ 3/11] [policy] policy ✅
|
||||||
[ 4/10] [params] default ✅
|
[ 4/11] [params] default ✅
|
||||||
[ 5/10] [transaction]
|
[ 5/11] [lock] parameter:other_param ✅
|
||||||
[ 6/10] [step] in_transaction_step_1 ✅
|
[ 6/11] [transaction]
|
||||||
[ 7/10] [step] in_transaction_step_2 ✅
|
[ 7/11] [step] in_transaction_step_1 ✅
|
||||||
[ 8/10] [try]
|
[ 8/11] [step] in_transaction_step_2 ✅
|
||||||
[ 9/10] [step] might_raise 💥
|
[ 9/11] [try]
|
||||||
|
[10/11] [step] might_raise 💥
|
||||||
|
|
||||||
(1 more steps not shown as the execution flow was stopped before reaching them)
|
(1 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "when the lock step is failing" do
|
||||||
|
before { allow(DistributedMutex).to receive(:synchronize) }
|
||||||
|
|
||||||
|
it "shows the failing step" do
|
||||||
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
|
[ 1/11] [options] default ✅
|
||||||
|
[ 2/11] [model] model ✅
|
||||||
|
[ 3/11] [policy] policy ✅
|
||||||
|
[ 4/11] [params] default ✅
|
||||||
|
[ 5/11] [lock] parameter:other_param ❌
|
||||||
|
|
||||||
|
(6 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
|
OUTPUT
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context "when running in specs" do
|
context "when running in specs" do
|
||||||
context "when a successful step is flagged as being an unexpected result" do
|
context "when a successful step is flagged as being an unexpected result" do
|
||||||
before { result["result.policy.policy"]["spec.unexpected_result"] = true }
|
before { result["result.policy.policy"]["spec.unexpected_result"] = true }
|
||||||
|
|
||||||
it "adapts its output accordingly" do
|
it "adapts its output accordingly" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅ ⚠️ <= expected to return false but got true instead
|
[ 3/11] [policy] policy ✅ ⚠️ <= expected to return false but got true instead
|
||||||
[ 4/10] [params] default ✅
|
[ 4/11] [params] default ✅
|
||||||
[ 5/10] [transaction]
|
[ 5/11] [lock] parameter:other_param ✅
|
||||||
[ 6/10] [step] in_transaction_step_1 ✅
|
[ 6/11] [transaction]
|
||||||
[ 7/10] [step] in_transaction_step_2 ✅
|
[ 7/11] [step] in_transaction_step_1 ✅
|
||||||
[ 8/10] [try]
|
[ 8/11] [step] in_transaction_step_2 ✅
|
||||||
[ 9/10] [step] might_raise ✅
|
[ 9/11] [try]
|
||||||
[10/10] [step] final_step ✅
|
[10/11] [step] might_raise ✅
|
||||||
|
[11/11] [step] final_step ✅
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@ -205,11 +228,11 @@ RSpec.describe Service::StepsInspector do
|
|||||||
|
|
||||||
it "adapts its output accordingly" do
|
it "adapts its output accordingly" do
|
||||||
expect(output).to eq <<~OUTPUT.chomp
|
expect(output).to eq <<~OUTPUT.chomp
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ❌ ⚠️ <= expected to return true but got false instead
|
[ 3/11] [policy] policy ❌ ⚠️ <= expected to return true but got false instead
|
||||||
|
|
||||||
(7 more steps not shown as the execution flow was stopped before reaching them)
|
(8 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
@ -263,7 +286,7 @@ RSpec.describe Service::StepsInspector do
|
|||||||
end
|
end
|
||||||
|
|
||||||
it "returns the provided paramaters" do
|
it "returns the provided paramaters" do
|
||||||
expect(error).to match(/{"parameter"=>nil}/)
|
expect(error).to match(/{"parameter"=>nil, "other_param"=>nil}/)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
@ -312,6 +335,14 @@ RSpec.describe Service::StepsInspector do
|
|||||||
expect(error).to match(/BOOM \([^(]*RuntimeError[^)]*\)/)
|
expect(error).to match(/BOOM \([^(]*RuntimeError[^)]*\)/)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "when the lock step is failing" do
|
||||||
|
before { allow(DistributedMutex).to receive(:synchronize) }
|
||||||
|
|
||||||
|
it "returns an error" do
|
||||||
|
expect(error).to eq("Lock 'parameter:other_param' was not acquired.")
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe "#inspect" do
|
describe "#inspect" do
|
||||||
@ -321,18 +352,18 @@ RSpec.describe Service::StepsInspector do
|
|||||||
expect(inspector.inspect.gsub(%r{ \(\d+\.\d+ ms\)}, "")).to eq(<<~OUTPUT)
|
expect(inspector.inspect.gsub(%r{ \(\d+\.\d+ ms\)}, "")).to eq(<<~OUTPUT)
|
||||||
Inspecting DummyService result object:
|
Inspecting DummyService result object:
|
||||||
|
|
||||||
[ 1/10] [options] default ✅
|
[ 1/11] [options] default ✅
|
||||||
[ 2/10] [model] model ✅
|
[ 2/11] [model] model ✅
|
||||||
[ 3/10] [policy] policy ✅
|
[ 3/11] [policy] policy ✅
|
||||||
[ 4/10] [params] default ❌
|
[ 4/11] [params] default ❌
|
||||||
|
|
||||||
(6 more steps not shown as the execution flow was stopped before reaching them)
|
(7 more steps not shown as the execution flow was stopped before reaching them)
|
||||||
|
|
||||||
Why it failed:
|
Why it failed:
|
||||||
|
|
||||||
#<ActiveModel::Errors [#<ActiveModel::Error attribute=parameter, type=blank, options={}>]>
|
#<ActiveModel::Errors [#<ActiveModel::Error attribute=parameter, type=blank, options={}>]>
|
||||||
|
|
||||||
Provided parameters: {"parameter"=>nil}
|
Provided parameters: {"parameter"=>nil, "other_param"=>nil}
|
||||||
OUTPUT
|
OUTPUT
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
Reference in New Issue
Block a user