diff --git a/idempotent-request.gemspec b/idempotent-request.gemspec index e289f4c..00094ac 100644 --- a/idempotent-request.gemspec +++ b/idempotent-request.gemspec @@ -27,5 +27,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'rake', '~> 10.0' spec.add_development_dependency 'rspec', '~> 3.0' spec.add_development_dependency 'fakeredis', '~> 0.6' - spec.add_development_dependency 'pry', '~> 0.11' + spec.add_development_dependency 'byebug', '~> 10.0' end diff --git a/lib/idempotent-request/middleware.rb b/lib/idempotent-request/middleware.rb index 382ea75..1baf339 100644 --- a/lib/idempotent-request/middleware.rb +++ b/lib/idempotent-request/middleware.rb @@ -4,6 +4,7 @@ module IdempotentRequest @app = app @config = config @policy = config.fetch(:policy) + @notifier = ActiveSupport::Notifications if defined?(ActiveSupport::Notifications) end def call(env) @@ -13,10 +14,12 @@ module IdempotentRequest def process(env) set_request(env) + request.env['idempotent.request'] = {} return app.call(request.env) unless process? - read_idempotent_request || - write_idempotent_request || - concurrent_request_response + request.env['idempotent.request']['key'] = request.key + response = read_idempotent_request || write_idempotent_request || concurrent_request_response + instrument(request) + response end private @@ -26,19 +29,30 @@ module IdempotentRequest end def read_idempotent_request - storage.read + request.env['idempotent.request']['read'] = storage.read end def write_idempotent_request return unless storage.lock - storage.write(*app.call(request.env)) + begin + result = app.call(request.env) + request.env['idempotent.request']['write'] = result + storage.write(*result) + ensure + request.env['idempotent.request']['unlocked'] = storage.unlock + result + end end def concurrent_request_response - [429, {}, []] + status = 429 + headers = { 'Content-Type' => 'application/json' } + body = [ Oj.dump('error' => 'Concurrent requests detected') ] + request.env['idempotent.request']['concurrent_request_response'] = true + Rack::Response.new(body, status, headers).finish end - attr_reader :app, :env, :config, :request, :policy + attr_reader :app, :env, :config, :request, :policy, :notifier def process? !request.key.to_s.empty? && should_be_idempotent? @@ -49,6 +63,10 @@ module IdempotentRequest policy.new(request).should? end + def instrument(request) + notifier.instrument('idempotent.request', request: request) if notifier + end + def set_request(env) @env = env @request ||= Request.new(env, config) diff --git a/lib/idempotent-request/redis_storage.rb b/lib/idempotent-request/redis_storage.rb index 551d7e7..f8f642c 100644 --- a/lib/idempotent-request/redis_storage.rb +++ b/lib/idempotent-request/redis_storage.rb @@ -9,7 +9,7 @@ module IdempotentRequest end def lock(key) - setnx_with_expiration(lock_key(key), true) + setnx_with_expiration(lock_key(key), Time.now.to_f) end def unlock(key) diff --git a/lib/idempotent-request/request.rb b/lib/idempotent-request/request.rb index f1c23c8..aa6b7b0 100644 --- a/lib/idempotent-request/request.rb +++ b/lib/idempotent-request/request.rb @@ -22,10 +22,9 @@ module IdempotentRequest private def header_name - key = @header_name - .to_s - .upcase - .gsub('-', '_') + key = @header_name.to_s + .upcase + .tr('-', '_') key.start_with?('HTTP_') ? key : "HTTP_#{key}" end diff --git a/lib/idempotent-request/request_manager.rb b/lib/idempotent-request/request_manager.rb index c603c15..f63035d 100644 --- a/lib/idempotent-request/request_manager.rb +++ b/lib/idempotent-request/request_manager.rb @@ -30,8 +30,6 @@ module IdempotentRequest if (200..226).cover?(status) storage.write(key, payload(status, headers, response)) - else - unlock end data @@ -46,11 +44,9 @@ module IdempotentRequest end def payload(status, headers, response) - Oj.dump({ - status: status, - headers: headers.to_h, - response: Array(response) - }) + Oj.dump(status: status, + headers: headers.to_h, + response: Array(response)) end def run_callback(action, args) diff --git a/spec/idempotent-request/middleware_spec.rb b/spec/idempotent-request/middleware_spec.rb index 7b9f054..900edb3 100644 --- a/spec/idempotent-request/middleware_spec.rb +++ b/spec/idempotent-request/middleware_spec.rb @@ -29,6 +29,26 @@ RSpec.describe IdempotentRequest::Middleware do middleware.call(env) end + it 'should obtain lock and release lock' do + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:lock).and_return(true) + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:write) + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:unlock) + + middleware.call(env) + end + + context 'when an exception happens inside another middleware' do + let(:app) { ->(_) { raise 'fatality' } } + + it 'should release lock' do + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:lock).and_return(true) + expect_any_instance_of(IdempotentRequest::RequestManager).not_to receive(:write) + expect_any_instance_of(IdempotentRequest::RequestManager).to receive(:unlock) + + expect { middleware.call(env) }.to raise_error('fatality') + end + end + context 'when has data in storage' do before do data = [200, {}, 'body'] diff --git a/spec/idempotent-request/redis_storage_spec.rb b/spec/idempotent-request/redis_storage_spec.rb index 5d4e853..9c510ec 100644 --- a/spec/idempotent-request/redis_storage_spec.rb +++ b/spec/idempotent-request/redis_storage_spec.rb @@ -18,7 +18,7 @@ RSpec.describe IdempotentRequest::RedisStorage do 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) + expect(redis).to receive(:set).with(lock_key, Float, nx: true, ex: expire_time) redis_storage.lock(key) end end diff --git a/spec/idempotent-request/request_manager_spec.rb b/spec/idempotent-request/request_manager_spec.rb index 0550f8d..8d59f9c 100644 --- a/spec/idempotent-request/request_manager_spec.rb +++ b/spec/idempotent-request/request_manager_spec.rb @@ -144,11 +144,6 @@ RSpec.describe IdempotentRequest::RequestManager do request_storage.write(*data) expect(memory_storage.read(request.key)).to be_nil end - - it 'should unlock stored key' do - expect(memory_storage).to receive(:unlock).with(request.key) - request_storage.write(*data) - end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7a309df..a14a4fe 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,6 +1,6 @@ require "bundler/setup" require 'fakeredis' -require 'pry' +require 'byebug' require "idempotent-request" spec = File.expand_path('../', __FILE__)