From 17a2fed1f6048870cb1123f1e2314a8113c0be37 Mon Sep 17 00:00:00 2001 From: Dmytro Zakharov Date: Thu, 28 Jun 2018 16:09:02 +0200 Subject: [PATCH] This commit is intended to fix an issue, when concurrent requests sent to an endpoint won't be protected by idempotency until the 1st request is finished. This works adding a "lock" key in redis and returning 429 http error when concurrent requests are sent. --- lib/idempotent-request/middleware.rb | 22 +++++++++++++++-- lib/idempotent-request/redis_storage.rb | 22 +++++++++++++---- lib/idempotent-request/request_manager.rb | 4 ++++ spec/idempotent-request/middleware_spec.rb | 24 +++++++++++++++++++ spec/idempotent-request/redis_storage_spec.rb | 10 ++++++++ .../request_manager_spec.rb | 23 ++++++++++++++++++ spec/support/memory_storage.rb | 12 ++++++++++ 7 files changed, 110 insertions(+), 7 deletions(-) diff --git a/lib/idempotent-request/middleware.rb b/lib/idempotent-request/middleware.rb index cd0f0ba..382ea75 100644 --- a/lib/idempotent-request/middleware.rb +++ b/lib/idempotent-request/middleware.rb @@ -14,12 +14,30 @@ module IdempotentRequest def process(env) set_request(env) return app.call(request.env) unless process? - storage = RequestManager.new(request, config) - storage.read || storage.write(*app.call(request.env)) + read_idempotent_request || + write_idempotent_request || + concurrent_request_response end private + def storage + @storage ||= RequestManager.new(request, config) + end + + def read_idempotent_request + storage.read + end + + def write_idempotent_request + return unless storage.lock + storage.write(*app.call(request.env)) + end + + def concurrent_request_response + [429, {}, []] + end + attr_reader :app, :env, :config, :request, :policy def process? diff --git a/lib/idempotent-request/redis_storage.rb b/lib/idempotent-request/redis_storage.rb index 1594b52..5c029c2 100644 --- a/lib/idempotent-request/redis_storage.rb +++ b/lib/idempotent-request/redis_storage.rb @@ -8,14 +8,24 @@ module IdempotentRequest @expire_time = config[:expire_time] end + def lock(key) + setnx_with_expiration(lock_key(key), true) + end + def read(key) redis.get(namespaced_key(key)) end def write(key, payload) + setnx_with_expiration(namespaced_key(key), payload) + end + + private + + def setnx_with_expiration(key, data) redis.set( - namespaced_key(key), - payload, + key, + data, {}.tap do |options| options[:nx] = true options[:ex] = expire_time.to_i if expire_time.to_i > 0 @@ -23,10 +33,12 @@ module IdempotentRequest ) end - private + def lock_key(key) + namespaced_key("lock:#{key}") + end - def namespaced_key(idempotency_key) - [namespace, idempotency_key.strip] + def namespaced_key(key) + [namespace, key.strip] .compact .join(':') .downcase diff --git a/lib/idempotent-request/request_manager.rb b/lib/idempotent-request/request_manager.rb index 1c45027..b56be8d 100644 --- a/lib/idempotent-request/request_manager.rb +++ b/lib/idempotent-request/request_manager.rb @@ -8,6 +8,10 @@ module IdempotentRequest @callback = config[:callback] end + def lock + storage.lock(key) + end + def read status, headers, response = parse_data(storage.read(key)).values diff --git a/spec/idempotent-request/middleware_spec.rb b/spec/idempotent-request/middleware_spec.rb index 9efebf6..7b9f054 100644 --- a/spec/idempotent-request/middleware_spec.rb +++ b/spec/idempotent-request/middleware_spec.rb @@ -42,6 +42,30 @@ RSpec.describe IdempotentRequest::Middleware do middleware.call(env) end end + + context 'when concurrent requests' do + before do + allow_any_instance_of(IdempotentRequest::RequestManager).to receive(:lock).and_return(false) + end + + it 'should not return data from storage' do + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:read).and_return(nil) + + middleware.call(env) + end + + it 'should not obtain lock' do + expect_any_instance_of(IdempotentRequest::RequestManager).not_to receive(:write) + + middleware.call(env) + end + + it 'returns 429' do + expect_any_instance_of(described_class).to receive(:concurrent_request_response) + + middleware.call(env) + end + end end context 'when should not be idempotent' do diff --git a/spec/idempotent-request/redis_storage_spec.rb b/spec/idempotent-request/redis_storage_spec.rb index 8c37a5d..d338bbb 100644 --- a/spec/idempotent-request/redis_storage_spec.rb +++ b/spec/idempotent-request/redis_storage_spec.rb @@ -13,6 +13,16 @@ RSpec.describe IdempotentRequest::RedisStorage do end end + describe '#lock' do + let(:key) { 'key' } + let(:lock_key) { "#{namespace}:lock:#{key}" } + + it 'should add lock' do + expect(redis).to receive(:set).with(lock_key, true, nx: true, ex: expire_time) + redis_storage.lock(key) + end + end + describe '#write' do let(:key) { 'key' } let(:payload) { {} } diff --git a/spec/idempotent-request/request_manager_spec.rb b/spec/idempotent-request/request_manager_spec.rb index 0caf68a..5b30b23 100644 --- a/spec/idempotent-request/request_manager_spec.rb +++ b/spec/idempotent-request/request_manager_spec.rb @@ -13,6 +13,29 @@ RSpec.describe IdempotentRequest::RequestManager do memory_storage.clear end + describe '#lock' do + it 'delegates to storage service' do + expect(memory_storage).to receive(:lock).with(request.key) + request_storage.lock + end + + context 'for the first lock' do + it 'returns true' do + expect(request_storage.lock).to be_truthy + end + end + + context 'for the second lock' do + before do + request_storage.lock + end + + it 'returns false' do + expect(request_storage.lock).to be_falsey + end + end + end + describe '#read' do context 'when there is no data' do it 'should return nil' do diff --git a/spec/support/memory_storage.rb b/spec/support/memory_storage.rb index 814f19d..8295f7a 100644 --- a/spec/support/memory_storage.rb +++ b/spec/support/memory_storage.rb @@ -4,6 +4,12 @@ module IdempotentRequest @memory = {} end + def lock(key) + namespaced_key = lock_key(key) + return false if @memory.key?(namespaced_key) + @memory[namespaced_key] = true + end + def read(key) @memory[key] end @@ -15,5 +21,11 @@ module IdempotentRequest def clear @memory = {} end + + private + + def lock_key(key) + "lock:#{key}" + end end end