Merge pull request #50 from github/copilot/fix-49

Implement Grape's `error!` method for handler subclasses

Author: GrantBirki

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    hooks-ruby (0.1.0)
+    hooks-ruby (0.2.0)
       dry-schema (~> 1.14, >= 1.14.1)
       grape (~> 2.3)
       puma (~> 6.6)

docs/handler_plugins.md

@@ -140,6 +140,57 @@ This section goes into details on the built-in features that exist in all handle
 
 The `log.debug`, `log.info`, `log.warn`, and `log.error` methods are available in all handler plugins. They are used to log messages at different levels of severity.
 
+### `#error!`
+
+All handler plugins have access to the `error!` method, which is used to raise an error with a specific message and HTTP status code. This is useful for returning error responses to the webhook sender.
+
+```ruby
+class Example < Hooks::Plugins::Handlers::Base
+  # Example webhook handler
+  #
+  # @param payload [Hash, String] Webhook payload
+  # @param headers [Hash<String, String>] HTTP headers
+  # @param env [Hash] A modified Rack environment that contains a lot of context about the request
+  # @param config [Hash] Endpoint configuration
+  # @return [Hash] Response data
+  def call(payload:, headers:, env:, config:)
+
+    if payload.nil? || payload.empty?
+      log.error("Payload is empty or nil")
+      error!("Payload cannot be empty or nil", 400)
+    end
+
+    return {
+      status: "success"
+    }
+  end
+end
+```
+
+You can also use the `error!` method to return a JSON response as well:
+
+```ruby
+class Example < Hooks::Plugins::Handlers::Base
+  def call(payload:, headers:, env:, config:)
+
+    if payload.nil? || payload.empty?
+      log.error("Payload is empty or nil")
+      error!({
+        error: "payload_empty",
+        message: "the payload cannot be empty or nil",
+        success: false,
+        custom_value: "some_custom_value",
+        request_id: env["hooks.request_id"]
+      }, 500)
+    end
+
+    return {
+      status: "success"
+    }
+  end
+end
+```
+
 ### `#Retryable.with_context(:default)`
 
 This method uses a default `Retryable` context to handle retries. It is used to wrap the execution of a block of code that may need to be retried in case of failure.
@@ -172,3 +223,7 @@ end
 > If `Retryable.with_context(:default)` fails after all retries, it will re-raise the last exception encountered.
 
 See the source code at `lib/hooks/utils/retry.rb` for more details on how `Retryable.with_context(:default)` works.
+
+### `#failbot` and `#stats`
+
+The `failbot` and `stats` methods are available in all handler plugins. They are used to report errors and send statistics, respectively. These are custom methods and you can learn more about them in the [Instrumentation Plugins](instrument_plugins.md) documentation.

lib/hooks/app/api.rb

@@ -7,6 +7,7 @@
 require_relative "auth/auth"
 require_relative "rack_env_builder"
 require_relative "../plugins/handlers/base"
+require_relative "../plugins/handlers/error"
 require_relative "../plugins/handlers/default"
 require_relative "../core/logger_factory"
 require_relative "../core/log"
@@ -110,6 +111,23 @@ def self.create(config:, endpoints:, log:)
                   status 200
                   content_type "application/json"
                   response.to_json
+                rescue Hooks::Plugins::Handlers::Error => e
+                  # Handler called error! method - immediately return error response and exit the request
+                  log.debug("handler #{handler_class_name} called `error!` method")
+
+                  error_response = nil
+
+                  status e.status
+                  case e.body
+                  when String
+                    content_type "text/plain"
+                    error_response = e.body
+                  else
+                    content_type "application/json"
+                    error_response = e.body.to_json
+                  end
+
+                  return error_response
                 rescue StandardError => e
                   err_msg = "Error processing webhook event with handler: #{handler_class_name} - #{e.message} " \
                     "- request_id: #{request_id} - path: #{full_path} - method: #{http_method} - " \

lib/hooks/plugins/handlers/base.rb

@@ -2,6 +2,7 @@
 
 require_relative "../../core/global_components"
 require_relative "../../core/component_access"
+require_relative "error"
 
 module Hooks
   module Plugins
@@ -23,6 +24,28 @@ class Base
         def call(payload:, headers:, env:, config:)
           raise NotImplementedError, "Handler must implement #call method"
         end
