Add processing response (#11)

* Add processing response

* Return 409 instead of 102 for processing requests

* Update responses

* Fix json example response

README.md

@@ -74,6 +74,7 @@ To execute an idempotent request, simply request your user to include an extra `
 This gem operates by storing the initial request's status code and response body, regardless of whether the request succeeded or failed, using a specific idempotency key. Subsequent requests with the same key will consistently yield the same result, even if there were 500 errors.
 
 Keys are automatically removed from the system if they are at least 24 hours old, and a new request is generated when a key is reused after the original has been removed. The idempotency layer compares incoming parameters to those of the original request and returns a `409 - Conflict` status code if they don't match, preventing accidental misuse.
+If a request is received while another one with the same idempotency key is still being processed the idempotency layer returns a `409 - Conflict` status
 
 Results are only saved if an API endpoint begins its execution. If incoming parameters fail validation or if the request conflicts with another one executing concurrently, no idempotent result is stored because no API endpoint has initiated execution. In such cases, retrying these requests is safe.
 
@@ -133,7 +134,8 @@ When providing a `Idempotency-Key: <key>` header, this gem compares incoming par
 ```json
 {
 
-  "error": "You are using the same idempotent key for two different requests"
+  "title": "Idempotency-Key is already used",
+  "detail": "This operation is idempotent and it requires correct usage of Idempotency Key. Idempotency Key MUST not be reused across different payloads of this operation."
 }
 ```
 
@@ -151,6 +153,32 @@ Grape::Idempotency.configure do |c|
 end
 ```
 
+### processing_response
+
+When a request with a `Idempotency-Key: <key>` header is performed while a previous one still on going with the same idempotency value, this gem returns a `409 - Conflict` status. Thre response body returned by the gem looks like:
+
+```json
+{
+
+  "title": "A request is outstanding for this Idempotency-Key",
+  "detail": "A request with the same idempotent key for the same operation is being processed or is outstanding."
+}
+```
+
+You have the option to specify the desired response body to be returned to your users when this error occurs. This allows you to align the error format with the one used in your application.
+
+```ruby
+Grape::Idempotency.configure do |c|
+  c.storage = @storage
+  c.processing_response = {
+    "type": "about:blank",
+    "status": 409,
+    "title": "A request is still being processed",
+    "detail": "A request with the same idempotent key is being procesed"
+}
+end
+```
+
 In the configuration above, the error is following the [RFC-7807](https://datatracker.ietf.org/doc/html/rfc7807) format.
 
 ## Changelog

lib/grape/idempotency.rb

@@ -31,20 +31,29 @@ def idempotent(grape, &block)
         if cached_request && (cached_request["params"] != grape.request.params || cached_request["path"] != grape.request.path)
           grape.status 409
           return configuration.conflict_error_response
+        elsif cached_request && cached_request["processing"] == true
+          grape.status 409
+          return configuration.processing_response.to_json
         elsif cached_request
           grape.status cached_request["status"]
           grape.header(ORIGINAL_REQUEST_HEADER, cached_request["original_request"])
           grape.header(configuration.idempotency_key_header, idempotency_key)
           return cached_request["response"]
         end
 
+        original_request_id = get_request_id(grape.request.headers)
+        success = store_processing_request(idempotency_key, grape.request.path, grape.request.params, original_request_id)
+        if !success
+          grape.status 409
+          return configuration.processing_response
+        end
+
         response = catch(:error) do
           block.call
         end
 
         response = response[:message] if is_an_error?(response)
 
-        original_request_id = get_request_id(grape.request.headers)
         grape.header(ORIGINAL_REQUEST_HEADER, original_request_id)
         grape.body response
       rescue => e
@@ -59,7 +68,7 @@ def idempotent(grape, &block)
       ensure
         if !cached_request && response
           validate_config!
-          stored_key = store_request(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, response)
+          stored_key = store_request_response(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, response)
           grape.header(configuration.idempotency_key_header, stored_key)
         end
       end
@@ -76,7 +85,7 @@ def update_error_with_rescue_from_result(error, status, response)
         params = request_with_unmanaged_error["params"]
         original_request_id = request_with_unmanaged_error["original_request"]
 
-        store_request(idempotency_key, path, params, status, original_request_id, response)
+        store_request_response(idempotency_key, path, params, status, original_request_id, response)
         storage.del(stored_error[:error_key])
       end
 
@@ -113,22 +122,29 @@ def get_from_cache(idempotency_key)
         JSON.parse(value)
       end
 
-      def store_request(idempotency_key, path, params, status, request_id, response)
+      def store_processing_request(idempotency_key, path, params, request_id)
+        body = {
+          path: path,
+          params: params,
+          original_request: request_id,
+          processing: true
+        }
+
+        storage.set(key(idempotency_key), body.to_json, ex: configuration.expires_in, nx: true)
+      end
+
+      def store_request_response(idempotency_key, path, params, status, request_id, response)
         body = {
           path: path,
           params: params,
           status: status,
           original_request: request_id,
           response: response
-        }.to_json
+        }
 
-        result = storage.set(key(idempotency_key), body, ex: configuration.expires_in, nx: true)
+        storage.set(key(idempotency_key), body.to_json, ex: configuration.expires_in, nx: false)
 
-        if !result
-          return store_request(random_idempotency_key, path, params, status, request_id, response)
-        else
-          return idempotency_key
-        end
+        idempotency_key
       end
 
       def store_error_request(idempotency_key, path, params, status, request_id, error)
@@ -143,13 +159,9 @@ def store_error_request(idempotency_key, path, params, status, request_id, error
           }
         }.to_json
 
