From 10fe42448a482f7028250a311d9b883bfbf11bd5 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Fri, 9 Sep 2022 18:58:25 +0100
Subject: [PATCH 1/2] Add reason attribute to ServiceResponse

Instead of using `http_status` which is an infrastructure
layer responsibility we can use `reason` which is more generic
and can be better used for internal domain service objects.

Endpoints can then convert the `reason` value into HTTP status.
---
 app/services/service_response.rb       | 26 +++++++++++++++++++-------
 spec/services/service_response_spec.rb | 15 ++++++++++++---
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index c7ab75a44266c0..848f90e7f25116 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -2,20 +2,28 @@
 
 class ServiceResponse
   def self.success(message: nil, payload: {}, http_status: :ok)
-    new(status: :success, message: message, payload: payload, http_status: http_status)
+    new(status: :success,
+        message: message,
+        payload: payload,
+        http_status: http_status)
   end
 
-  def self.error(message:, payload: {}, http_status: nil)
-    new(status: :error, message: message, payload: payload, http_status: http_status)
+  def self.error(message:, payload: {}, http_status: nil, reason: nil)
+    new(status: :error,
+        message: message,
+        payload: payload,
+        http_status: http_status,
+        reason: reason)
   end
 
-  attr_reader :status, :message, :http_status, :payload
+  attr_reader :status, :message, :http_status, :payload, :reason
 
-  def initialize(status:, message: nil, payload: {}, http_status: nil)
+  def initialize(status:, message: nil, payload: {}, http_status: nil, reason: nil)
     self.status = status
     self.message = message
     self.payload = payload
     self.http_status = http_status
+    self.reason = reason
   end
 
   def track_exception(as: StandardError, **extra_data)
@@ -41,7 +49,11 @@ def [](key)
   end
 
   def to_h
-    (payload || {}).merge(status: status, message: message, http_status: http_status)
+    (payload || {}).merge(
+      status: status,
+      message: message,
+      http_status: http_status,
+      reason: reason)
   end
 
   def success?
@@ -60,5 +72,5 @@ def errors
 
   private
 
-  attr_writer :status, :message, :http_status, :payload
+  attr_writer :status, :message, :http_status, :payload, :reason
 end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 3ede90fbc44cb0..2d70979dd3a5ce 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -43,14 +43,14 @@
   end
 
   describe '.error' do
-    it 'creates a failed response without HTTP status' do
+    it 'creates an error response without HTTP status' do
       response = described_class.error(message: 'Bad apple')
 
       expect(response).to be_error
       expect(response.message).to eq('Bad apple')
     end
 
-    it 'creates a failed response with HTTP status' do
+    it 'creates an error response with HTTP status' do
       response = described_class.error(message: 'Bad apple', http_status: 400)
 
       expect(response).to be_error
@@ -58,7 +58,7 @@
       expect(response.http_status).to eq(400)
     end
 
-    it 'creates a failed response with payload' do
+    it 'creates an error response with payload' do
       response = described_class.error(message: 'Bad apple',
                                        payload: { bad: 'apple' })
 
@@ -66,6 +66,15 @@
       expect(response.message).to eq('Bad apple')
       expect(response.payload).to eq(bad: 'apple')
     end
+
+    it 'creates an error response with a reason' do
+      response = described_class.error(message: 'Bad apple',
+                                       reason: :permission_denied)
+
+      expect(response).to be_error
+      expect(response.message).to eq('Bad apple')
+      expect(response.reason).to eq(:permission_denied)
+    end
   end
 
   describe '#success?' do
-- 
GitLab


From 3c2918c36c56549835aab6adaa040448ce6012c3 Mon Sep 17 00:00:00 2001
From: Fabio Pitino <fpitino@gitlab.com>
Date: Mon, 12 Sep 2022 13:20:24 +0100
Subject: [PATCH 2/2] Document the use of reason

---
 doc/development/reusing_abstractions.md | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md
index ef4e8b0310fba8..826782d7036770 100644
--- a/doc/development/reusing_abstractions.md
+++ b/doc/development/reusing_abstractions.md
@@ -206,6 +206,31 @@ response = ServiceResponse.success(payload: { issue: issue })
 response.payload[:issue] # => issue
 ```
 
+Error responses can also specify the failure `reason` which can be used by the caller
+to understand the nature of the failure.
+The caller, if an HTTP endpoint, could translate the reason symbol into an HTTP status code:
+
+```ruby
+response = ServiceResponse.error(
+  message: 'Job is in a state that cannot be retried',
+  reason: :job_not_retrieable)
+
+if response.success?
+  head :ok
+if response.reason == :job_not_retriable
+  head :unprocessable_entity
+else
+  head :bad_request
+end
+```
+
+For common failures such as resource `:not_found` or operation `:forbidden`, we could
+leverage the Rails [HTTP status symbols](http://www.railsstatuscodes.com/) as long as
+they are sufficiently specific for the domain logic involved.
+For other failures use domain-specific reasons whenever possible.
+
+For example: `:job_not_retriable`, `:duplicate_package`, `:merge_request_not_mergeable`.
+
 ### Finders
 
 Everything in `app/finders`, typically used for retrieving data from a database.
-- 
GitLab