hanami / router

Ruby/Rack HTTP router
http://hanamirb.org
MIT License
362 stars 92 forks source link

POST route recognised as GET #220

Closed romankovt closed 1 year ago

romankovt commented 2 years ago

Hello!

I'm following the README guide from the main branch (which points to v2.0.0.alpha6 now). The Testing section shows an example of how it does not recognize a POST route when the router has a route defined as GET.

require "hanami/router"

router = Hanami::Router.new do
  get "/books/:id", to: "books#show", as: :book
end

route = router.recognize("/books/23", method: :post)
route.verb      # "POST"
route.routable? # => false

But in fact, what I get is:

require "hanami/router"

router = Hanami::Router.new do
  get "/books/:id", to: "books#show", as: :book
end

route = router.recognize("/books/23", method: :post)
route.verb      # => "GET"
route.routable? # => "true"
romankovt commented 2 years ago

Upon further investigation, it seems like the other examples when you recognize non-String routes also don't work

require "hanami/router"

router = Hanami::Router.new do
  get "/books/:id", to: "books#show", as: :book
end

route = router.recognize(:book, id: 23, method: :POST)

route.verb      # => GET
route.routable? # => true

The cause of the problem is that #env_for takes 2 hashes as arguments, but when you use router.recognize(:book, id: 23, method: :post) it takes id and method as the first hash argument, but the intended behavior is for them to be 2 separate arguments.

It can be fixed by passing the first argument as empty hash:

require "hanami/router"

router = Hanami::Router.new do
  get "/books/:id", to: "books#show", as: :book
end

route = router.recognize(:book, { id: 23 }, { method: :post })

route.verb      # => POST
route.routable? # => false

route = router.recognize("/books/32", {}, { method: :post })
route.verb      # => POST
route.routable? # => false

So, the docs should be updated, or if it was not intended to be this way it is worth changing the #env_for behavior leaving it with only 2 arguments def env_for(env, params = {}) and parsing and separating all the possible options keys from the params hash here, but as a side effect one wouldn't be able to have reserved keywords as HTTP param names:)

jodosha commented 1 year ago

@romankovt Good catch! Thanks for reporting.