From 1a61b8dbd7d3c64267e30783e003a915214c2bf3 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 18 Jun 2025 12:40:57 -0700 Subject: [PATCH 1/5] Rename handler_dir to handler_plugin_dir in configuration and update related tests. All refs to the handler dir must be made through `handler_plugin_dir` going forward to be consistent --- docs/design.md | 8 ++++---- lib/hooks/core/config_loader.rb | 1 - lib/hooks/core/config_validator.rb | 1 - spec/unit/lib/hooks/core/config_validator_spec.rb | 6 +++--- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/docs/design.md b/docs/design.md index 8c4fcbe..3f32026 100644 --- a/docs/design.md +++ b/docs/design.md @@ -109,7 +109,7 @@ export HOOKS_ROOT_PATH="/webhooks" export HOOKS_LOG_LEVEL=info # Paths -export HOOKS_HANDLER_DIR=./handlers +export HOOKS_HANDLER_PLUGIN_DIR=./handlers export HOOKS_HEALTH_PATH=/health export HOOKS_VERSION_PATH=/version @@ -163,7 +163,7 @@ lib/hooks/ ```yaml # config/endpoints/team1.yaml path: /team1 # Mounted at /team1 -handler: Team1Handler # Class in handler_dir +handler: Team1Handler # Class in handler_plugin_dir # Signature validation auth: @@ -181,7 +181,7 @@ opts: # Freeform user-defined options ```yaml # config/config.yaml -handler_dir: ./handlers # handler class directory +handler_plugin_dir: ./handlers # handler class directory log_level: info # debug | info | warn | error # Request handling @@ -345,7 +345,7 @@ app = Hooks.build( **Handler & Plugin Discovery:** -* Handler classes are auto-discovered from `handler_dir` using file naming convention +* Handler classes are auto-discovered from `handler_plugin_dir` using file naming convention * File `team1_handler.rb` → class `Team1Handler` * Plugin classes are loaded from `plugin_dir` and registered based on class inheritance * All classes must inherit from appropriate base classes to be recognized diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index 3125b08..1fd2df3 100644 --- a/lib/hooks/core/config_loader.rb +++ b/lib/hooks/core/config_loader.rb @@ -127,7 +127,6 @@ def self.load_env_config env_config = {} env_mappings = { - "HOOKS_HANDLER_DIR" => :handler_dir, "HOOKS_HANDLER_PLUGIN_DIR" => :handler_plugin_dir, "HOOKS_AUTH_PLUGIN_DIR" => :auth_plugin_dir, "HOOKS_LIFECYCLE_PLUGIN_DIR" => :lifecycle_plugin_dir, diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb index 0e7807b..f3965a5 100644 --- a/lib/hooks/core/config_validator.rb +++ b/lib/hooks/core/config_validator.rb @@ -12,7 +12,6 @@ class ValidationError < StandardError; end # Global configuration schema GLOBAL_CONFIG_SCHEMA = Dry::Schema.Params do - optional(:handler_dir).filled(:string) # For backward compatibility optional(:handler_plugin_dir).filled(:string) optional(:auth_plugin_dir).maybe(:string) optional(:lifecycle_plugin_dir).maybe(:string) diff --git a/spec/unit/lib/hooks/core/config_validator_spec.rb b/spec/unit/lib/hooks/core/config_validator_spec.rb index 545961c..ed48453 100644 --- a/spec/unit/lib/hooks/core/config_validator_spec.rb +++ b/spec/unit/lib/hooks/core/config_validator_spec.rb @@ -5,7 +5,7 @@ context "with valid configuration" do it "returns validated configuration with all optional fields" do config = { - handler_dir: "./custom_handlers", + handler_plugin_dir: "./custom_handlers", log_level: "debug", request_limit: 2_048_000, request_timeout: 45, @@ -101,7 +101,7 @@ it "raises ValidationError for empty string values" do config = { - handler_dir: "", + handler_plugin_dir: "", root_path: "", health_path: "" } @@ -125,7 +125,7 @@ it "raises ValidationError for non-string paths" do config = { - handler_dir: 123, + handler_plugin_dir: 123, root_path: [], endpoints_dir: {} } From 978e345948bc69b45cfb28e36afd751c5582b50c Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 18 Jun 2025 12:48:19 -0700 Subject: [PATCH 2/5] make the `handler_plugin_dir` a required field --- lib/hooks/core/config_validator.rb | 2 +- .../lib/hooks/core/config_validator_spec.rb | 25 +++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb index f3965a5..c7b3ddc 100644 --- a/lib/hooks/core/config_validator.rb +++ b/lib/hooks/core/config_validator.rb @@ -12,7 +12,7 @@ class ValidationError < StandardError; end # Global configuration schema GLOBAL_CONFIG_SCHEMA = Dry::Schema.Params do - optional(:handler_plugin_dir).filled(:string) + required(:handler_plugin_dir).filled(:string) optional(:auth_plugin_dir).maybe(:string) optional(:lifecycle_plugin_dir).maybe(:string) optional(:instruments_plugin_dir).maybe(:string) diff --git a/spec/unit/lib/hooks/core/config_validator_spec.rb b/spec/unit/lib/hooks/core/config_validator_spec.rb index ed48453..73938d7 100644 --- a/spec/unit/lib/hooks/core/config_validator_spec.rb +++ b/spec/unit/lib/hooks/core/config_validator_spec.rb @@ -24,15 +24,18 @@ end it "returns validated configuration with minimal fields" do - config = {} + config = { handler_plugin_dir: "/path/to/handlers" } result = described_class.validate_global_config(config) - expect(result).to eq({}) + expect(result).to eq({ handler_plugin_dir: "/path/to/handlers" }) end it "accepts production environment" do - config = { environment: "production" } + config = { + environment: "production", + handler_plugin_dir: "/path/to/handlers" + } result = described_class.validate_global_config(config) @@ -41,7 +44,10 @@ it "accepts valid log levels" do %w[debug info warn error].each do |log_level| - config = { log_level: log_level } + config = { + log_level: log_level, + handler_plugin_dir: "/path/to/handlers" + } result = described_class.validate_global_config(config) @@ -114,7 +120,8 @@ it "coerces boolean-like string values" do config = { use_catchall_route: "true", - normalize_headers: "1" + normalize_headers: "1", + handler_plugin_dir: "/path/to/handlers" } result = described_class.validate_global_config(config) @@ -138,7 +145,8 @@ it "coerces string numeric values" do config = { request_limit: "1024", - request_timeout: "30" + request_timeout: "30", + handler_plugin_dir: "/path/to/handlers" } result = described_class.validate_global_config(config) @@ -164,7 +172,10 @@ end it "coerces float values to integers by truncating" do - config = { request_timeout: 30.5 } + config = { + request_timeout: 30.5, + handler_plugin_dir: "/path/to/handlers" + } result = described_class.validate_global_config(config) From 21f5b369871d40a77a8f80cab817eddd2c126776 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 18 Jun 2025 12:53:04 -0700 Subject: [PATCH 3/5] Make `log_level`, `root_path`, and `environment` required fields in global configuration schema --- lib/hooks/core/config_validator.rb | 6 ++-- .../lib/hooks/core/config_validator_spec.rb | 32 +++++++++++++++---- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb index c7b3ddc..7f53d7f 100644 --- a/lib/hooks/core/config_validator.rb +++ b/lib/hooks/core/config_validator.rb @@ -16,13 +16,13 @@ class ValidationError < StandardError; end optional(:auth_plugin_dir).maybe(:string) optional(:lifecycle_plugin_dir).maybe(:string) optional(:instruments_plugin_dir).maybe(:string) - optional(:log_level).filled(:string, included_in?: %w[debug info warn error]) + required(:log_level).filled(:string, included_in?: %w[debug info warn error]) optional(:request_limit).filled(:integer, gt?: 0) optional(:request_timeout).filled(:integer, gt?: 0) - optional(:root_path).filled(:string) + required(:root_path).filled(:string) optional(:health_path).filled(:string) optional(:version_path).filled(:string) - optional(:environment).filled(:string, included_in?: %w[development production]) + required(:environment).filled(:string, included_in?: %w[development production]) optional(:endpoints_dir).filled(:string) optional(:use_catchall_route).filled(:bool) optional(:normalize_headers).filled(:bool) diff --git a/spec/unit/lib/hooks/core/config_validator_spec.rb b/spec/unit/lib/hooks/core/config_validator_spec.rb index 73938d7..4e40f14 100644 --- a/spec/unit/lib/hooks/core/config_validator_spec.rb +++ b/spec/unit/lib/hooks/core/config_validator_spec.rb @@ -24,17 +24,24 @@ end it "returns validated configuration with minimal fields" do - config = { handler_plugin_dir: "/path/to/handlers" } + config = { + handler_plugin_dir: "/path/to/handlers", + log_level: "info", + root_path: "/app", + environment: "development" + } result = described_class.validate_global_config(config) - expect(result).to eq({ handler_plugin_dir: "/path/to/handlers" }) + expect(result).to eq(config) end it "accepts production environment" do config = { environment: "production", - handler_plugin_dir: "/path/to/handlers" + handler_plugin_dir: "/path/to/handlers", + log_level: "info", + root_path: "/app" } result = described_class.validate_global_config(config) @@ -46,7 +53,9 @@ %w[debug info warn error].each do |log_level| config = { log_level: log_level, - handler_plugin_dir: "/path/to/handlers" + handler_plugin_dir: "/path/to/handlers", + root_path: "/app", + environment: "development" } result = described_class.validate_global_config(config) @@ -121,7 +130,10 @@ config = { use_catchall_route: "true", normalize_headers: "1", - handler_plugin_dir: "/path/to/handlers" + handler_plugin_dir: "/path/to/handlers", + log_level: "info", + root_path: "/app", + environment: "development" } result = described_class.validate_global_config(config) @@ -146,7 +158,10 @@ config = { request_limit: "1024", request_timeout: "30", - handler_plugin_dir: "/path/to/handlers" + handler_plugin_dir: "/path/to/handlers", + log_level: "info", + root_path: "/app", + environment: "development" } result = described_class.validate_global_config(config) @@ -174,7 +189,10 @@ it "coerces float values to integers by truncating" do config = { request_timeout: 30.5, - handler_plugin_dir: "/path/to/handlers" + handler_plugin_dir: "/path/to/handlers", + log_level: "info", + root_path: "/app", + environment: "development" } result = described_class.validate_global_config(config) From 0d1b3a31edeebbeb4e156490d78c1a0b52a495b5 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 18 Jun 2025 12:53:20 -0700 Subject: [PATCH 4/5] bump --- Gemfile.lock | 2 +- lib/hooks/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 0de8f5f..3e6e53a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - hooks-ruby (0.4.0) + hooks-ruby (0.5.0) dry-schema (~> 1.14, >= 1.14.1) grape (~> 2.3) puma (~> 6.6) diff --git a/lib/hooks/version.rb b/lib/hooks/version.rb index 0420f04..19bff12 100644 --- a/lib/hooks/version.rb +++ b/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.4.0".freeze + VERSION = "0.5.0".freeze end From 6753d313adca6c753781e85d5641b838cfa91baa Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 18 Jun 2025 12:57:41 -0700 Subject: [PATCH 5/5] Add error handling for missing and invalid configuration files in ConfigLoader --- lib/hooks/core/config_loader.rb | 10 ++++++- .../unit/lib/hooks/core/config_loader_spec.rb | 28 ++++++++----------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index 1fd2df3..c04d8bd 100644 --- a/lib/hooks/core/config_loader.rb +++ b/lib/hooks/core/config_loader.rb @@ -32,16 +32,24 @@ class ConfigLoader # # @param config_path [String, Hash] Path to config file or config hash # @return [Hash] Merged configuration + # @raise [ArgumentError] if config file path is provided but file doesn't exist + # @raise [RuntimeError] if config file exists but fails to load def self.load(config_path: nil) config = DEFAULT_CONFIG.dup overrides = [] # Load from file if path provided - if config_path.is_a?(String) && File.exist?(config_path) + if config_path.is_a?(String) + unless File.exist?(config_path) + raise ArgumentError, "Configuration file not found: #{config_path}" + end + file_config = load_config_file(config_path) if file_config overrides << "file config" config.merge!(file_config) + else + raise RuntimeError, "Failed to load configuration from file: #{config_path}" end end diff --git a/spec/unit/lib/hooks/core/config_loader_spec.rb b/spec/unit/lib/hooks/core/config_loader_spec.rb index 95c2072..1f68d63 100644 --- a/spec/unit/lib/hooks/core/config_loader_spec.rb +++ b/spec/unit/lib/hooks/core/config_loader_spec.rb @@ -101,12 +101,10 @@ context "when file does not exist" do let(:config_file) { File.join(temp_dir, "nonexistent.yml") } - it "returns default configuration" do - config = described_class.load(config_path: config_file) - - expect(config[:log_level]).to eq("info") - expect(config[:environment]).to eq("production") - expect(config[:production]).to be true + it "raises ArgumentError" do + expect { + described_class.load(config_path: config_file) + }.to raise_error(ArgumentError, "Configuration file not found: #{config_file}") end end @@ -117,11 +115,10 @@ File.write(config_file, "invalid: yaml: content: [") end - it "returns default configuration" do - config = described_class.load(config_path: config_file) - - expect(config[:log_level]).to eq("info") - expect(config[:environment]).to eq("production") + it "raises RuntimeError" do + expect { + described_class.load(config_path: config_file) + }.to raise_error(RuntimeError, "Failed to load configuration from file: #{config_file}") end end @@ -132,11 +129,10 @@ File.write(config_file, "log_level: debug") end - it "returns default configuration" do - config = described_class.load(config_path: config_file) - - expect(config[:log_level]).to eq("info") - expect(config[:environment]).to eq("production") + it "raises RuntimeError" do + expect { + described_class.load(config_path: config_file) + }.to raise_error(RuntimeError, "Failed to load configuration from file: #{config_file}") end end end