refactor: better organise the idempotent request public interface (#10)

* refactor: better organise the idempotent request public interface

* refactor: remove request statuses

* refactor: add get and set template methods

* chore: bump Ruby min version to 2.6 and add Redis as dep

* refactor: rescue Redis::BaseError

* refactor: extract magic number into DEFAULT_EXPIRATION

* refactor: add thread-safety through mutex for memory store

* refactor: extract handle_request!

---------

Co-authored-by: Matteo Rossi <[email protected]>

.github/workflows/lint.yml

@@ -16,6 +16,6 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           bundler-cache: true
-          ruby-version: 2.5
+          ruby-version: 2.6
       - run: bundle install
       - run: bundle exec rubocop

.github/workflows/release.yml

@@ -47,7 +47,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           bundler-cache: true
-          ruby-version: 2.5
+          ruby-version: 2.6
         if: ${{steps.release.outputs.release_created}}
       - run: bundle install
         if: ${{steps.release.outputs.release_created}}

.github/workflows/test.yml

@@ -18,7 +18,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        ruby: [2.5, 2.6, 2.7, '3.0', 3.1, 3.2, 3.3, 3.4, head]
+        ruby: [2.6, 2.7, '3.0', 3.1, 3.2, 3.3, 3.4, head]
     steps:
       - uses: actions/checkout@v4
       - uses: ruby/setup-ruby@v1
@@ -34,7 +34,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           bundler-cache: true
-          ruby-version: 2.5
+          ruby-version: 2.6
       - run: bundle install
       - name: Test & publish code coverage
         uses: paambaati/[email protected]

.rubocop.yml

@@ -5,7 +5,7 @@ require:
   - rubocop-rspec
 
 AllCops:
-  TargetRubyVersion: 2.5
+  TargetRubyVersion: 2.6
 
 Bundler/OrderedGems:
   Enabled: false

idempotency_key.gemspec

@@ -10,7 +10,7 @@ Gem::Specification.new do |spec|
   spec.email = ["[email protected]"]
   spec.summary = "A Rack Middleware implementing the idempotency principle"
   spec.homepage = "https://github.com/matteoredz/rack-idempotency_key"
-  spec.required_ruby_version = Gem::Requirement.new(">= 2.5.0")
+  spec.required_ruby_version = Gem::Requirement.new(">= 2.6.0")
   spec.metadata["homepage_uri"] = spec.homepage
   spec.metadata["source_code_uri"] = "https://github.com/matteoredz/rack-idempotency_key"
   spec.metadata["changelog_uri"] = "https://github.com/matteoredz/rack-idempotency_key/CHANGELOG.md"
@@ -24,4 +24,6 @@ Gem::Specification.new do |spec|
   spec.bindir = "exe"
   spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
   spec.require_paths = ["lib"]
+
+  spec.add_runtime_dependency "redis", "~> 5.2"
 end

lib/rack/idempotency_key.rb

@@ -1,42 +1,45 @@
 # frozen_string_literal: true
 
 require "rack/idempotency_key/version"
+require "rack/idempotency_key/error"
 
 # Stores
 require "rack/idempotency_key/memory_store"
 require "rack/idempotency_key/redis_store"
 
 # Collaborators
-require "rack/idempotency_key/idempotent_request"
+require "rack/idempotency_key/request"
 
 module Rack
   class IdempotencyKey
-    Error = Class.new(StandardError)
-
     def initialize(app, routes: [], store: MemoryStore.new)
       @app    = app
       @routes = routes
       @store  = store
     end
 
     def call(env)
-      request = IdempotentRequest.new(Rack::Request.new(env), routes)
+      request = Request.new(Rack::Request.new(env), routes, store)
       return app.call(env) unless request.allowed?
 
-      cached_response = store.get(request.idempotency_key)
-
-      if cached_response
-        cached_response[1]["Idempotent-Replayed"] = true
-        return cached_response
-      end
-
-      app.call(env).tap do |response|
-        store.set(request.idempotency_key, response) if response[0] != 400
-      end
+      handle_request!(request, env)
+    rescue Request::ConflictError
+      [409, { "Content-Type" => "text/plain" }, ["Conflict"]]
+    rescue Store::Error => e
+      [503, { "Content-Type" => "text/plain" }, [e.message]]
     end
 
     private
 
       attr_reader :app, :store, :routes
+
+      def handle_request!(request, env)
+        request.with_lock! do
+          cached_response = request.cached_response!
+          return cached_response unless cached_response.nil?
+
+          app.call(env).tap { |response| request.cache!(response) }
+        end
+      end
   end
 end

lib/rack/idempotency_key/error.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Rack
+  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
+
+    class Store
+      Error = Class.new(Error)
+    end
+  end
+end

lib/rack/idempotency_key/memory_store.rb

@@ -1,33 +1,39 @@
 # frozen_string_literal: true
 
+require "rack/idempotency_key/store"
+
 module Rack
   class IdempotencyKey
-    class MemoryStore
-      def initialize(expires_in: 86_400)
-        @store      = {}
-        @expires_in = expires_in
+    class MemoryStore < Store
+      def initialize(store = {}, expires_in: 86_400)
+        super(store, expires_in: expires_in)
+        @mutex = Mutex.new
       end
 
       def get(key)
-        value = store[key]
-        return if value.nil?
+        mutex.synchronize do
+          value = store[key]
+          return if value.nil?
 
-        if expired?(value[:added_at])
-          store.delete(key)
-          return
-        end
+          if expired?(value[:added_at])
+            store.delete(key)
+            return
+          end
 
-        value[:value]
+          value[:value]
+        end
       end
 
       def set(key, value)
-        store[key] ||= { value: value, added_at: Time.now.utc }
-        get(key)
+        mutex.synchronize do
+          store[key] ||= { value: value, added_at: Time.now.utc }
+          store[key][:value]
+        end
       end
 
       private
 
-        attr_reader :store, :expires_in
+        attr_reader :mutex
 
         def expired?(added_at)
           Time.now.utc - added_at > expires_in

lib/rack/idempotency_key/redis_store.rb

@@ -1,29 +1,34 @@
 # frozen_string_literal: true
 
+require "redis"
+
+require "rack/idempotency_key/store"
+
 module Rack
   class IdempotencyKey
-    class RedisStore
+    class RedisStore < Store
       KEY_NAMESPACE = "idempotency_key"
 
       def initialize(store, expires_in: 86_400)
-        @store      = store
-        @expires_in = expires_in
+        super(store, expires_in: expires_in)
       end
 
       def get(key)
         value = store.get(namespaced_key(key))
         JSON.parse(value) unless value.nil?
+      rescue Redis::BaseError => e
+        raise Rack::IdempotencyKey::Store::Error, "#{self.class}: #{e.message}"
       end
 
       def set(key, value)
         store.set(namespaced_key(key), value, nx: true, ex: expires_in)
         get(key)
+      rescue Redis::BaseError => e
+        raise Rack::IdempotencyKey::Store::Error, "#{self.class}: #{e.message}"
       end
 
       private
 
-        attr_reader :store, :expires_in
-
         def namespaced_key(key)
           "#{KEY_NAMESPACE}:#{key.split.join}"
         end

lib/rack/idempotency_key/idempotent_request.rb → lib/rack/idempotency_key/request.rb

@@ -2,38 +2,61 @@
 
 module Rack
   class IdempotencyKey
-    class IdempotentRequest
-      # @param [Rack::Request] request
-      # @param [Array]         routes
-      def initialize(request, routes = [])
+    class Request
+      # @param request [Rack::Request]
+      # @param routes  [Array]
+      # @param store   [Store]
+      def initialize(request, routes, store)
         @request = request
         @routes  = routes
+        @store   = store
       end
 
-      # Check if the `Idempotency-Key` header is present, if the HTTP request method is
+      # Checks if the `Idempotency-Key` header is present, if the HTTP request method is
       # allowed and if there is any matching route whitelisted in the `routes` array.
       #
       # @return [Boolean]
       def allowed?
         idempotency_key? && allowed_method? && any_matching_route?
       end
 
-      # Check if the HTTP request method is non-idempotent by design.
+      # 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?
+        end
+      end
+
+      def cache!(response)
+        status, = response
+        store.set(cache_key, response) if status != 400
+      end
+
+      # Checks if the HTTP request method is non-idempotent by design.
       #
       # @return [Boolean]
       def allowed_method?
         %w[POST PATCH CONNECT].include? request.request_method
       end
 
-      # Check if there is any matching route from the `routes` input array against
+      # Checks if there is any matching route from the `routes` input array against
       # the currently requested path.
       #
       # @return [Boolean]
       def any_matching_route?
         routes.any? { |route| matching_route?(route[:path]) && matching_method?(route[:method]) }
       end
 
-      # Check if the given request has the Idempotency-Key header
+      # Checks if the given request has the Idempotency-Key header
       #
       # @return [Boolean]
       def idempotency_key?
@@ -47,9 +70,13 @@ def idempotency_key
         request.get_header "HTTP_IDEMPOTENCY_KEY"
       end
 
+      def cache_key
+        idempotency_key
+      end
+
       private
 
-        attr_reader :request, :routes
+        attr_reader :request, :routes, :store
 
         def matching_route?(route_path)
           route_segments = segments route_path

lib/rack/idempotency_key/store.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Rack
+  class IdempotencyKey
+    class Store
+      DEFAULT_EXPIRATION = 86_400 # 24 hours in seconds
+
+      def initialize(store, expires_in: DEFAULT_EXPIRATION)
+        @store      = store
+        @expires_in = expires_in
+      end
+
+      protected
+
+        attr_reader :store, :expires_in
+    end
+  end
+end

spec/rack/idempotency_key/redis_store_spec.rb

@@ -5,7 +5,27 @@
 require "spec_helper"
 
 RSpec.describe Rack::IdempotencyKey::RedisStore do
-  subject(:store) { described_class.new(MockRedis.new) }
+  subject(:store) { described_class.new(redis_mock) }
+
+  let(:redis_mock) { MockRedis.new }
 
   include_examples "describe store get and set methods"
+
+  describe "a generic Redis error" do
+    context "when the underlying redis store raises Redis::BaseError on get" do
+      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)
+      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)
+      end
+    end
+  end
 end