feat: properly handle concurrent requests from Memory and Redis stores

lib/rack/idempotency_key.rb

@@ -22,9 +22,9 @@ def call(env)
       return app.call(env) unless request.allowed?
 
       handle_request!(request, env)
-    rescue Request::ConflictError
-      [409, { "Content-Type" => "text/plain" }, ["Conflict"]]
-    rescue Store::Error => e
+    rescue ConflictError => e
+      [409, { "Content-Type" => "text/plain" }, [e.message]]
+    rescue StoreError => e
       [503, { "Content-Type" => "text/plain" }, [e.message]]
     end
 
@@ -33,12 +33,10 @@ def call(env)
       attr_reader :app, :store
 
       def handle_request!(request, env)
-        request.with_lock! do
-          cached_response = request.cached_response!
-          return cached_response unless cached_response.nil?
+        cached_response = request.cached_response!
+        return cached_response unless cached_response.nil?
 
-          app.call(env).tap { |response| request.cache!(response) }
-        end
+        app.call(env).tap { |response| request.cache!(response) }
       end
   end
 end

lib/rack/idempotency_key/error.rb

@@ -5,15 +5,16 @@ class IdempotencyKey
     # Base error class for all IdempotencyKey errors
     Error = Class.new(StandardError)
 
-    class Request
-      # Error raised for general idempotent request failures
-      Error = Class.new(Error)
-      # Error raised when a conflicting idempotent request is detected
-      ConflictError = Class.new(Error)
-    end
+    # Error raised when a conflicting idempotent request is detected
+    class ConflictError < Error
+      DEFAULT_MESSAGE = "This request is already being processed. Please retry later."
 
-    class Store
-      Error = Class.new(Error)
+      def initialize(msg = DEFAULT_MESSAGE)
+        super(msg)
+      end
     end
+
+    # Error raised for general store failures
+    StoreError = Class.new(Error)
   end
 end

lib/rack/idempotency_key/memory_store.rb

@@ -27,6 +27,8 @@ def get(key)
 
       def set(key, value)
         mutex.synchronize do
+          raise Rack::IdempotencyKey::ConflictError if store.key?(key)
+
           store[key] ||= { value: value, added_at: Time.now.utc }
           store[key][:value]
         end

lib/rack/idempotency_key/redis_store.rb

@@ -17,14 +17,18 @@ def get(key)
         value = with_redis { |redis| redis.get(namespaced_key(key)) }
         JSON.parse(value) unless value.nil?
       rescue Redis::BaseError => e
-        raise Rack::IdempotencyKey::Store::Error, "#{self.class}: #{e.message}"
+        raise Rack::IdempotencyKey::StoreError, "#{self.class}: #{e.message}"
       end
 
       def set(key, value)
-        with_redis { |redis| redis.set(namespaced_key(key), value, nx: true, ex: expires_in) }
+        with_redis do |redis|
+          result = redis.set(namespaced_key(key), value, nx: true, ex: expires_in)
+          raise Rack::IdempotencyKey::ConflictError unless result
+        end
+
         get(key)
       rescue Redis::BaseError => e
-        raise Rack::IdempotencyKey::Store::Error, "#{self.class}: #{e.message}"
+        raise Rack::IdempotencyKey::StoreError, "#{self.class}: #{e.message}"
       end
 
       private

lib/rack/idempotency_key/request.rb

@@ -17,16 +17,6 @@ def allowed?
         idempotency_key? && allowed_method?
       end
 
-      # TODO
-      #
-      # 1. Lock immediately the request using the store!
-      #   1.1. Raise ConflictError if the execution should already be locked
-      # 2. Yield the block
-      # 3. Release the lock w/o affecting other locks
-      def with_lock!
-        yield
-      end
-
       def cached_response!
         store.get(cache_key).tap do |response|
           response[1]["Idempotent-Replayed"] = true unless response.nil?

spec/rack/idempotency_key/redis_store_spec.rb

@@ -16,15 +16,15 @@
       before { allow(redis_mock).to receive(:get).and_raise(Redis::BaseError) }
 
       it "raises a store error" do
-        expect { store.get("key") }.to raise_error(Rack::IdempotencyKey::Store::Error)
+        expect { store.get("key") }.to raise_error(Rack::IdempotencyKey::StoreError)
       end
     end
 
     context "when the underlying redis store fails the setter" do
       before { allow(redis_mock).to receive(:set).and_raise(Redis::BaseError) }
 
       it "raises a store error" do
-        expect { store.set("key", "val") }.to raise_error(Rack::IdempotencyKey::Store::Error)
+        expect { store.set("key", "val") }.to raise_error(Rack::IdempotencyKey::StoreError)
       end
     end
   end

spec/support/shared_examples/describe_store_get_and_set_methods.rb

@@ -40,12 +40,8 @@
 
       before { store.set(key, value) }
 
-      it "does not override the existing value" do
-        expect(store.set(key, new_value)).to eq(value)
-      end
-
-      it "returns the already set value" do
-        store.set(key, new_value)
+      it "does not override the existing value" do # rubocop:disable RSpec/MultipleExpectations
+        expect { store.set(key, new_value) }.to raise_error(Rack::IdempotencyKey::ConflictError)
         expect(store.get(key)).to eq(value)
       end
     end