+
+        # Terminate request processing with a custom error response
+        #
+        # This method provides the same interface as Grape's `error!` method,
+        # allowing handlers to immediately stop processing and return a specific
+        # error response to the client.
+        #
+        # @param body [Object] The error body/data to return to the client
+        # @param status [Integer] The HTTP status code to return (default: 500)
+        # @raise [Hooks::Plugins::Handlers::Error] Always raises to terminate processing
+        #
+        # @example Return a custom error with status 400
+        #   error!({ error: "validation_failed", message: "Invalid payload" }, 400)
+        #
+        # @example Return a simple string error with status 401
+        #   error!("Unauthorized", 401)
+        #
+        # @example Return an error with default 500 status
+        #   error!({ error: "internal_error", message: "Something went wrong" })
+        def error!(body, status = 500)
+          raise Error.new(body, status)
+        end
       end
     end
   end

lib/hooks/plugins/handlers/error.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+module Hooks
+  module Plugins
+    module Handlers
+      # Custom exception class for handler errors
+      #
+      # This exception is used when handlers call the `error!` method to
+      # immediately terminate request processing and return a specific error response.
+      # It carries the error details back to the Grape API context where it can be
+      # properly formatted and returned to the client.
+      #
+      # @example Usage in handler
+      #   error!({ error: "validation_failed", message: "Invalid payload" }, 400)
+      #
+      # @see Hooks::Plugins::Handlers::Base#error!
+      class Error < StandardError
+        # @return [Object] The error body/data to return to the client
+        attr_reader :body
+
+        # @return [Integer] The HTTP status code to return
+        attr_reader :status
+
+        # Initialize a new handler error
+        #
+        # @param body [Object] The error body/data to return to the client
+        # @param status [Integer] The HTTP status code to return (default: 500)
+        def initialize(body, status = 500)
+          @body = body
+          @status = status.to_i
+          super("Handler error: #{status} - #{body}")
+        end
+      end
+    end
+  end
+end

lib/hooks/version.rb

@@ -4,5 +4,5 @@
 module Hooks
   # Current version of the Hooks webhook framework
   # @return [String] The version string following semantic versioning
-  VERSION = "0.1.0".freeze
+  VERSION = "0.2.0".freeze
 end

spec/acceptance/acceptance_tests.rb

@@ -496,26 +496,28 @@ def expired_unix_timestamp(seconds_ago = 600)
       end
 
       it "sends a POST request to the /webhooks/boomtown_with_error endpoint and it explodes" do
-        # TODO: Fix this acceptance test - the current error looks like this:
-        # 1) Hooks endpoints boomtown_with_error sends a POST request to the /webhooks/boomtown_with_error endpoint and it explodes
-          # Failure/Error: expect(response.body).to include(expected_body_content) if expected_body_content
-            #expected "{\"error\":\"server_error\",\"message\":\"undefined method 'error!' for an instance of BoomtownWithE...thread_pool.rb:167:in 'block in #Puma::ThreadPool#spawn_thread'\",\"handler\":\"BoomtownWithError\"}" to include "the payload triggered a boomtown error"
-          # ./spec/acceptance/acceptance_tests.rb:28:in 'RSpec::ExampleGroups::Hooks#expect_response'
-          # ./spec/acceptance/acceptance_tests.rb:501:in 'block (4 levels) in <top (required)>'
-
-        # payload = { boom: true }.to_json
-        # response = make_request(:post, "/webhooks/boomtown_with_error", payload, json_headers)
-        # expect_response(response, Net::HTTPInternalServerError, "the payload triggered a boomtown error")
-
-        # body = parse_json_response(response)
-        # expect(body["error"]).to eq("server_error")
-        # expect(body["message"]).to eq("the payload triggered a boomtown error")
-        # expect(body).to have_key("backtrace")
-        # expect(body["backtrace"]).to be_a(String)
-        # expect(body).to have_key("request_id")
-        # expect(body["request_id"]).to be_a(String)
-        # expect(body).to have_key("handler")
-        # expect(body["handler"]).to eq("BoomtownWithError")
+        payload = { boom: true }.to_json
+        response = make_request(:post, "/webhooks/boomtown_with_error", payload, json_headers)
+        expect_response(response, Net::HTTPInternalServerError, "the payload triggered a boomtown error")
+
+        body = parse_json_response(response)
+        expect(body["error"]).to eq("boomtown_with_error")
+        expect(body["message"]).to eq("the payload triggered a boomtown error")
+        expect(body).to have_key("request_id")
+        expect(body["request_id"]).to be_a(String)
+        expect(body["foo"]).to eq("bar")
+        expect(body["truthy"]).to eq(true)
+      end
+
+      it "sends a POST request to the /webhooks/boomtown_with_error endpoint and it explodes with a simple text error" do
+        payload = { boom_simple_text: true }.to_json
+        response = make_request(:post, "/webhooks/boomtown_with_error", payload, json_headers)
+        expect_response(response, Net::HTTPInternalServerError, "boomtown_with_error: the payload triggered a simple text boomtown error")
+
+        body = response.body
+        expect(body).to eq("boomtown_with_error: the payload triggered a simple text boomtown error")
+        expect(response.content_type).to eq("text/plain")
+        expect(response.code).to eq("500")
       end
     end
   end

