Fix Allow header typo and Mail::Message.set guard bug#2318
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two correctness issues discovered during triage: (1) the HTTP 405 response header should be Allow (not Allows), and (2) Mail::Message.set should correctly guard method definition based on the provided setting name.
Changes:
- Fix 405 responses to emit the RFC-compliant
Allowheader. - Fix
Mail::Message.setto avoid incorrectly blocking subsequentset()calls after:erbis defined. - Add/extend tests to cover both fixes (405
Allowheader assertion, andMail::Message.setbehavior).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| padrino-mailer/test/test_message.rb | Adds tests for Mail::Message.set defining/guarding setting accessors. |
| padrino-mailer/lib/padrino-mailer/ext.rb | Fixes the set guard to check method_defined?(name) rather than :erb. |
| padrino-core/test/test_routing.rb | Adds assertion for Allow header on 405. |
| padrino-core/lib/padrino-core/application/routing.rb | Fixes header name from Allows to Allow on 405 responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Tested locally — full suite passes across all sub-gems (1,443 runs, 0 failures, 0 errors). Both fixes verified manually: Allow header: confirmed POST, PUT (unique, correct header name) on 405 responses One minor issue: the test ensure blocks clean up from Class but the methods are now on Mail::Message.singleton_class, so the cleanup is a no-op. Copilot's latest suggestion is correct — should be Mail::Message.singleton_class.method_defined? / Mail::Message.singleton_class.send(:remove_method, ...). Not blocking since the leaked settings are harmless, but worth a quick fix before merge. Otherwise LGTM. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ensure | ||
| Class.send(:remove_method, :_test_custom_setting) if Class.method_defined?(:_test_custom_setting) |
There was a problem hiding this comment.
The ensure cleanup is removing methods from Class, but Mail::Message.set defines a singleton method on Mail::Message (its singleton_class). This ensure currently won’t clean up the method and could also accidentally remove a real method from Class if one ever exists with that name. Please remove the method from Mail::Message.singleton_class (and check Mail::Message.singleton_class.method_defined?).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed — the ensure block now checks and removes from Mail::Message.singleton_class instead of Class. Commit: the latest change to test_message.rb.
There was a problem hiding this comment.
Fixed in commit 32dd813. Both ensure blocks now check and remove methods from Mail::Message.singleton_class instead of Class, matching where Mail::Message.set actually defines the methods.
| ensure | ||
| Class.send(:remove_method, :_test_another_setting) if Class.method_defined?(:_test_another_setting) | ||
| end |
There was a problem hiding this comment.
Same cleanup issue here: _test_another_setting is defined as a singleton method on Mail::Message, but the ensure block removes from Class. This will leave the test method defined across the suite and can cause cross-test pollution; remove it from Mail::Message.singleton_class instead.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed — same correction applied here: cleanup now targets Mail::Message.singleton_class.send(:remove_method, ...) with the appropriate guard check.
Fixed in commit |
The ensure blocks were cleaning up from Class instead of Mail::Message.singleton_class, making them no-ops since set() defines methods on the singleton class.
|
Pushed one small fix — the test ensure blocks were cleaning up from Class but set() now defines methods on Mail::Message.singleton_class, so the cleanup was a no-op. Updated to target Mail::Message.singleton_class so the methods are actually removed after each test. LGTM — merging as soon as CI is all green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Allowheader typo in 405 response (wasAllows).uniqtoAllowheader verbs to avoid duplicatesMail::Message.setguard to checkmethod_defined?(name)instead of:erbMail::Message.setto define methods onsingleton_classinstead ofClassMail::Message.setdefining accessor methodsMail::Message.setnot redefining an already defined methodensureblocks to check/remove methods fromMail::Message.singleton_class(notClass)💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.