redirect route destination not wrapped in `Routing::Endpoint`

[kaikuchn]

Hey folks,

@padget stumbled on a bug in hanami-router. After some digging around I found out what's broken. So here's the issue report.

Suppose you have the following routes definition:

redirect '/redirect-me', to: '/to-here'
get '/to-here', to: ->(env) { [200, {}, ['Hello from Redirect-Target']] }

When you visit /redirect-me in the browser you will get redirected. And everything seems to work. However, when you want to write a test case like this:

RSpec.describe Web.routes do
  it 'recognizes "GET /redirect-me"' do
    env   = Rack::MockRequest.env_for('/redirect-me')
    route = described_class.recognize(env)

    expect(route).to be_routable
  end

You will receive this error:

NoMethodError: undefined method `routable?' for #<Proc:0x007f89c7227628>

The reason is that the destination of the recognized route is not wrapped in a Hanami::Routing::Endpoint for the Hanami::Router::redirect call. It seems that the redirect keyword was not considered when the Hanami::Routing::Endpoint classes where introduced. I was unable to find a good way to fix this with minimal changes.

For all the other methods (get, post, root, etc.) the flow is that the corresponding http verb method is called on Hanami::HttpRouter which returns a Hanami::Routing::Route by going through Hanami::Routing::Route#generate which gets a Hanami::Routing::EndpointResolver passed whose job it is - among other things - to wrap the Hanami::Routing::Route @dest in a Hanami::Routing::Endpoint depending on the endpoint type.

Redirect, however, calls ::HttpRouter::Route#redirect on the result of Hanami::HttpRouter#get and that method overrides the @dest instance variable of the Hanami::Routing::Route. The Hanami::Routing::EndpointResolver is not visible in either Hanami::Routing::Route or Hanami::Router, it is only known to the Hanami::Routing::HttpRouter who is no longer involved.

I hope I could explain this in a manner that is easy to follow. It was quite difficult gathering all the moving pieces to locate the issue.

[kaikuchn]

Here's the monkey patch that fixes this in the mean time, for anyone who absolutely needs this to work..

class Hanami::Router
  def redirect(path, options = {}, &endpoint)
    get(path).redirect(@router.find(options), options[:code] || 301).tap do |route|
      route.dest = Hanami::Routing::Endpoint.new(route.dest)
    end
  end
end

It is save to just use the Endpoint class since redirect will always result in a callable for the route's destination.

[jodosha]

@kaikuchn Hi and sorry for the late reply. Can you please review this fix? #149

[kaikuchn]

@jodosha No worries! I'll be able to take a look at it over the weekend.