-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dsl method supported_http_methods
#3106
Conversation
I've converted this to a draft, as it needs changes.
Thoughts? EDIT: Updated per the above, Re fifth point, it would require passing a value into Client, not sure if the API changes are worth it? |
3a3b4da
to
008c3b6
Compare
additional_http_methods
supported_http_methods
After #3107, looked over open PR's tagged 'feature', and noticed #3041, which is very similar. Not sure what to do, but in the meantime I'll add @francois-ferrandis as a co-author, and add some more comments. |
008c3b6
to
79a5938
Compare
I've added more code comments, and added text to the upgrade doc Welcome to Puma 6: Sunflower. These additions are at the bottom of the doc. I'm not sure we should be basing our default ( This can lead to problems if EDIT:* Removed 'the' from 6.0-Upgrade.md... |
It's OK for me to close my PR and let you take it from here. 🤝 I will be glad to be credited, thank you very much! 🙏 |
79a5938
to
5474d78
Compare
I want some more time to think about this one so I'll leave it out from 6.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This seems like a good compromise if we are not going back to the Puma 5 behaviour of accepting any HTTP method (see #3014 (comment) and #3014 (comment)). We should probably assume some users do like the new behaviour introduced with Puma 6... |
73feb2c
to
0af6002
Compare
So, I guess this is your favorite new feature? Sorry...
I think one feature Puma that has always had is something similar to 'just start it and go'. That's fine for many apps, but high volume/capacity sites need a fair amount of fine-grained control/configuration. Obviously, stopping invalid request methods in Puma helps. So, allowing configuration of exactly what http methods are allowed seemed like the only way to handle it. Having options like the below just weren't flexible enough...
|
Good day!
Any thoughts? Assuming you agree with "high volume/capacity sites need a fair amount of fine-grained control/configuration", one can certainly screw things up, but that requires changing the default configuration... |
@MSP-Greg Hehe, it is not my favorite feature. I'm actually questioning why we did this to being with. I think we should go so far to (provide the option) to restore the Puma 5 behavior of accepting arbitrary value for the HTTP method. I don't like that Puma will log a row because it thinks the HTTP method isn't supported. Anyone can send any value to my Puma app. With Puma 5 that was just a 404 (not logged unless I have request logging on): #3014 (comment) |
Maybe we should change the logging, so it only is done so with 'request logging on'? |
Maybe use LogWriter#debug`, or just remove the log liine
So how does it respond to a "PUMA-IS-GREAT" method? I'm thinking of adding a const |
@nateberkopec wdyt about this that I suggested a few weeks ago
|
…ORTED_HTTP_METHODS Co-authored-by: francois-ferrandis <[email protected]>
…http_methods Co-authored-by: Patrik Ragnarsson <[email protected]>
@dentarg I updated this (forgot to rebase), below are four examples in the DSL# docs, I think the last one is what you'd like... # Adds 'PROPFIND' to existing supported methods
supported_http_methods(Puma::Const::SUPPORTED_HTTP_METHODS + ['PROPFIND'])
# Restricts methods to the array elements
supported_http_methods %w[HEAD GET POST PUT DELETE OPTIONS PROPFIND]
# Restricts methods to the methods in the IANA Registry
supported_http_methods Puma::Const::IANA_HTTP_METHODS
# Allows any method
supported_http_methods :any |
That sounds great (haven't looked at the code here). Only comment is that just seeing |
Good idea. I'll update... |
That would be the default you're proposing then, right? |
@dentarg Code is now using
Not really. I added the constant We could certainly change the default, not sure whether that means releasing Puma 7? |
Description
Adds
Puma::DSL#supported_http_methods
, which allows overriding defaults inPuma::Const::SUPPORTED_HTTP_METHODS
.Turbo-Rails CI is currently broken.
Closes #3014
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.