search

bullet-train-co / bullet_train

The Open Source Ruby on Rails SaaS Template
MIT License
1.67k stars 267 forks source link

Super scaffolded routing fails for pretty standard nested, namespaces resources. #1655

Closed andrewculver closed 1 week ago

andrewculver commented 3 months ago

When super scaffolding the following resources:

bin/rails generate super_scaffold Task Team name:text_field
bin/rails generate super_scaffold Project Team name:text_field
bin/rails generate super_scaffold Projects::Milestone Project,Team name:text_field
bin/rails generate super_scaffold Projects::Milestones::IncludedTask Projects::Milestone,Project,Team task_id:super_select{class_name=Task} details:text_field --sortable

We produce the following error when trying to show a Projects::Milestone at http://localhost:3000/account/projects/milestones/:id:

undefined method `new_account_projects_milestone_included_task_path' for an instance of #<Class:0x00000001394b2f00>

This is because the routes weren't properly generated for this combination of namespacing and nesting.

Super scaffolding currently produces:

resources :tasks

resources :projects do
  scope module: 'projects' do
    resources :milestones, only: collection_actions do
      # ⚠️ This is in the wrong place!
      scope module: 'milestones' do
        resources :included_tasks, only: collection_actions, concerns: [:sortable]
      end
    end

    namespace :milestones do
      resources :included_tasks, except: collection_actions, concerns: [:sortable]
    end
  end
end

namespace :projects do
  resources :milestones, except: collection_actions
end

But it should have produced:

resources :tasks

resources :projects do
  scope module: 'projects' do
    resources :milestones, only: collection_actions

    namespace :milestones do
      resources :included_tasks, except: collection_actions, concerns: [:sortable]
    end
  end
end

namespace :projects do
  resources :milestones, except: collection_actions do 
    # ✅ This is the right place to put this!
    scope module: 'milestones' do
      resources :included_tasks, only: collection_actions, concerns: [:sortable]
    end
  end
end
andrewculver commented 3 months ago

Looking at https://github.com/bullet-train-co/bullet_train/blob/main/test/system/super_scaffolding/super_scaffolding_test.rb, it looks like we don't have a test for this scenario so it makes sense that it's either always been broken or has been broken by refactors.

jagthedrummer commented 3 weeks ago

Just making notes as I look into this.

With this incorrect placement mentioned above we get routes like this:

$ rails routes | grep included | grep -v avo | sort
                                                                                                DELETE   /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#destroy
                                                                                                PATCH    /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#update
                                                                                                POST     /account/milestones/:milestone_id/included_tasks(.:format)                                                                                            account/projects/milestones/included_tasks#create
                                                                                                PUT      /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#update
                                                               account_milestone_included_tasks GET      /account/milestones/:milestone_id/included_tasks(.:format)                                                                                            account/projects/milestones/included_tasks#index
                                                               account_milestones_included_task GET      /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#show
                                                            new_account_milestone_included_task GET      /account/milestones/:milestone_id/included_tasks/new(.:format)                                                                                        account/projects/milestones/included_tasks#new
                                                          edit_account_milestones_included_task GET      /account/milestones/included_tasks/:id/edit(.:format)                                                                                                 account/projects/milestones/included_tasks#edit
                                                       reorder_account_milestone_included_tasks POST     /account/milestones/:milestone_id/included_tasks/reorder(.:format)                                                                                    account/projects/milestones/included_tasks#reorder
                                              reorder_account_project_milestones_included_tasks POST     /account/projects/:project_id/milestones/included_tasks/reorder(.:format)                                                                             account/projects/milestones/included_tasks#reorder

With the correct placement we get:

rails routes | grep included | grep -v avo | sort
                                                                                                DELETE   /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#destroy
                                                                                                PATCH    /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#update
                                                                                                POST     /account/projects/milestones/:milestone_id/included_tasks(.:format)                                                                                   account/projects/milestones/included_tasks#create
                                                                                                PUT      /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#update
                                                               account_milestones_included_task GET      /account/milestones/included_tasks/:id(.:format)                                                                                                      account/projects/milestones/included_tasks#show
                                                          edit_account_milestones_included_task GET      /account/milestones/included_tasks/:id/edit(.:format)                                                                                                 account/projects/milestones/included_tasks#edit
                                                      account_projects_milestone_included_tasks GET      /account/projects/milestones/:milestone_id/included_tasks(.:format)                                                                                   account/projects/milestones/included_tasks#index
                                                   new_account_projects_milestone_included_task GET      /account/projects/milestones/:milestone_id/included_tasks/new(.:format)                                                                               account/projects/milestones/included_tasks#new
                                              reorder_account_project_milestones_included_tasks POST     /account/projects/:project_id/milestones/included_tasks/reorder(.:format)                                                                             account/projects/milestones/included_tasks#reorder
                                              reorder_account_projects_milestone_included_tasks POST     /account/projects/milestones/:milestone_id/included_tasks/reorder(.:format)                                                                           account/projects/milestones/included_tasks#reorder
jagthedrummer commented 3 weeks ago

@andrewculver I could use a little more context on why some of these routes get generated like they do.

I've read these two articles, but am still not grocking it all. https://blog.bullettrain.co/rails-model-namespacing/ https://blog.bullettrain.co/nested-namespaced-rails-routing-examples/

Please note that I'm not trying to argue that we should do things differently, I'm just trying to understand why we do things the way we do. I know you've spent a lot of time on this problem space, so I assume there are good reasons. I just need to understand them to be effective in making changes around this stuff.

To try to simplify and get at my basic questions I'm using just these two scaffolding commands that you mentioned above:

bin/rails generate super_scaffold Project Team name:text_field
bin/rails generate super_scaffold Projects::Milestone Project,Team name:text_field

Those commands leave us with these declared routes for milestones:

resources :projects do
  scope module: 'projects' do
    resources :milestones, only: collection_actions
  end
end

namespace :projects do
  resources :milestones, except: collection_actions
end

Which results in these URL paths:

$ rails routes | grep milestones | grep -v avo | grep -v api | sort
                                DELETE   /account/projects/milestones/:id(.:format)              account/projects/milestones#destroy
                                PATCH    /account/projects/milestones/:id(.:format)              account/projects/milestones#update
                                POST     /account/projects/:project_id/milestones(.:format)      account/projects/milestones#create
                                PUT      /account/projects/milestones/:id(.:format)              account/projects/milestones#update
     account_project_milestones GET      /account/projects/:project_id/milestones(.:format)      account/projects/milestones#index
     account_projects_milestone GET      /account/projects/milestones/:id(.:format)              account/projects/milestones#show
  new_account_project_milestone GET      /account/projects/:project_id/milestones/new(.:format)  account/projects/milestones#new
edit_account_projects_milestone GET      /account/projects/milestones/:id/edit(.:format)         account/projects/milestones#edit

Some of these make sense to me, and are exactly what I'd expect.

                                POST     /account/projects/:project_id/milestones(.:format)      account/projects/milestones#create
     account_project_milestones GET      /account/projects/:project_id/milestones(.:format)      account/projects/milestones#index
  new_account_project_milestone GET      /account/projects/:project_id/milestones/new(.:format)  account/projects/milestones#new

But then there are these, and I don't understand why we'd want a bare /projects/ segment in the URL that doesn't come with a matching :project_id.

                                DELETE   /account/projects/milestones/:id(.:format)              account/projects/milestones#destroy
                                PATCH    /account/projects/milestones/:id(.:format)              account/projects/milestones#update
                                PUT      /account/projects/milestones/:id(.:format)              account/projects/milestones#update
     account_projects_milestone GET      /account/projects/milestones/:id(.:format)              account/projects/milestones#show
edit_account_projects_milestone GET      /account/projects/milestones/:id/edit(.:format)         account/projects/milestones#edit

Why would we not want the paths to be more like this? (Isn't that the recommendation/convention for shallow routing?)

                                DELETE   /account/milestones/:id(.:format)              account/projects/milestones#destroy
                                PATCH    /account/milestones/:id(.:format)              account/projects/milestones#update
                                PUT      /account/milestones/:id(.:format)              account/projects/milestones#update
     account_projects_milestone GET      /account/milestones/:id(.:format)              account/projects/milestones#show
edit_account_projects_milestone GET      /account/milestones/:id/edit(.:format)         account/projects/milestones#edit
jagthedrummer commented 3 weeks ago

@andrewculver - Ok, after struggling with this for a while I think something finally clicked and I figured out the answer to my question.

I think the deal is that for URL paths like /account/projects/milestones/:id the /projects/ segment doesn't have anything to do with the Project model it's a reference to the namespace on the Projects::Milestone model & controller. And so from a "shallow routes" perspective that is a shallow route because it only references one type of model. It just happens to be a model with a namespace that has the same URL segment (aka /projects/) that the parent model has. Which made it confusing.

Just to illustrate by using a different namespace. Say that instead of Projects::Milestones you did Foo::Milestones like this:

bin/rails generate super_scaffold Project Team name:text_field
bin/rails generate super_scaffold Foo::Milestone Project,Team name:text_field

Then the path to a Foo::Milestone would be /account/foo/milestones/:id.

I think the problem is going to be in this file somewhere, but I haven't quite figured out where.

https://github.com/bullet-train-co/bullet_train-core/blob/main/bullet_train-super_scaffolding/lib/scaffolding/routes_file_manipulator.rb

I think generally what's happening is that we just look for the first instance of a block for the parent namespace and then cram everything into it. I think we'll probably have to do something like looking for the first instance and the last instance and then cram the appropriate bits into each one. (And if they happen to be the same block then it'll get both.)