UrlGenerationError are not catched as 404 anymore #16229

Merged
merged 1 commit into from Oct 27, 2014

Projects

None yet

6 participants

@byroot
Ruby on Rails member

Fixes: #14185

Since UrlGenerationError inherits from RoutingError, it is handled as a 404 by default.

In my opinion it's not a good idea. Failing to generate an URL should be a 500 not a 404, and it should not be silently swallowed but reported to whatever error handler is present.

@rafaelfranca
Ruby on Rails member

@pixeltrix WDYT?

@matthewd
Ruby on Rails member

👍

RoutingError is translated to 404 because it means "error while routing a request".

While UrlGenerationError is an error in the routing code, by the above definition, it's not a RoutingError.

See also #14567 -- another use of RoutingError that probably shouldn't be.

@rafaelfranca
Ruby on Rails member

Yes. I believe this change is fine too. Just want to make sure I'm not missing anything.

@arthurnn
Ruby on Rails member

Can you also add a regression test for this?

@byroot
Ruby on Rails member

@arthurnn test added. I'm not 100% sure it's the best place for that test though.

@pixeltrix
Ruby on Rails member

👍 to the idea of not returning 404 for url generation errors.

The namespacing of the routing errors under ActionController is somewhat of a historical legacy. I wonder whether these errors would be better under ActionDispatch and deprecated in 4.2 - wdyt?

@rafaelfranca
Ruby on Rails member

@pixeltrix 👍

@pixeltrix
Ruby on Rails member

@byroot can you update your pull request to move the errors out of Action Controller and into Action Dispatch and then add aliases which are deprecated? Thanks!

@byroot
Ruby on Rails member

Sure. I might not find the time before tomorrow though, but I guess there is no emergency :)

@byroot
Ruby on Rails member

@pixeltrix I moved UrlGenerationError and RoutingError. I think it was pretty clear they belonged to ActionDispatch since they were referenced almost exclusively there.

Did you had other exceptions in mind?

@byroot
Ruby on Rails member

Any news on this? Should I rebase?

@robin850 robin850 and 1 other commented on an outdated diff Oct 26, 2014
guides/source/upgrading_ruby_on_rails.md
@@ -680,7 +680,7 @@ instead rely on a data attribute (e.g. `data: { confirm: 'Are you sure?' }`).
This deprecation also concerns the helpers based on this one (such as `link_to_if`
or `link_to_unless`).
-* Rails 4.0 changed how `assert_generates`, `assert_recognizes`, and `assert_routing` work. Now all these assertions raise `Assertion` instead of `ActionController::RoutingError`.
+* Rails 4.0 changed how `assert_generates`, `assert_recognizes`, and `assert_routing` work. Now all these assertions raise `Assertion` instead of `ActionDispatch::RoutingError`.
@robin850
robin850 Oct 26, 2014

This should certainly not be updated as this is under the upgrading from 3.2 to 4.0 section ; the error was still ActionController::Routing.

However, this could be updated here in the rescue_responses code snippet. Thanks for your patch so far ! 👍

@byroot
byroot Oct 26, 2014

I fixed that and rebased, thanks!

@robin850 robin850 added this to the 4.2.0 milestone Oct 26, 2014
@robin850 robin850 and 1 other commented on an outdated diff Oct 26, 2014
actionpack/lib/action_controller/metal/exceptions.rb
@@ -1,4 +1,13 @@
module ActionController
+
+ def self.const_missing(const_name)
+ return super unless const_name == :RoutingError || const_name == :UrlGenerationError
@robin850
robin850 Oct 26, 2014

Just a nit-picky styling concern but what do you think about:

return super unless [:RoutingError, :UrlGenerationError].include?(const_name)
@byroot
byroot Oct 26, 2014

I'm not a huge fan on allocating an array on const_missing.

@robin850
robin850 Oct 26, 2014

Ugh, yes, fair point plus it will be allocated very often, you're right, let's keep it as is ! 👍

@robin850
Ruby on Rails member

There's still just a remaining deprecated constant here ; sorry I missed it. Also what do you think about adding a test to make sure that these constants are deprecated ? Thanks for the quick update ! :-)

@byroot
Ruby on Rails member

I fixed the remaining deprecated reference.

As for testing the deprecation, how would you do that? By expecting ActiveSupport::Deprecation.warn to be called?

@robin850
Ruby on Rails member

As for testing the deprecation, how would you do that?

Actually there's an assert_deprecated helper to handle that. :-)

@byroot
Ruby on Rails member

Test added.

@matthewd
Ruby on Rails member

There's actually existing magic for deprecating constants: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/fixtures.rb#L775

However, this also seems to be relocating a rather arbitrary subset of the exceptions: MethodNotAllowed / NotImplemented, for example, seem no less worthy of being moved.

Given our current timing, I'm thinking maybe we grab just the first commit here (the change of parent class) for 4.2, then we'll take a more comprehensive accounting of where all these exceptions ought to live for 5.0. @pixeltrix?

@byroot
Ruby on Rails member

maybe we grab just the first commit here

I agree, this PR was initially just to bring a more sane behaviour, it kinda drifted to renaming constants. TBH even though I think these constants should be moved, I don't think it's as important than the first commit.

@pixeltrix
Ruby on Rails member

@matthewd @byroot agreed - can you amend the PR, thanks!

@byroot
Ruby on Rails member

Done! I have the commits in another branch, I'll submit them separately once this is merged.

@pixeltrix
Ruby on Rails member

@byroot the railties tests are failing - can you have a look?

@byroot
Ruby on Rails member

@pixeltrix this should go green now. The issue was that in that environment there is a catch all routes, so the UrlGenerationError couldn't be generated properly.

Now I directly raise one, it's not as nice but that should do it.

@pixeltrix pixeltrix merged commit df0ea90 into rails:master Oct 27, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@pixeltrix
Ruby on Rails member

@byroot thanks for your effort and patience! 😄

@cupakromer cupakromer added a commit to rspec/rspec-rails that referenced this pull request Nov 16, 2014
@cupakromer cupakromer Fix failing builds on Rails 4.2.0-beta
Rails updated the way route generation errors are raised. Specifically,
this applies only to internal server code which generates a route and
not routes passed in from a request. This change comes from
rails/rails#16229. In a nutshell, the change is
`ActionController::UrlGenerationError` no longer inherits from
`ActionController::RoutingError`.

This updates the cukes which expect the unknown routes used in the
internal rails controller code to raise the correct base error
`UrlGenerationError`.

The current routing matchers do not require updating as they do not rely
on the internal route generation code. Instead they are utilizing the
routing engine's path for parsing routes passed from requests. This
path still raises a `ActionController::RoutingError`.
1cef609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment