Minor improvements: storage move, small behaviour tweaks & simplistic…

… specs (guitsaru#3)

README.md

@@ -28,12 +28,12 @@ Rack::Idempotency is implemented as a piece of Rack middleware and can be used w
 ```ruby
 require "rack/idempotency"
 
-use Rack::Idempotency, store: Rack::Idempotency::MemoryStore.new
+use Rack::Idempotency, store: Rack::Idempotency::Store::Memory.new
 
 run app
 ```
 
-The `store` argument should be any object that responds to both `read(id)` and `write(id, value)`.  `Rack::Idempotency::MemoryStore` is good for testing, but should not be used in production.
+The `store` argument should be any object that responds to both `read(id)` and `write(id, value)`.  `Rack::Idempotency::Store::Memory` is good for testing, but should not be used in production.
 
 ## Using with Rails
 

lib/rack/idempotency.rb

@@ -5,9 +5,10 @@
 require 'rack/idempotency/version'
 
 require 'rack/idempotency/errors'
-require 'rack/idempotency/memory_store'
-require 'rack/idempotency/null_store'
-require 'rack/idempotency/redis_store'
+require 'rack/idempotency/store/base'
+require 'rack/idempotency/store/memory'
+require 'rack/idempotency/store/null'
+require 'rack/idempotency/store/redis'
 require 'rack/idempotency/request'
 require 'rack/idempotency/request_storage'
 require 'rack/idempotency/response'
@@ -19,7 +20,7 @@ module Rack
   # the given cache.  When the client retries, it will get the previously
   # cached response.
   class Idempotency
-    def initialize(app, store: NullStore.new)
+    def initialize(app, store: Store::Null.new)
       @app     = app
       @store   = store
     end
@@ -36,7 +37,7 @@ def call(env)
     def store_response(storage, env)
       response = Response.new(*@app.call(env))
 
-      storage.write(response) if response.success?
+      storage.write(response)
 
       response.to_a
     end

lib/rack/idempotency/errors.rb

@@ -4,7 +4,8 @@ module Rack
   class Idempotency
     # For all errors
     class Error < RuntimeError
-      attr :env
+      attr_reader :env
+
       def initialize(env)
         @env = env
       end

lib/rack/idempotency/request.rb

@@ -7,7 +7,7 @@ class Idempotency
     class Request < Rack::Request
       def idempotency_key
         get_header('HTTP_IDEMPOTENCY_KEY').tap do |key|
-          unless key.nil? || key.match?(/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i)
+          unless key.nil? || key.match?(/^[\da-f]{8}-[\da-f]{4}-[1-5][\da-f]{3}-[89ab][\da-f]{3}-[\da-f]{12}$/i)
             raise InsecureKeyError.new(env), 'Idempotency-Key must be a valid UUID'
           end
         end

lib/rack/idempotency/request_storage.rb

@@ -30,15 +30,12 @@ def write(response)
       attr_reader :request, :store
 
       def key
-        Digest::SHA256.hexdigest(hashed_structure.to_s)
+        "idempotency:#{digest}"
       end
 
-      def hashed_structure
-        [
-          request.idempotency_key,
-          request.env.reject { |key, _value| key.start_with?('HTTP_') || key.start_with?('rack.') },
-          request.body.read
-        ]
+      def digest
+        digestable = "#{request.idempotency_key}:#{request.body.read}"
+        Digest::SHA256.hexdigest(digestable)
       end
     end
   end

lib/rack/idempotency/response.rb

@@ -13,10 +13,6 @@ def initialize(status, headers, body)
         @body    = body
       end
 
-      def success?
-        status.between?(200, 400)
-      end
-
       def to_a
         [status, headers.to_hash, body.map(&:to_s)]
       end

lib/rack/idempotency/store/base.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Rack
+  class Idempotency
+    class Store
+      # Basic interface for a store
+      class Base
+        def read(_key)
+          raise NotImplementedError
+        end
+
+        def write(_key, _value)
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+end

lib/rack/idempotency/store/memory.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Rack
+  class Idempotency
+    class Store
+      # Stores idempotency information in a Hash.
+      class Memory < Base
+        def initialize
+          @store = {}
+          super
+        end
+
+        def read(key)
+          store[key]
+        end
+
+        def write(key, value)
+          store[key] = value
+        end
+
+        private
+
+        attr_reader :store
+      end
+    end
+  end
+end

lib/rack/idempotency/store/null.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Rack
+  class Idempotency
+    class Store
+      # Most basic version of the store.  This class doesn't read or write.
+      class Null < Base
+        def read(_key); end
+
+        def write(_key, _value); end
+      end
+    end
+  end
+end

lib/rack/idempotency/store/redis.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'redis'
+
+module Rack
+  class Idempotency
+    # Stores idempotency information in a provided Redis instance.
+    class Store
+      class Redis < Base
+        DEFAULT_TTL = 86400
+
+        # @param client [::Redis] A Redis client used to retrieve the requests.
+        # @param ttl [#to_i, nil] Expiry duration in seconds.
+        def initialize(client:, ttl: DEFAULT_TTL)
+          @client = client
+          @ttl = ttl.to_i
+        end
+
+        def read(key)
+          client.get(key)
+        end
+
+        def write(key, value)
+          if ttl.positive?
+            client.setex(key, ttl, value)
+          else
+            client.set(key, value)
+          end
+        end
+
+        private
+
+        attr_reader :client, :ttl
+      end
+    end
+  end
+end

spec/rack/idempotency/store/base_spec.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Rack::Idempotency::Store::Base do
+  describe '#write' do
+    subject { described_class.new.write('key', 'd353c6836cc8c2a0bcf79626bb64bc48032e33ece8e8407c4cb65830965fa814') }
+
+    it 'raises NotImplementedError' do
+      expect { subject }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#read' do
+    subject { described_class.new.read('key') }
+
+    it 'raises NotImplementedError' do
+      expect { subject }.to raise_error(NotImplementedError)
+    end
+  end
+end

spec/rack/idempotency/store/memory_spec.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Rack::Idempotency::Store::Memory do
+  let(:store) { described_class.new }
+  let(:value) { 'd353c6836cc8c2a0bcf79626bb64bc48032e33ece8e8407c4cb65830965fa814' }
+
+  describe '#write' do
+    subject { store.write('key', value) }
+
+    it 'stores value in memory' do
+      expect(subject).to eq(value)
+      expect(store.read('key')).to eq(value)
+    end
+  end
+
+  describe '#read' do
+    subject { store.read('key') }
+
+    it 'reads value from memory' do
+      store.write('key', value)
+      expect(subject).to eq(value)
+    end
+  end
+end

spec/rack/idempotency/store/null_spec.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Rack::Idempotency::Store::Null do
+  let(:store) { described_class.new }
+  let(:value) { 'd353c6836cc8c2a0bcf79626bb64bc48032e33ece8e8407c4cb65830965fa814' }
+
+  describe '#write' do
+    subject { store.write('key', value) }
+
+    it 'stores nothing' do
+      expect(subject).to be_nil
+      expect(store.read('key')).to be_nil
+    end
+  end
+
+  describe '#read' do
+    subject { store.read('key') }
+
+    it 'reads nothing' do
+      store.write('key', value)
+      expect(subject).to be_nil
+    end
+  end
+end

spec/rack/idempotency/store/redis_spec.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Rack::Idempotency::Store::Redis do
+  let(:redis) { instance_spy(::Redis) }
+  let(:value) { 'd353c6836cc8c2a0bcf79626bb64bc48032e33ece8e8407c4cb65830965fa814' }
+
+  describe '#write' do
+    subject! { store.write('key', value) }
+
+    context 'with positive TTL' do
+      let(:store) { described_class.new(client: redis, ttl: 5) }
+
+      it 'sets the key expiring in TTL seconds' do
+        expect(redis).to have_received(:setex).with('key', 5, value).once
+      end
+    end
+
+    context 'with negative or zero TTL' do
+      let(:store) { described_class.new(client: redis, ttl: -5) }
+
+      it 'sets the key without expiration' do
+        expect(redis).to have_received(:set).with('key', value).once
+      end
+    end
+
+    context 'without a provided TTL' do
+      let(:store) { described_class.new(client: redis) }
+
+      it 'sets the key for default TTL' do
+        expect(redis).to have_received(:setex).with('key', described_class::DEFAULT_TTL, value).once
+      end
+    end
+  end
+
+  describe '#read' do
+    subject! { store.read('key') }
+
+    let(:store) { described_class.new(client: redis) }
+
+    before { redis.set('key', value) }
+
+    it 'reads value using Redis client' do
+      expect(redis).to have_received(:get).with('key')
+    end
+  end
+end