-        result = storage.set(error_key(idempotency_key), body, ex: 30, nx: true)
+        storage.set(error_key(idempotency_key), body, ex: 30, nx: false)
 
-        if !result
-          return store_error_request(random_idempotency_key, path, params, status, request_id, error)
-        else
-          return idempotency_key
-        end
+        idempotency_key
       end
 
       def get_error_request_for(error)
@@ -191,16 +203,6 @@ def gem_prefix
         "grape:idempotency:"
       end
 
-      def random_idempotency_key
-        tentative_key = SecureRandom.uuid
-        already_existing_key = storage.get(key(tentative_key))
-        if already_existing_key
-          return random_idempotency_key
-        else
-          return tentative_key
-        end
-      end
-
       def storage
         configuration.storage
       end
@@ -211,7 +213,7 @@ def configuration
     end
 
     class Configuration
-      attr_accessor :storage, :expires_in, :idempotency_key_header, :request_id_header, :conflict_error_response
+      attr_accessor :storage, :expires_in, :idempotency_key_header, :request_id_header, :conflict_error_response, :processing_response
 
       class Error < StandardError; end
 
@@ -220,8 +222,13 @@ def initialize
         @expires_in = 216_000
         @idempotency_key_header = "idempotency-key"
         @request_id_header = "x-request-id"
