-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 message to NotFound exception #1566
Conversation
…s useful for exception reporting tools.
Looks like a great improvement to me 👍🏻 |
I believe the documentation also needs an update after this improvement like
|
See github.com/sinatra/sinatra/pull/1566
Update ruby-sinatra to 2.0.8.1. ## 2.0.8.1 / 2020-01-02 * Allow multiple hashes to be passed in `merge` and `merge!` for `Sinatra::IndifferentHash` [#1572](sinatra/sinatra#1572) by Shota Iguchi ## 2.0.8 / 2020-01-01 * Lookup Tilt class for template engine without loading files [#1558](sinatra/sinatra#1558). Fixes [#1172](sinatra/sinatra#1172) by Jordan Owens * Add request info in NotFound exception [#1566](sinatra/sinatra#1566) by Stefan Sundin * Add `.yaml` support in `Sinatra::Contrib::ConfigFile` [#1564](sinatra/sinatra#1564). Fixes [#1563](sinatra/sinatra#1563) by Emerson Manabu Araki * Remove only routing parameters from @params hash [#1569](sinatra/sinatra#1569). Fixes [#1567](sinatra/sinatra#1567) by Jordan Owens, Horacio * Support `capture` and `content_for` with Hamlit [#1580](sinatra/sinatra#1580) by Takashi Kokubun * Eliminate warnings of keyword parameter for Ruby 2.7.0 [#1581](sinatra/sinatra#1581) by Osamtimizer ## 2.0.7 / 2019-08-22 * Fix a regression [#1560](sinatra/sinatra#1560) by Kunpei Sakai ## 2.0.6 / 2019-08-21 * Fix an issue setting environment from command line option [#1547](sinatra/sinatra#1547), [#1554](sinatra/sinatra#1554) by Jordan Owens, Kunpei Sakai * Support pandoc as a new markdown renderer [#1533](sinatra/sinatra#1533) by Vasiliy * Remove outdated code for tilt 1.x [#1532](sinatra/sinatra#1532) by Vasiliy * Remove an extra logic for `force_encoding` [#1527](sinatra/sinatra#1527) by Jordan Owens * Avoid multiple errors even if `params` contains special values [#1526](sinatra/sinatra#1527) by Kunpei Sakai * Support `bundler/inline` with `require 'sinatra'` integration [#1520](sinatra/sinatra#1520) by Kunpei Sakai * Avoid `TypeError` when params contain a key without a value on Ruby < 2.4 [#1516](sinatra/sinatra#1516) by Samuel Giddins * Improve development support and documentation and source code by Olle Jonsson, Basavanagowda Kanur, Yuki MINAMIYA
I wonder who really thought this is a great change. Instead of providing the fixed size message body which corresponds to the error so the user can immediately see what was wrong you have turned this into an echo engine so people can ddos not only your incoming bandwidth but also the outcoming one. Great. 🤦 And while the OP could have simply used the Sorry for being negative, but this really didn't please me at all. |
Seriously, guys, can we talk about getting this reverted? @zzak? @namusyaka? There are so many things wrong with this. To name a few:
not_found do
content_type :text
"#{env['REQUEST_METHOD']} #{env['REQUEST_URI']}"
end So, where do we move from here? I am willing to prepare a pull request which reverts this and fixes the specs, too. But I would like to know first that it has a chance of being accepted. And sorry for not objecting earlier. We seldom upgrade our gems when there is no need and sinatra was mature and stable enough as it was... I found this only when starting on a new app and using the latest versions of everything. |
I've felt this pain too, would be onboard with a change (maybe optional?), but I think rack does the same? https://github.com/rack/rack/blob/a7747ec32e9971649257e838b0b49b9588945107/lib/rack/urlmap.rb#L79 |
Good point, but IMO the context is entirely different. I mean, rack does report that only when it doesn't find the app responsible for some path mapping, which in practice should happen only while you are setting up and testing the stack, but never in production. Whereas sinatra does that now for every path which is not found, unconditionally, even if everything is set up as it should be. Edit: You may compare it to the not found and error handlers while in development. I am grateful that sinatra does that in dev mode, as it helps seeing the request and everything. But echoing the path back in production is wrong, IMO. |
@raxoft I think you should open a PR changing this, I support it |
@raxoft is a PR incoming? |
Definitely. Sorry for the wait. I know it should be just few lines in the end, but I have been very busy with other things recently, and this week is not any better. I'll do my best to get back to this ASAP. Thanks. |
Finally got around to creating the PR. Sorry for the long wait and thanks for looking into this. |
Hi. I am currently monkey-patching this in my own project, but I think it would be useful for everyone, so I'm submitting this PR.
Add request method and path to the NotFound exception message. This is useful for exception reporting tools.
As an example, this is what Airbrake looks before this change. I can still get the URL but I have to click in to each report. It is very hard to get a nice overview.
And this is what it looks like after:
Let me know what you think!