Add a Missing Keys Method to Formatter

[schneems]

When a named route cannot be resolved due to a missing key, the router should be capable of identifying the required keys for the named route. This will be extremely useful in debugging routing errors by providing enough information to fix the problem in the message.

Once this information is available via Journey I plan to submit a PR to Rails to utilize the functionality and enhance the named route error message similar to this previous pull request: https://github.com/rails/rails/pull/6130

Functionality is tested, ATP. Let me know if you have any comments or questions.

[pixeltrix]

Why not add the the missing keys information to the RoutingError exception? That way we don't have to run match_route twice. I'd also like to see a different exception used in Action Dispatch for url generation as I've seen multiple bug reports where people are getting confused between recognition and generation.

[bogdan]

Routing error means that no one route match parameters you specified. In case of named route there is probably only one route that matches. If url_for is used a group of routes could match and miss different parameters.

Non-named routing is a huge problem. It spawns a lot of compexity to impelement new features.

You should probably send a pull that make use of this patch in actionpack along with this PR. I am not sure this is gonna work.

[schneems]

Removed whitespace, reverted the test helper change, and refactored the added test.

[pixeltrix]

@bogdan if the block passed to match_route in generate returns the last matched route when generation fails this could be added to RouteError. Whilst I accept that for non-named routes it may be a bit misleading it's certainly better than what we've got.

[schneems]

@pixeltrix On putting the keys in the error message: We could. The error in question gets raised via the raise_routing_error method here: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/route_set.rb#L546 it can be called if no path is present or when rescuing from a Journey error. At the time it seemed more reasonable that Rails would request missing keys from journey than to rely on journey raising an error with a given message. If you think that is called for, I'm up for implementation changes.

For the error name: What would you like to see the error class get changed to? Perhaps something along the lines of ActionController::RouteGenerationError or ActionController::NamedRouteError ? If it helps I can open up a PR in Rails as @bogdan suggested.

[pixeltrix]

@schneems I think adding it to the exception makes more sense and will be faster than running through the routes again (not that performance is a concern here).

I think the exception should be called ActionController::UrlGenerationError. This will allow us to map that to a 500 response rather than the 404 response you get now when a route fails to generate, which is generally dropped by exception trackers like Airbrake, etc.

[pixeltrix]

@schneems yes, opening a pull request on Rails will help

[schneems]

Updated to pass the keys in the exception message. Rails PR opened using the Exception message method https://github.com/rails/rails/pull/7230

[schneems]

Looks like this feature has the green light from the rails/rails side of things. Let me know if there is anything I can do to help get this functionality added to Journey.

[schneems]

Ping. I know you are all busy, if you get a second could you let me know what else I can do to get this, or similar functionality merged in to Journey? Ideally would like to get this in by September. Functionality has :+1: from the rails/rails side of things: https://github.com/rails/rails/pull/7230. If you want to increase bandwidth to chat on this i'm richard.schneeman @ gmail on IM.