Ensure and enforce that configs are loaded before options are accesse…#8
Ensure and enforce that configs are loaded before options are accesse…#8MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Puma's configuration system to enforce that configurations are properly loaded and clamped before options are accessed. The change introduces a more explicit initialization flow where clamp must be called before accessing configuration options, preventing premature access to unfinalized configuration state.
Changes:
- Added error guards to prevent accessing options before
clampis called - Replaced
conf.loadcalls withconf.clampthroughout the test suite - Refactored the
Binderclass to accept options hash instead of Configuration object - Updated hook processing to store metadata for cluster-only hooks and defer warnings until clamp
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/puma/configuration.rb | Introduces NotLoadedError and NotClampedError exceptions; adds guards to options and config_files accessors; refactors clamp to call load internally and validate hooks |
| lib/puma/binder.rb | Changes constructor to accept options hash instead of Configuration object |
| lib/puma/launcher.rb | Updates initialization sequence to call clamp before accessing options; adjusts Binder instantiation |
| lib/puma/dsl.rb | Refactors hook processing to store cluster_only metadata and defer warnings to clamp time |
| lib/puma/server.rb | Updates Binder instantiation to pass options hash |
| lib/rack/handler/puma.rb | Changes log_writer keyword argument syntax from hash rocket to colon |
| test/test_config.rb | Updates all tests to call clamp before accessing options; adds tests for NotLoadedError and NotClampedError |
| test/test_cli.rb | Adds clamp calls before accessing configuration properties |
| test/test_binder.rb | Updates Binder instantiation in setup and tests to pass options hash |
| test/test_launcher.rb | Changes direct options manipulation to use configure block with DSL |
| test/test_rack_handler.rb | Replaces all conf.load calls with conf.clamp |
| test/test_web_concurrency_auto.rb | Wraps Configuration instantiation in assert_raises block and calls clamp |
| lib/puma/control_cli.rb | Adds clamp call before accessing configuration options |
| benchmarks/local/puma_info.rb | Replaces load with clamp for configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def process_hook(options_key, key, block, meth) | ||
| raise ArgumentError, "expected #{meth} to be given a block" unless block | ||
| def process_hook(options_key, key, block, method, cluster_only: false) |
There was a problem hiding this comment.
The parameter name 'method' is misleading as it stores the hook name string, not a method object. Consider renaming to 'hook_name' or 'method_name' for clarity.
|
|
||
| conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings) | ||
| assert_equal messages, ["#{hook_name} is called with ARG"] | ||
| assert_equal ["#{hook_name} is called with ARG"], messages |
There was a problem hiding this comment.
The assertion order has been reversed from the previous assert_equal messages, [...] pattern. For consistency with Ruby/Minitest conventions, consider keeping the expected value as the first argument: assert_equal [expected], actual.
|
|
||
| conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings) | ||
| assert_equal messages, ["#{hook_name} is called with ARG one time", "#{hook_name} is called with ARG a second time"] | ||
| assert_equal ["#{hook_name} is called with ARG one time", "#{hook_name} is called with ARG a second time"], messages |
There was a problem hiding this comment.
The assertion order has been reversed from the previous assert_equal messages, [...] pattern. For consistency with Ruby/Minitest conventions, consider keeping the expected value as the first argument.
2 participants
PR_058