-        @conflict_error_response = { 
-          "error" => "You are using the same idempotent key for two different requests"
+        @conflict_error_response = {
+          "title" => "Idempotency-Key is already used",
+          "detail" => "This operation is idempotent and it requires correct usage of Idempotency Key. Idempotency Key MUST not be reused across different payloads of this operation."
+        }
+        @processing_response = {
+          "title" => "A request is outstanding for this Idempotency-Key",
+          "detail" => "A request with the same idempotent key for the same operation is being processed or is outstanding."
         }
       end
     end

spec/idempotent_spec.rb

@@ -64,7 +64,7 @@
                   end
                 end
 
-                expect(storage).to receive(:set).once.and_call_original
+                expect(storage).to receive(:set).twice.and_call_original
 
                 header "idempotency-key", idempotency_key
                 post 'payments?locale=es', { amount: 100_00 }.to_json
@@ -162,7 +162,7 @@
                 header "idempotency-key", idempotency_key
                 post 'payments?locale=en', { amount: 800_00 }.to_json
                 expect(last_response.status).to eq(409)
-                expect(last_response.body).to eq("{\"error\"=>\"You are using the same idempotent key for two different requests\"}")
+                expect(last_response.body).to eq("{\"title\"=>\"Idempotency-Key is already used\", \"detail\"=>\"This operation is idempotent and it requires correct usage of Idempotency Key. Idempotency Key MUST not be reused across different payloads of this operation.\"}")
               end
             end
 
@@ -192,35 +192,63 @@
                 header "idempotency-key", idempotency_key
                 post 'refunds?locale=es', { amount: 100_00 }.to_json
                 expect(last_response.status).to eq(409)
-                expect(last_response.body).to eq("{\"error\"=>\"You are using the same idempotent key for two different requests\"}")
+                expect(last_response.body).to eq("{\"title\"=>\"Idempotency-Key is already used\", \"detail\"=>\"This operation is idempotent and it requires correct usage of Idempotency Key. Idempotency Key MUST not be reused across different payloads of this operation.\"}")
               end
             end
           end
 
           context 'and there is NOT a response already stored in the storage' do
-            it 'stores the response in the storage' do
+            it 'stores the the request as processing initially without overwritting and later stores the response' do
               expected_response_body = { error: "Internal Server Error" }
               app.post('/payments') do
                 idempotent do
                   status 500
                   expected_response_body.to_json
                 end
               end
-  
+
               expect(storage).to receive(:set) do |key, body, opts|
                 expect(key).to eq("grape:idempotency:#{idempotency_key}")
                 json_body = JSON.parse(body, symbolize_names: true)
-                expect(json_body[:response]).to eq(expected_response_body.to_json)
                 expect(json_body[:path]).to eq("/payments")
                 expect(json_body[:params]).to eq({:"locale"=>"undefined", :"{\"amount\":10000}"=>nil})
-                expect(json_body[:status]).to eq(500)
                 expect(opts).to eq({ex: 216_000, nx: true})
               end
-
+
+              expect(storage).to receive(:set) do |key, body, opts|
+                expect(key).to eq("grape:idempotency:#{idempotency_key}")
+                json_body = JSON.parse(body, symbolize_names: true)
+                expect(json_body[:path]).to eq("/payments")
+                expect(json_body[:params]).to eq({:"locale"=>"undefined", :"{\"amount\":10000}"=>nil})
+                expect(json_body[:status]).to eq(500)
+                expect(json_body[:response]).to eq(expected_response_body.to_json)
+                expect(opts).to eq({ex: 216_000, nx: false})
+              end
+
+
               header "idempotency-key", idempotency_key
               post 'payments?locale=undefined', { amount: 100_00 }.to_json
             end
 
+            context 'but there is no possible to store the request as processing' do
+              it 'returns conflict' do
+                app.post('/payments') do
+                  idempotent do
+                    status 500
+                    { some: 'value' }.to_json
+                  end
+                end
+
+                expect(storage).to receive(:set).and_return(false)
+
+                header "idempotency-key", idempotency_key
+                post 'payments?locale=es', { amount: 100_00 }.to_json
+
+                expect(last_response.status).to eq(409)
+                expect(last_response.body).to eq("{\"title\"=>\"A request is outstanding for this Idempotency-Key\", \"detail\"=>\"A request with the same idempotent key for the same operation is being processed or is outstanding.\"}")
+              end
+            end
+
             context 'and a managed exception appears executing the code' do
               it 'stores the exception response and returns the same response in the second call' do
                 allow(SecureRandom).to receive(:random_number).and_return(1, 2)
@@ -292,25 +320,6 @@
               post 'payments?locale=undefined', { amount: 100_00 }.to_json
               expect(last_response.headers).to include("idempotency-key" => idempotency_key)
             end
-
-            context 'because parallel requests and not stored yet when performing the check' do
-              it 'stores the request using a new random idempotency key and returns it in the header response' do
-                expected_idempotency_key = 'a-idempotency-key-value'
-                app.post('/payments') do
-                  idempotent do
-                    status 201
-                    { }.to_json
-                  end
-                end
-
-                allow(SecureRandom).to receive(:uuid).and_return(expected_idempotency_key)
-                allow(storage).to receive(:set).and_return(false, true)
-
-                header "idempotency-key", idempotency_key
-                post 'payments?locale=undefined', { amount: 100_00 }.to_json
-                expect(last_response.headers).to include("idempotency-key" => expected_idempotency_key)
-              end
-            end
           end
         end
 
@@ -388,6 +397,10 @@
           expect(opts).to eq({ex: 1800, nx: true})
         end
 
+        expect(storage).to receive(:set) do |key, body, opts|
+          expect(opts).to eq({ex: 1800, nx: false})
+        end
+
         header "idempotency-key", idempotency_key
         post 'payments?locale=undefined', { amount: 100_00 }.to_json
       end
@@ -503,5 +516,46 @@
         expect(last_response.body).to eq("{:error=>\"An error wadus with conflict\", :status=>409, :message=>\"You are using the same idempotency key for two different requests\"}")
       end
     end
+
+    describe 'processing_response' do
+      let(:expected_processing_request) do
+        {
+          message: "A request with the same idempotency key is being processed",
+          status: 409
+        }
+      end
+
+      before do
+        Grape::Idempotency.configure do |c|
+          c.storage = storage
+          c.processing_response = expected_processing_request
+        end
+      end
+
+      it 'returns an 409 processing http response using the configured processing response' do
+        app.post('/payments') do
+          idempotent do
+            status 200
+
+            { some: 'value' }.to_json
+          end
+        end
+
+        processing_body = {
+          path: '/payments',
+          params: { 'locale' => 'es', amount: "80000" },
+          original_request: 'request-id',
+          processing: true
+        }
+
+        allow(storage).to receive(:get).with("grape:idempotency:#{idempotency_key}").and_return(processing_body.to_json)
+
+        header "idempotency-key", idempotency_key
+        post 'payments?locale=es', { amount: 800_00 }
+
+        expect(last_response.status).to eq(409)
+        expect(last_response.body).to eq(expected_processing_request.to_json)
+      end
+    end
   end
 end