Merge pull request #9 from jcagarcia/rescue-from-fix

Store valid response when error is raised and managed in rescue_from handler

.github/workflows/gem-push.yml

@@ -1,9 +1,5 @@
 name: Ruby Gem
 
-on:
-  push:
-    branches: [ "main" ]
-
 jobs:
   build:
     name: Build + Publish

CHANGELOG.md

@@ -4,13 +4,19 @@ All changes to `grape-idempotency` will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.1.3] - 2023-01-07
+
+### Fix
+
+- Second calls were returning `null` when the first response was generated inside a `rescue_from`.
+- Conflict response had invalid format.
+
 ## [0.1.2] - 2023-01-06
 
 ### Fix
 
 - Return correct original response when the endpoint returns a hash in the body
 
-
 ## [0.1.1] - 2023-01-06
 
 ### Fix

lib/grape/idempotency.rb

@@ -1,6 +1,7 @@
 require 'grape'
 require 'securerandom'
 require 'grape/idempotency/version'
+require 'grape/idempotency/middleware/error'
 
 module Grape
   module Idempotency
@@ -29,7 +30,7 @@ def idempotent(grape, &block)
         cached_request = get_from_cache(idempotency_key)
         if cached_request && (cached_request["params"] != grape.request.params || cached_request["path"] != grape.request.path)
           grape.status 409
-          return configuration.conflict_error_response.to_json
+          return configuration.conflict_error_response
         elsif cached_request
           grape.status cached_request["status"]
           grape.header(ORIGINAL_REQUEST_HEADER, cached_request["original_request"])
@@ -41,19 +42,44 @@ def idempotent(grape, &block)
           block.call
         end
 
-        response = response[:message].to_json if is_an_error?(response)
+        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
+        if !cached_request && !response
+          validate_config!
+          original_request_id = get_request_id(grape.request.headers)
+          stored_key = store_error_request(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, e)
+          grape.header(ORIGINAL_REQUEST_HEADER, original_request_id)
+          grape.header(configuration.idempotency_key_header, stored_key)
+        end
+        raise
       ensure
-        validate_config!
-        unless cached_request
-          stored_key = store_in_cache(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, response)
+        if !cached_request && response
+          validate_config!
+          stored_key = store_request(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, response)
           grape.header(configuration.idempotency_key_header, stored_key)
         end
       end
 
+      def update_error_with_rescue_from_result(error, status, response)
+        validate_config!
+
+        stored_error = get_error_request_for(error)
+        return unless stored_error
+
+        request_with_unmanaged_error = stored_error[:request]
+        idempotency_key = stored_error[:idempotency_key]
+        path = request_with_unmanaged_error["path"]
+        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)
+        storage.del(stored_error[:error_key])
+      end
+
       private
 
       def validate_config!
@@ -87,7 +113,7 @@ def get_from_cache(idempotency_key)
         JSON.parse(value)
       end
 
