joshbuddy / http_router

A kick-ass HTTP router for use in Rack
MIT License
199 stars 45 forks source link

default_error being reset #35

Open matisojka opened 10 years ago

matisojka commented 10 years ago

I have been trying to use a default 404 handling in my app, so I look in the source code / documentation and found that it is possible to pass a valid Rack-style object that responds to #call:

class HttpRouter
  # (...) omitted on purpose

  # Creates a new HttpRouter.
  # Can be called with either <tt>HttpRouter.new(proc{|env| ... }, { .. options .. })</tt> or with the first argument omitted.
  # If there is a proc first, then it's used as the default app in the case of a non-match.
  # Supported options are
  # * :default_app -- Default application used if there is a non-match on #call. Defaults to 404 generator.
  # * :ignore_trailing_slash -- Ignore a trailing / when attempting to match. Defaults to +true+.
  # * :redirect_trailing_slash -- On trailing /, redirect to the same path without the /. Defaults to +false+.
  def initialize(*args, &blk)
    default_app, options     = args.first.is_a?(Hash) ? [nil, args.first] : [args.first, args[1]]
    @options                 = options
    @default_app             = default_app || options && options[:default_app] || proc{|env| ::Rack::Response.new("Not Found", 404, {'X-Cascade' => 'pass'}).finish }
    @ignore_trailing_slash   = options && options.key?(:ignore_trailing_slash) ? options[:ignore_trailing_slash] : true
    @redirect_trailing_slash = options && options.key?(:redirect_trailing_slash) ? options[:redirect_trailing_slash] : false
    @route_class             = Route
    reset!
    instance_eval(&blk) if blk
  end

So I tried instantiating the router like this:

    @route_resolver = HttpRouter.new(proc { |env|
      error_404_handler.call(env)
    })

In this case, error_404_handler should be responding with a custom message but it wasn't.

So looking further in the source code, I discovered a rather strange thing: when instantiating the HttpRouter class, the first argument can be a callable object, and this object is going to be the default_app, called when there is no matching route. But as we can see in the #initialize method, after defining the default_app, the method #reset! is getting called, which basically resets some stuff and the default_app.

I fixed it putting the #reset! call right at the beginning of #initialize:

def initialize(*args, &blk)
    reset!
    default_app, options     = args.first.is_a?(Hash) ? [nil, args.first] : [args.first, args[1]]
    @options                 = options
    @default_app             = default_app || options && options[:default_app] || proc{|env| ::Rack::Response.new("Not Found", 404, {'X-Cascade' => 'pass'}).finish }
    @ignore_trailing_slash   = options && options.key?(:ignore_trailing_slash) ? options[:ignore_trailing_slash] : true
    @redirect_trailing_slash = options && options.key?(:redirect_trailing_slash) ? options[:redirect_trailing_slash] : false
    @route_class             = Route
    instance_eval(&blk) if blk
  end

As I don't know enough about the code to know if this is going to break other stuff, I'd like to get some feedback on it first, before proposing a PR. Thanks!