joshbuddy / http_router

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

README claims that Rack::Request can be used interchangeably with rack env hash, but that does not seem to be true. #41

Open lasso opened 9 years ago

lasso commented 9 years ago

If I use rack's env hash directly everything is ok:

working_app = lambda do |env|
  router = HttpRouter.new
  router.add('/').to(:main)
  router.add('/foo').to(:foo)
  router.add('/foo/bar').to(:foobar)
  matching_routes = router.recognize(env)
  return Rack::Response.new('Not found', 400) if matching_routes.first.nil?
  Rack::Response.new(matching_routes.first.first.route.dest.inspect)
end

run working_app

If I go to '/' I get :main if I go to '/foo/' I get :foo If I go to '/foo/bar' I get :foobar

However, if I use a Rack::Request object instead of the env hash things does not work as expected:

non_working_app = lambda do |env|
  router = HttpRouter.new
  router.add('/').to(:main)
  router.add('/foo').to(:foo)
  router.add('/foo/bar').to(:foobar)
  # Wrapping env in a Rack::Request
  matching_routes = router.recognize(Rack::Request.new(env))
  return Rack::Response.new('Not found', 400) if matching_routes.first.nil?
  Rack::Response.new(matching_routes.first.first.route.dest.inspect)
end

run non_working_app

If I go to '/' I get :main if I go to '/foo/' I get :main If I go to '/foo/bar' I get :main

The README suggests that the env hash and Rack::Request objects can both be used by the recognize method, but that does not seem to be true. I don't know if it is a real bug or if the docs are simply wrong (or maybe I am doing something wrong), but I'd thought it would be a good thing to report this inconsistency anyway.

lholden commented 9 years ago

Hi there! Neither Josh or myself have really touched this project in a really long time... so it's always possible that things have changed since the README was written.

If you fix this on your end... feel free to make a pull request and I'll accept it.

lasso commented 9 years ago

Hi!

Sorry for the late reply. I haven't really touched my router code since I filed this issue. It was trivial for me to change the code to use the rack environment directly instead of a Rack::Request object.

I don't think there is much demand for supporting Rack::Request objects in addition to the rack env hash, so the simplest solution would probably be to update the README to say that a hash is expected. I'll do a MR for the docs shortly.