spec/acceptance/plugins/handlers/boomtown_with_error.rb

@@ -6,14 +6,25 @@ def call(payload:, headers:, env:, config:)
     if payload["boom"] == true
       log.error("boomtown error triggered by payload: #{payload.inspect} - request_id: #{env["hooks.request_id"]}")
 
-      # TODO: Get Grape's `error!` method to work with this
+      # Use Grape's `error!` method to return a custom error response
       error!({
         error: "boomtown_with_error",
         message: "the payload triggered a boomtown error",
+        foo: "bar",
+        truthy: true,
+        payload:,
+        headers:,
         request_id: env["hooks.request_id"]
       }, 500)
     end
 
+    if payload["boom_simple_text"] == true
+      log.error("boomtown simple text error triggered by payload: #{payload.inspect} - request_id: #{env["hooks.request_id"]}")
+
+      # Use Grape's `error!` method to return a simple text error response
+      error!("boomtown_with_error: the payload triggered a simple text boomtown error", 500)
+    end
+
     return { status: "ok" }
   end
 end

spec/unit/lib/hooks/handlers/base_spec.rb

@@ -250,4 +250,42 @@ def report(error_or_message, context = {})
       end
     end
   end
+
+  describe "#error!" do
+    let(:handler) { described_class.new }
+
+    it "raises a handler error with default status 500" do
+      expect {
+        handler.error!("Something went wrong")
+      }.to raise_error(Hooks::Plugins::Handlers::Error) do |error|
+        expect(error.body).to eq("Something went wrong")
+        expect(error.status).to eq(500)
+      end
+    end
+
+    it "raises a handler error with custom status" do
+      expect {
+        handler.error!({ error: "validation_failed", message: "Invalid input" }, 400)
+      }.to raise_error(Hooks::Plugins::Handlers::Error) do |error|
+        expect(error.body).to eq({ error: "validation_failed", message: "Invalid input" })
+        expect(error.status).to eq(400)
+      end
+    end
+
+    it "can be called from subclasses" do
+      test_handler = Class.new(described_class) do
+        def call(payload:, headers:, env:, config:)
+          error!("Custom error from subclass", 422)
+        end
+      end
+
+      handler = test_handler.new
+      expect {
+        handler.call(payload: {}, headers: {}, env: {}, config: {})
+      }.to raise_error(Hooks::Plugins::Handlers::Error) do |error|
+        expect(error.body).to eq("Custom error from subclass")
+        expect(error.status).to eq(422)
+      end
+    end
+  end
 end

spec/unit/lib/hooks/handlers/error_spec.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+describe Hooks::Plugins::Handlers::Error do
+  describe "#initialize" do
+    it "creates an error with default status 500" do
+      error = described_class.new("test error")
+      expect(error.body).to eq("test error")
+      expect(error.status).to eq(500)
+      expect(error.message).to eq("Handler error: 500 - test error")
+    end
+
+    it "creates an error with custom status" do
+      error = described_class.new({ error: "validation_failed" }, 400)
+      expect(error.body).to eq({ error: "validation_failed" })
+      expect(error.status).to eq(400)
+      expect(error.message).to match(/^Handler error: 400 - \{.*error.*validation_failed.*\}$/)
+    end
+
+    it "converts status to integer" do
+      error = described_class.new("test", "404")
+      expect(error.status).to eq(404)
+    end
+  end
+
+  describe "inheritance" do
+    it "inherits from StandardError" do
+      expect(described_class.ancestors).to include(StandardError)
+    end
+  end
+end