Routes with suffixes conflict with each other

Question

Routes with suffixes conflict with each other

francois opened this issue 5 months ago · 8 comments

Given these two routes, the order in which the routes are declared in config/routes.rb impacts how the router behaves:

get '/attendance/:season_slug/events', to: 'attendances.index', as: :select_event_for_attendance
get '/attendance/:event_slug', to: 'attendance.show', as: :attendances

Notice both routes have the same prefix. One route adds "/events" as a suffix.

When the routes are written as above, given the URL http://localhost:2300/attendance/qgox, I get a backtrace:

Hanami::Router::NotAllowedError at /attendance/qgox
Only DELETE, POST requests are allowed at /attendance/qgox

block in <module:ClassMethods>
hanami (2.2.0.beta1) lib/hanami/slice.rb, line 1072
Hanami::Slice::Router#not_allowed
hanami-router (2.2.0.beta1) lib/hanami/router.rb, line 640
Hanami::Slice::Router#call
hanami-router (2.2.0.beta1) lib/hanami/router.rb, line 106
Hanami::Middleware::Assets#call
rack (2.2.9) lib/rack/static.rb, line 161
Rack::Session::Cookie#context
rack (2.2.9) lib/rack/session/abstract/id.rb, line 266
Rack::Session::Cookie#call
rack (2.2.9) lib/rack/session/abstract/id.rb, line 260
Rack::MethodOverride#call
rack (2.2.9) lib/rack/method_override.rb, line 24
BetterErrors::Middleware#protected_app_call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 87
BetterErrors::Middleware#better_errors_call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 82
BetterErrors::Middleware#call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 60

If I flip the order of the routes around, where /events is last, then routing occurs and I can render the page I was trying to visit. On the flip-side, trying to visit http://localhost:2300/attendance/abgf/events results in a different error:

Hanami::Router::NotFoundError at /attendance/abgf/events
No route found for GET /attendance/abgf/events

block in <module:ClassMethods>
hanami (2.2.0.beta1) lib/hanami/slice.rb, line 1077
Hanami::Slice::Router#not_found
hanami-router (2.2.0.beta1) lib/hanami/router.rb, line 646
Hanami::Slice::Router#call
hanami-router (2.2.0.beta1) lib/hanami/router.rb, line 106
Hanami::Middleware::Assets#call
rack (2.2.9) lib/rack/static.rb, line 161
Rack::Session::Cookie#context
rack (2.2.9) lib/rack/session/abstract/id.rb, line 266
Rack::Session::Cookie#call
rack (2.2.9) lib/rack/session/abstract/id.rb, line 260
Rack::MethodOverride#call
rack (2.2.9) lib/rack/method_override.rb, line 24
BetterErrors::Middleware#protected_app_call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 87
BetterErrors::Middleware#better_errors_call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 82
BetterErrors::Middleware#call
better_errors (2.10.1) lib/better_errors/middleware.rb, line 60
Hanami::Webconsole::Middleware#call
hanami-webconsole (2.2.0.beta1) lib/hanami/webconsole/middleware.rb, line 56

Full config/routes.rb:

# frozen_string_literal: true

module Scoutatt
  class Routes < Hanami::Routes
    get '/seasons', to: 'seasons.index', as: :seasons
    get '/seasons/new', to: 'seasons.new', as: :new_season
    get '/seasons/:slug', to: 'seasons.show', as: :show_season
    post '/seasons', to: 'seasons.create', as: :create_season
    get '/seasons/:slug/edit', to: 'seasons.edit', as: :edit_season
    patch '/seasons/:slug', to: 'seasons.update', as: :update_season
    delete '/seasons/:slug', to: 'seasons.destroy', as: :destroy_season

    get '/seasons/:slug/events', to: 'events.index', as: :events
    post '/seasons/:slug/events', to: 'events.create', as: :create_event

    post '/registrations/:slug', to: 'registrations.create', as: :create_registrations

    post '/events', to: 'events.create'
    patch '/events/:slug', to: 'events.update', as: :update_event
    delete '/events/:slug', to: 'events.destroy', as: :destroy_event

    get '/attendance/:event_slug', to: 'attendance.show', as: :attendances
    get '/attendance/:season_slug/events', to: 'attendances.index', as: :select_event_for_attendance
    post '/attendance/registration/:registration_slug',
         to: 'attendance.create_from_registration',
         as: :create_attendance_from_registration
    post '/attendance/:event_slug',
         to: 'attendance.create', as: :create_attendance
    delete '/attendance/:registration_slug',
           to: 'attendance.destroy', as: :destroy_attendance
  end
end

Environment

$ bundle show | ag hanami
  * hanami (2.2.0.beta1)
  * hanami-assets (2.2.0.beta1)
  * hanami-cli (2.2.0.beta1)
  * hanami-controller (2.2.0.beta1)
  * hanami-db (2.2.0.beta1)
  * hanami-reloader (2.2.0.beta1)
  * hanami-router (2.2.0.beta1)
  * hanami-rspec (2.2.0.beta1)
  * hanami-utils (2.2.0.beta1)
  * hanami-validations (2.2.0.beta1)
  * hanami-view (2.2.0.beta1)
  * hanami-webconsole (2.2.0.beta1)

$ ruby --version
ruby 3.2.4 (2024-04-23 revision af471c0e01) [arm64-darwin23]

Answer 1 · 2024-08-08T14:25:52.000Z

Of course, now I'm trying to reproduce in a new Hanami application and I am failing...

config/routes.rb

# frozen_string_literal: true

