fix access priorities of each level in LeveledOptions (#1118)

[MitchLewis930]

PR_056

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Corrected the order of configuration option resolution to properly prioritize recently set options.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request reverses the lookup order in Puma's LeveledOptions#[] method to prioritize more recently added configuration options, and adds a test to verify configuration options remain mutable after loading.

Changes

Cohort / File(s)	Summary
Configuration Option Precedence lib/puma/configuration.rb, test/test_config.rb	Modified LeveledOptions#[] to iterate in reverse over the @set collection, giving newer options higher precedence during key resolution. Added test_overwrite_options test to verify that configuration options can be mutated after loading.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A hop, skip, and reverse we go,
New options now steal the show!
Fresh configs take the lead,
Mutability's the rabbit's creed,
Priorities shuffled, just as we need! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is nearly empty with only 'PR_056' provided, missing all required template sections including motivation, problem statement, and checklist items.	Fill out the complete PR description template including the motivation for the change, related issues, alternatives considered, and the contributor checklist.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: reversing lookup order in LeveledOptions to fix access priorities.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr_056_after

---

🧹 Recent nitpick comments

test/test_config.rb (2)

66-68: assert_equal arguments are in wrong order.

The conventional argument order for assert_equal is (expected, actual). The current code has them reversed.

🔧 Proposed fix

-    assert_equal conf.options[:workers], 3
+    assert_equal 3, conf.options[:workers]
     conf.options[:workers] += 1
-    assert_equal conf.options[:workers], 4
+    assert_equal 4, conf.options[:workers]

---

60-69: Consider adding a test that directly validates priority ordering.

This test verifies option mutability post-load, but it doesn't directly test the priority fix (reverse lookup order). A more targeted test would verify that later configuration levels override earlier ones.

For example, a test could set a value via constructor options, then override it in a DSL block or config file, and verify the later value takes precedence.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8aac2f and 8509829.

📒 Files selected for processing (2)

• lib/puma/configuration.rb
• test/test_config.rb

🧰 Additional context used

🧬 Code graph analysis (1)

test/test_config.rb (2)

lib/puma/dsl.rb (3)

• workers (286-288)
• load (7-14)
• load (118-120)

lib/puma/configuration.rb (1)

• load (190-208)

🔇 Additional comments (1)

lib/puma/configuration.rb (1)

33-46: The fix correctly prioritizes later configuration levels.

The change from @set.each to @set.reverse_each ensures that more recently added configuration levels take precedence, which is the expected behavior for layered configuration systems.

Note: The flatten method (lines 92-102) still iterates forward with ||= (giving precedence to earlier values), creating an inconsistency with the new [] behavior. However, the flatten method appears to be unused in the codebase, so this inconsistency has no practical impact. If flatten is meant to be a public API, either remove it as dead code or update it to match the new [] behavior for consistency.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

lib/puma/configuration.rb (2)

92-102: ⚠️ Potential issue | 🟠 Major

Inconsistency: flatten still prioritizes earlier layers.

The flatten method uses forward iteration with ||=, which gives priority to the first occurrence of a key. This is now inconsistent with [] which uses reverse_each to prioritize the last (most recent) occurrence.

For example, if @set = [{}, {workers: 3}, {workers: 4}]:

• self[:workers] returns 4 (most recent)
• flatten[:workers] returns 3 (first non-nil)

Proposed fix to align `flatten` with the new priority order

 def flatten
   options = {}

-  `@set.each` do |o|
+  `@set.reverse_each` do |o|
     o.each do |k,v|
       options[k] ||= v
     end
   end

   options
 end

---

76-84: ⚠️ Potential issue | 🔴 Critical

Typo: @default should be @defaults.

Line 83 references @default but the instance variable is defined as @defaults on line 20. This would raise a NoMethodError when a key is not found in any layer of @set.

Proposed fix

   def key?(key)
     `@set.each` do |o|
       if o.key? key
         return true
       end
     end

-    `@default.key`? key
+    `@defaults.key`? key
   end

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8aac2f and 8509829.

📒 Files selected for processing (2)

• lib/puma/configuration.rb
• test/test_config.rb

🧰 Additional context used

🧬 Code graph analysis (1)

test/test_config.rb (3)

lib/puma/dsl.rb (3)

• workers (286-288)
• load (7-14)
• load (118-120)

lib/puma/configuration.rb (1)

• load (190-208)

lib/puma/state_file.rb (1)

• load (13-15)

🔇 Additional comments (2)

lib/puma/configuration.rb (1)

33-38: LGTM! Reverse iteration correctly prioritizes recent layers.

The change to reverse_each ensures that the most recently added option layer takes precedence when looking up a key, which is the expected behavior for layered configuration overrides.

test/test_config.rb (1)

60-69: Test correctly validates the override behavior.

The test properly exercises the mutability of options after load, confirming that the reverse_each change ensures the most recently set value takes precedence.

Minor style note: The assertion argument order is expected, actual in Test::Unit (e.g., assert_equal 3, conf.options[:workers]), though the current order will still work.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}