From fd71efae3b7c2333fc6c9b132b5ad20d8f4bec41 Mon Sep 17 00:00:00 2001 From: Juan Carlos Garcia Date: Sat, 9 Dec 2023 23:45:02 +0100 Subject: [PATCH] feat(#19): Manage exceptions by default and allow the gem's consumer to manage them by itself --- CHANGELOG.md | 3 +- README.md | 51 ++++++++++++++++- grape-idempotency.gemspec | 3 +- lib/grape/idempotency.rb | 71 ++++++++++++++++------- lib/grape/idempotency/version.rb | 2 +- spec/idempotent_spec.rb | 96 +++++++++++++++++++++++++------- 6 files changed, 182 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dff0b34..36d8c29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ 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). -## [1.0.1] - (Next) +## [1.1.0] - (Next) ### Fix @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Feature +* [#20](https://github.com/jcagarcia/grape-idempotency/pull/20): Manage `Redis` exceptions by default and allow the gem's consumer to manage them by itself - [@jcagarcia](https://github.com/jcagarcia). * Your contribution here. ## [1.0.0] - 2023-11-23 diff --git a/README.md b/README.md index fc9fe7a..5712e1c 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,8 @@ Topics covered in this README: - [Installation](#installation-) - [Basic Usage](#basic-usage-) - [How it works](#how-it-works-) -- [Making idempotency key header mandatory](#making-idempotency-key-header-mandatory-) + - [Making idempotency key header mandatory](#making-idempotency-key-header-mandatory-) + - [Redis Storage Connectivity Issue](#redis-storage-connectivity-issue) - [Configuration](#configuration-) - [Changelog](#changelog) - [Contributing](#contributing) @@ -81,7 +82,7 @@ Results are only saved if an API endpoint begins its execution. If incoming para Additionally, this gem automatically appends the `Original-Request` header and the `Idempotency-Key` header to your API's response, enabling you to trace back to the initial request that generated that specific response. -## Making idempotency key header mandatory ⚠️ +### Making idempotency key header mandatory ⚠️ For some endpoints, you want to enforce your consumers to provide idempotency key. So, when wrapping the code inside the `idempotent` method, you can mark it as `required`: @@ -113,6 +114,14 @@ If the Idempotency-Key request header is missing for a idempotent operation requ If you want to change the error message returned in this scenario, check [How to configure idempotency key missing error message](#mandatory_header_response) section. +### Redis Storage Connectivity Issue + +By default, the `grape-idempotency` gem is configured to handle potential `Redis` exceptions. + +Therefore, if an exception arises while attempting to read, write or delete data from the `Redis` storage, the gem will safeguard against a crash by returning empty results. Consequently, the associated `block` will execute without considering potential prior calls made with the provided idempotency key. As a result, the expected idempotent behavior will not be enforced. + +If you want to avoid this functionality, you have the option to configure the gem to refrain from handling these `Redis` exceptions. Please refer to the [manage_redis_exceptions](#manage_redis_exceptions) configuration property. + ## Configuration 🪚 In addition to the storage aspect, you have the option to supply additional configuration details to tailor the gem to the specific requirements of your project. @@ -195,6 +204,44 @@ I, [2023-11-23T22:41:39.148523 #1] DEBUG -- : [my-own-prefix] Request has been I, [2023-11-23T22:41:39.148537 #1] DEBUG -- : [my-own-prefix] Returning the response from the original request. ``` +### manage_redis_exceptions + +By default, the `grape-idempotency` gem is configured to handle potential `Redis` exceptions. + +However, this approach carries a certain level of risk. In the case that `Redis` experiences an outage, the idempotent functionality will be lost, and this issue may go unnoticed. + +If you prefer to take control of handling potential `Redis` exceptions, you have the option to configure the gem to abstain from managing Redis exceptions. + +```ruby +Grape::Idempotency.configure do |c| + c.storage = @storage + c.manage_redis_exceptions = false +end +``` + +Now, if a `Redis` exception arises while attempting to utilize the `Redis` storage, the gem will re-raise the identical exception to your application. Thus, you will be responsible for handling it within your own code, such as: + +```ruby +require 'grape' +require 'grape-idempotency' + +class API < Grape::API + post '/payments' do + begin + idempotent do + status 201 + Payment.create!({ + amount: params[:amount] + }) + end + rescue Redis::BaseError => e + error!("Redis error! Idempotency is very important here and we cannot continue.", 500) + end + end + end +end +``` + ### conflict_error_response When providing a `Idempotency-Key: ` header, this gem compares incoming parameters to those of the original request (if exists) and returns a `409 - Conflict` status code if they don't match, preventing accidental misuse. The response body returned by the gem looks like: diff --git a/grape-idempotency.gemspec b/grape-idempotency.gemspec index c05ee69..3daffc6 100644 --- a/grape-idempotency.gemspec +++ b/grape-idempotency.gemspec @@ -22,7 +22,8 @@ Gem::Specification.new do |spec| spec.required_ruby_version = '>= 2.6' - spec.add_runtime_dependency 'grape', '~> 1' + spec.add_runtime_dependency 'grape', '>= 1' + spec.add_runtime_dependency 'redis', '>= 4' spec.add_development_dependency 'bundler' spec.add_development_dependency 'rspec' diff --git a/lib/grape/idempotency.rb b/lib/grape/idempotency.rb index 3318aa1..844127f 100644 --- a/lib/grape/idempotency.rb +++ b/lib/grape/idempotency.rb @@ -1,4 +1,5 @@ require 'grape' +require 'redis' require 'logger' require 'securerandom' require 'grape/idempotency/version' @@ -33,21 +34,21 @@ def idempotent(grape, required: false, &block) grape.error!(configuration.mandatory_header_response, 400) if required && !idempotency_key return block.call if !idempotency_key - cached_request = get_from_cache(idempotency_key) - log(:debug, "Request has been found for the provided idempotency key => #{cached_request}") if cached_request - if cached_request && (cached_request["params"] != grape.request.params || cached_request["path"] != grape.request.path) - log(:debug, "Request has conflicts. Same params? => #{cached_request["params"] != grape.request.params}. Same path? => #{cached_request["path"] != grape.request.path}") + stored_request = get_from_storage(idempotency_key) + log(:debug, "Request has been found for the provided idempotency key => #{stored_request}") if stored_request + if stored_request && (stored_request["params"] != grape.request.params || stored_request["path"] != grape.request.path) + log(:debug, "Request has conflicts. Same params? => #{stored_request["params"] != grape.request.params}. Same path? => #{stored_request["path"] != grape.request.path}") log(:debug, "Returning conflict error response.") grape.error!(configuration.conflict_error_response, 422) - elsif cached_request && cached_request["processing"] == true + elsif stored_request && stored_request["processing"] == true log(:debug, "Returning processing error response.") grape.error!(configuration.processing_response, 409) - elsif cached_request + elsif stored_request log(:debug, "Returning the response from the original request.") - grape.status cached_request["status"] - grape.header(ORIGINAL_REQUEST_HEADER, cached_request["original_request"]) + grape.status stored_request["status"] + grape.header(ORIGINAL_REQUEST_HEADER, stored_request["original_request"]) grape.header(configuration.idempotency_key_header, idempotency_key) - return cached_request["response"] + return stored_request["response"] end log(:debug, "Previous request information has NOT been found for the provided idempotency key.") @@ -78,24 +79,28 @@ def idempotent(grape, required: false, &block) grape.body response rescue => e log(:debug, "An unexpected error was raised when performing the block.") - if !cached_request && !response + if !stored_request && !response validate_config! log(:debug, "Storing error response.") 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) - log(:debug, "Error response stored.") - grape.header(ORIGINAL_REQUEST_HEADER, original_request_id) - grape.header(configuration.idempotency_key_header, stored_key) + if stored_key + log(:debug, "Error response stored.") + grape.header(ORIGINAL_REQUEST_HEADER, original_request_id) + grape.header(configuration.idempotency_key_header, stored_key) + end end log(:debug, "Re-raising the error.") raise ensure - if !cached_request && response + if !stored_request && response validate_config! log(:debug, "Storing response.") stored_key = store_request_response(idempotency_key, grape.request.path, grape.request.params, grape.status, original_request_id, response) - log(:debug, "Response stored.") - grape.header(configuration.idempotency_key_header, stored_key) + if stored_key + log(:debug, "Response stored.") + grape.header(configuration.idempotency_key_header, stored_key) + end end end @@ -113,6 +118,10 @@ def update_error_with_rescue_from_result(error, status, response) store_request_response(idempotency_key, path, params, status, original_request_id, response) storage.del(stored_error[:error_key]) + rescue Redis::BaseError => e + log(:error, "Storage error => #{e.message} - #{e}") + return if configuration.manage_redis_exceptions + raise end private @@ -122,7 +131,10 @@ def validate_config! end def valid_storage? - configuration.storage && configuration.storage.respond_to?(:set) + configuration.storage && + configuration.storage.respond_to?(:get) && + configuration.storage.respond_to?(:set) && + configuration.storage.respond_to?(:del) end def get_idempotency_key(headers) @@ -141,11 +153,15 @@ def get_request_id(headers) request_id || "req_#{SecureRandom.hex}" end - def get_from_cache(idempotency_key) + def get_from_storage(idempotency_key) value = storage.get(key(idempotency_key)) return unless value JSON.parse(value) + rescue Redis::BaseError => e + log(:error, "Storage error => #{e.message} - #{e}") + return if configuration.manage_redis_exceptions + raise end def store_processing_request(idempotency_key, path, params, request_id) @@ -157,6 +173,9 @@ def store_processing_request(idempotency_key, path, params, request_id) } storage.set(key(idempotency_key), body.to_json, ex: configuration.expires_in, nx: true) + rescue Redis::BaseError => e + return true if configuration.manage_redis_exceptions + raise end def store_request_response(idempotency_key, path, params, status, request_id, response) @@ -171,6 +190,10 @@ def store_request_response(idempotency_key, path, params, status, request_id, re storage.set(key(idempotency_key), body.to_json, ex: configuration.expires_in, nx: false) idempotency_key + rescue Redis::BaseError => e + log(:error, "Storage error => #{e.message} - #{e}") + return if configuration.manage_redis_exceptions + raise end def store_error_request(idempotency_key, path, params, status, request_id, error) @@ -188,6 +211,10 @@ def store_error_request(idempotency_key, path, params, status, request_id, error storage.set(error_key(idempotency_key), body, ex: 30, nx: false) idempotency_key + rescue Redis::BaseError => e + log(:error, "Storage error => #{e.message} - #{e}") + return if configuration.manage_redis_exceptions + raise end def get_error_request_for(error) @@ -207,6 +234,10 @@ def get_error_request_for(error) } end end.first + rescue Redis::BaseError => e + log(:error, "Storage error => #{e.message} - #{e}") + return if configuration.manage_redis_exceptions + raise end def is_an_error?(response) @@ -252,7 +283,8 @@ def configuration class Configuration attr_accessor :storage, :logger, :logger_level, :logger_prefix, :expires_in, :idempotency_key_header, - :request_id_header, :conflict_error_response, :processing_response, :mandatory_header_response + :request_id_header, :conflict_error_response, :processing_response, :mandatory_header_response, + :manage_redis_exceptions class Error < StandardError; end @@ -264,6 +296,7 @@ def initialize @expires_in = 216_000 @idempotency_key_header = "idempotency-key" @request_id_header = "x-request-id" + @manage_redis_exceptions = true @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." diff --git a/lib/grape/idempotency/version.rb b/lib/grape/idempotency/version.rb index 6f43466..69a9b5a 100644 --- a/lib/grape/idempotency/version.rb +++ b/lib/grape/idempotency/version.rb @@ -2,6 +2,6 @@ module Grape module Idempotency - VERSION = '1.0.1' + VERSION = '1.1.0' end end \ No newline at end of file diff --git a/spec/idempotent_spec.rb b/spec/idempotent_spec.rb index 36c22e5..de006a5 100644 --- a/spec/idempotent_spec.rb +++ b/spec/idempotent_spec.rb @@ -22,13 +22,13 @@ it 'is registered as grape helper' do expected_response_body = { payments: [] } - + app.post('/payments') do idempotent do status 401 end end - + post 'payments', { amount: 100_00 }.to_json end @@ -37,19 +37,19 @@ context 'and all the parameters matches with the original request' do it 'returns the original response' do allow(SecureRandom).to receive(:random_number).and_return(1, 2) - + app.post('/payments') do idempotent do status 200 { amount_to: SecureRandom.random_number }.to_json end end - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json expect(last_response.status).to eq(200) expect(last_response.body).to eq({ amount_to: 1 }.to_json) - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json expect(last_response.status).to eq(200) @@ -65,10 +65,10 @@ end expect(storage).to receive(:set).twice.and_call_original - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json end @@ -76,20 +76,20 @@ it 'includes the original request id and the idempotency key in the response headers' do original_request_id = "a-request-identifier" allow(SecureRandom).to receive(:random_number).and_return(1, 2) - + app.post('/payments') do idempotent do status 200 { amount_to: SecureRandom.random_number }.to_json end end - + header "idempotency-key", idempotency_key header "x-request-id", original_request_id post 'payments?locale=es', { amount: 100_00 }.to_json expect(last_response.headers).to include("original-request" => original_request_id) expect(last_response.headers).to include("idempotency-key" => idempotency_key) - + header "idempotency-key", idempotency_key header "x-request-id", 'another-request-id' post 'payments?locale=es', { amount: 100_00 }.to_json @@ -100,18 +100,18 @@ context 'when the original request does not include a request id header' do it 'generates a new one' do allow(SecureRandom).to receive(:hex).and_return("123456") - + app.post('/payments') do idempotent do status 200 { amount_to: SecureRandom.random_number }.to_json end end - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json expect(last_response.headers).to include("original-request" => "req_123456") - + header "idempotency-key", idempotency_key header "x-request-id", 'another-request-id' post 'payments?locale=es', { amount: 100_00 }.to_json @@ -196,7 +196,7 @@ end end end - + context 'and there is NOT a response already stored 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" } @@ -264,12 +264,12 @@ end end end - + 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\"}") - + header "idempotency-key", idempotency_key post 'payments?locale=es', { amount: 100_00 }.to_json expect(last_response.status).to eq(500) @@ -294,12 +294,12 @@ { 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) @@ -315,18 +315,74 @@ expected_response_body.to_json end end - + header "idempotency-key", idempotency_key post 'payments?locale=undefined', { amount: 100_00 }.to_json expect(last_response.headers).to include("idempotency-key" => idempotency_key) end end + + context 'and a Redis exception appears in any of the redis methods' do + before do + allow(storage).to receive(:get).and_raise(Redis::CannotConnectError) + allow(storage).to receive(:set).and_raise(Redis::CannotConnectError) + allow(storage).to receive(:del).and_raise(Redis::CannotConnectError) + end + + it 'manages the exeception and the endpoint behaves as no idempotent' do + allow(SecureRandom).to receive(:random_number).and_return(1, 2) + + app.post('/payments') do + idempotent do + status 201 + { amount_to: SecureRandom.random_number }.to_json + end + end + + header "idempotency-key", idempotency_key + post 'payments', { amount: 100_00 }.to_json + expect(last_response.body).to eq({ amount_to: 1 }.to_json) + + header "idempotency-key", idempotency_key + post 'payments', { amount: 100_00 }.to_json + expect(last_response.body).to eq({ amount_to: 2 }.to_json) + end + + context 'and the gem is configured for NOT managing Redis exceptions' do + before do + Grape::Idempotency.configure do |c| + c.storage = storage + c.manage_redis_exceptions = false + end + end + + after do + Grape::Idempotency.configure do |c| + c.storage = storage + end + end + + it 'raises the redis exception' do + app.post('/payments') do + idempotent do + status 201 + { amount_to: SecureRandom.random_number }.to_json + end + end + + expect { + header "idempotency-key", idempotency_key + post 'payments', { amount: 100_00 }.to_json + }.to raise_error(Redis::CannotConnectError) + end + end + end end context 'and a idempotency key is NOT provided in the request' do it 'returns the block result in the response body without checking idempotency' do allow(SecureRandom).to receive(:random_number).and_return(1, 2) - + app.post('/payments') do idempotent do status 201