module Bookshelf
  class Routes < Hanami::Routes
    # Add your routes here. See https://guides.hanamirb.org/routing/overview/ for details.
    get '/foo/:slug', to: 'foo.a'
    get '/foo/:slug/b', to: 'foo.b'
    post "/foo/:slug", to: "foo.c"
  end
end

I can visit both /foo/a and /foo/a/b and the page renders correctly in both cases. Flipping the order of foo.a and foo.b does not change the behaviour.

What mistake did I make in my app? Could Hanami have prevented this mistake, or warned me?

Answer 2 · 2024-08-11T01:43:05.000Z

Thanks for raising this, @francois, and for helping us dig into it. Looking at your full routes file vs your focused reproduction, I wonder what might happen if you make it match exactly, e.g. if you add the second post line as well as the delete? Maybe we might see the issue reoccur at that point?

Answer 3 · 2024-08-21T02:54:34.000Z

I have the same experience.

❯ be hanami routes
GET     /                             /experts (HTTP 301)           
GET     /up                           health.show                   as :health_check    
GET     /experts                      experts.index                 as :experts         
GET     /experts/:id/edit             experts.edit                  as :edit_expert     
GET     /experts/:uuid                experts.show                  as :expert          
PATCH   /experts/:id                  experts.update                as :update_expert   
GET     /unmatched                    unmatched.index               as :unmatcheds      
GET     /unmatched/:id                unmatched.show                as :unmatched

I started with experts.index and experts.show. When I added experts.edit, I had to put it before experts.show for it to work, but I didn't realize that experts.show then stopped working. At this point, Hanami can't find that route at all:

Hanami::Router::NotFoundError at /experts/79500c2a-aac9-4674-adfe-f8413786b8d7
==============================================================================

No route found for GET /experts/79500c2a-aac9-4674-adfe-f8413786b8d7

When I then added the experts.update route, the error changed to the same Only POST requests are allowed message that @francois received.

I am still on Hanami 2.1

❯ bundle show | grep "hanami"
  * hanami (2.1.1)
  * hanami-cli (2.1.1)
  * hanami-controller (2.1.0)
  * hanami-reloader (2.1.0)
  * hanami-router (2.1.0)
  * hanami-rspec (2.1.0)
  * hanami-utils (2.1.0)
  * hanami-validations (2.1.0)
  * hanami-view (2.1.0)
  * hanami-webconsole (2.1.0)

The main symptom that I see in common with @francois, and what I think is causing the problem, is the use of different path variables (:id and :uuid for me) in the routes. And, indeed, if I change experts.edit and experts.update to use :uuid as their path variable (even though this breaks the action code for both), then experts.show works again.

I can try to take a look at hanami-router. Any tips to guide me?

Answer 4 · 2024-08-21T03:00:57.000Z

This appears to be the same as #255.

Answer 5 · 2024-08-22T00:20:32.000Z

Yep, so it seems we have an honest-to-goodness bug here. Thank you for the reports & the corroboration!

Is anyone here (or anyone reading this) willing to have a go at a fix? Luca was our router expert, but he's stepped away from Ruby OSS now, so this would be a great chance for someone to come in and establish some vital knowledge :)

I'd be happy to cut a release immediately once this fix is done.

(I will also promise to get onto this myself, but it will have to follow the remainder of my work on preparing for the Hanami 2.2 release, so it'll just need a bit of time)

Answer 6 · 2024-08-22T21:55:10.000Z

I am willing to take a look. No idea what I'm getting into, so no promises . . . 😶

Answer 7 · 2024-08-23T02:44:16.000Z

@dcr8898 @francois @timriley (sorry creeping on this)

I think i found the issue and @dcr8898 hit the nail on the head: its the fact that its multiple different slugs (e.g. :id, :uuid). i've been staring at it for a minute so pardon my attempt at retracing my steps

my understanding of what happens is that on app init, the router builds a trie data structure off of all the routes (see Hanami::Router::Trie). this is fine makes total sense. the first foible is that its treating the two slugs as different, so they each get a node in the trie. important to note: these nodes are differentiated between variable and fixed types (see #initialize / #put on the trie class. so really, two different, independent tries if we're being obnoxious). these two 'tries' are hashes where the key is a Mustermann pattern, value is the actual router node.

then, when a route is called, we go into "matching land", and we start traversing the trie to find a matching node, matching by Mustermann pattern, for both fixed / variable tries (all this happens in Hanami::Router::Trie#get). this is foible number 2: mustermann (at least in 'rails' mode which we're using), will match anything to a pattern starting with :. so, the first match in this hash gets its node returned (another side note: / counts as a node. so /experts/:id, the trailing url slash counts and thats how it matches urls ending in a dynamic part). this is why the order of routes is important in the examples above. the router only ever gets to the first 'match' on dynamic slug. i agree w/ tim that this is a potentially a pretty significant issue, especially with domain driven design.

from my cursory research, i dont think mustermann has any work around for this. and even so, we'd probably end up down a path of trying to turn dynamic slugs into non-dynamic and that aint it. i dont have a ton of ideas on where to go here, but one thing we could do is translate all dynamic slugs to be the same exact text (e.g., turn all :uuid into :id, if :uuid is the first dynamic part encountered). by doing this i think all route nodes will be appended to the single dynamic route part in the appropriate trie, and hopefully be matched during the matching process. i havent tested this, and even if it did work, not sure the downstream implications.

open to suggestions, but wanted to share what i think the issue is to hopefully help us narrow in on a solution. lmk what you guys think thanks!

Answer 8 · 2024-08-31T20:09:39.000Z

Working on this with @kyleplump . PR shortly . . .