-      def store_in_cache(idempotency_key, path, params, status, request_id, response)
+      def store_request(idempotency_key, path, params, status, request_id, response)
         body = {
           path: path,
           params: params,
@@ -99,18 +125,70 @@ def store_in_cache(idempotency_key, path, params, status, request_id, response)
         result = storage.set(key(idempotency_key), body, ex: configuration.expires_in, nx: true)
 
         if !result
-          return store_in_cache(random_idempotency_key, path, params, status, request_id, response)
+          return store_request(random_idempotency_key, path, params, status, request_id, response)
         else
           return idempotency_key
         end
       end
 
+      def store_error_request(idempotency_key, path, params, status, request_id, error)
+        body = {
+          path: path,
+          params: params,
+          status: status,
+          original_request: request_id,
+          error: {
+            class_name: error.class.to_s,
+            message: error.message
+          }
+        }.to_json
+
+        result = storage.set(error_key(idempotency_key), body, ex: 30, nx: true)
+
+        if !result
+          return store_error_request(random_idempotency_key, path, params, status, request_id, error)
+        else
+          return idempotency_key
+        end
+      end
+
+      def get_error_request_for(error)
+        error_keys = storage.keys("#{error_key_prefix}*")
+        return if error_keys.empty?
+
+        error_keys.map do |key|
+          request_with_error = JSON.parse(storage.get(key))
+          error_class_name = request_with_error["error"]["class_name"]
+          error_message = request_with_error["error"]["message"]
+
+          if error_class_name == error.class.to_s && error_message == error.message
+            {
+              error_key: key,
+              request: request_with_error,
+              idempotency_key: key.gsub(error_key_prefix, '')
+            }
+          end
+        end.first
+      end
+
       def is_an_error?(response)
         response.is_a?(Hash) && response.has_key?(:message) && response.has_key?(:headers) && response.has_key?(:status)
       end
 
       def key(idempotency_key)
-        "grape:idempotency:#{idempotency_key}"
+        "#{gem_prefix}#{idempotency_key}"
+      end
+
+      def error_key(idempotency_key)
+        "#{error_key_prefix}#{idempotency_key}"
+      end
+
+      def error_key_prefix
+        "#{gem_prefix}error:"
+      end
+
+      def gem_prefix
+        "grape:idempotency:"
       end
 
       def random_idempotency_key

lib/grape/idempotency/middleware/error.rb

@@ -0,0 +1,36 @@
+require 'grape/middleware/base'
+
+module Grape
+  module Middleware
+    class Error < Base
+      def run_rescue_handler(handler, error)
+        if handler.instance_of?(Symbol)
+          raise NoMethodError, "undefined method '#{handler}'" unless respond_to?(handler)
+
+          handler = public_method(handler)
+        end
+
+        response = handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler)
+
+        if response.is_a?(Rack::Response)
+          update_idempotency_error_with(error, response)
+          response
+        else
+          run_rescue_handler(:default_rescue_handler, Grape::Exceptions::InvalidResponse.new)
+        end
+      end
+
+      private
+
+      def update_idempotency_error_with(error, response)
+        begin
+          body = JSON.parse(response.body.join)
+        rescue JSON::ParserError
+          body = response.body.join
+        end
+
+        Grape::Idempotency.update_error_with_rescue_from_result(error, response.status, body)
+      end
+    end
+  end
+end

lib/grape/idempotency/version.rb

@@ -2,6 +2,6 @@
 
 module Grape
   module Idempotency
-    VERSION = '0.1.2'
+    VERSION = '0.1.3'
   end
 end

spec/idempotent_spec.rb

@@ -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" }.to_json)
+                expect(last_response.body).to eq("{\"error\"=>\"You are using the same idempotent key for two different requests\"}")
               end
             end
 
@@ -192,7 +192,7 @@
                 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" }.to_json)
+                expect(last_response.body).to eq("{\"error\"=>\"You are using the same idempotent key for two different requests\"}")
               end
             end
           end
@@ -240,12 +240,42 @@
                 header "idempotency-key", idempotency_key
                 post 'payments?locale=es', { amount: 100_00 }.to_json
                 expect(last_response.status).to eq(500)
-                expect(last_response.body).to eq("{\"error\":\"Internal Server Error\",\"message\":\"Unexpected error\"}")
+                expect(last_response.body).to eq("{:error=>\"Internal Server Error\", :message=>\"Unexpected error\"}")
 
                 header "idempotency-key", idempotency_key
                 post 'payments?locale=es', { amount: 100_00 }.to_json
                 expect(last_response.status).to eq(500)
-                expect(last_response.body).to eq("{\"error\":\"Internal Server Error\",\"message\":\"Unexpected error\"}")
+                expect(last_response.body).to eq("{\"error\"=>\"Internal Server Error\", \"message\"=>\"Unexpected error\"}")
+              end
+            end
+
+            context 'and a managed exception using grape rescue_from 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)
+
+                app.rescue_from StandardError do |e|
+                  status = 404
+                  error = { message: "Not found error" }
+                  error!(error, status)
+                end
+
+                app.post('/payments') do
+                  idempotent do
+                    raise "Not found error error" if SecureRandom.random_number == 1
+                    status 200
+                    { amount_to: 100_00 }.to_json
+                  end
+                end
+
+                header "idempotency-key", idempotency_key
+                post 'payments?locale=es', { amount: 100_00 }.to_json
+                expect(last_response.status).to eq(404)
+                expect(last_response.body).to eq("{\"message\":\"Not found error\"}")
+
+                header "idempotency-key", idempotency_key
+                post 'payments?locale=es', { amount: 100_00 }.to_json
+                expect(last_response.status).to eq(404)
+                expect(last_response.body).to eq("{\"message\"=>\"Not found error\"}")
               end
             end
 
@@ -470,7 +500,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(expected_error_response.to_json)
+        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
   end