From 6a616b926a72084398c0e31ff7bc6777ad23e641 Mon Sep 17 00:00:00 2001
From: Alex Burgel <aburgel@gmail.com>
Date: Mon, 5 Aug 2024 12:37:57 -0400
Subject: [PATCH] Annotate controllers in Rails engines and packs (#48)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

~~This is a draft. I still have to get the tests updated and write more
tests, but I wanted to get some feedback before doing so.~~

I approached this in a different way from the current implementation.

1. It starts similarly by gathering all the routes, to which I added
following routes of any mounted engines.
2. Then instead of iterating through the controllers provided by the
controller pattern, I iterated through the routes and then loaded the
corresponding controller class.
3. From there I got the source location of controller, and added the
annotations but only if the source location matched the controller
pattern.

It's possible with Zeitwerk (and probably other mechanisms) for the
controller name and path to not line up exactly, so with this approach
you don't need to make any assumptions.

Please let me know what you think and if it looks good, I can add and
update the tests.

Fixes #47

---------

Co-authored-by: Nishiki (錦華) <hello@nshki.com>
---
 .gitignore                                    |   2 +
 lib/chusaku.rb                                |  29 +++--
 lib/chusaku/cli.rb                            |   3 +-
 lib/chusaku/routes.rb                         |  18 ++-
 test/chusaku_test.rb                          | 107 ++++++++++--------
 test/cli_test.rb                              |   2 +-
 .../controllers/api/burritos_controller.rb    |   8 +-
 .../app/controllers/api/cakes_controller.rb   |  10 +-
 .../app/controllers/api/tacos_controller.rb   |  50 ++++----
 .../app/controllers/application_controller.rb |   2 +
 test/mock/engine.rb                           |  46 ++++++++
 .../engine/app/controllers/cars_controller.rb |   4 +
 .../app/controllers/engine_controller.rb      |   2 +
 test/mock/rails.rb                            |  55 +++------
 test/mock/route_helper.rb                     |  59 ++++++++++
 test/routes_test.rb                           |   5 +
 16 files changed, 267 insertions(+), 135 deletions(-)
 create mode 100644 test/mock/app/controllers/application_controller.rb
 create mode 100644 test/mock/engine.rb
 create mode 100644 test/mock/engine/app/controllers/cars_controller.rb
 create mode 100644 test/mock/engine/app/controllers/engine_controller.rb
 create mode 100644 test/mock/route_helper.rb

diff --git a/.gitignore b/.gitignore
index a083a57..539d408 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,5 +7,7 @@
 /spec/reports/
 /tmp/
 
+.DS_Store
+
 .ruby-version
 Gemfile.lock
diff --git a/lib/chusaku.rb b/lib/chusaku.rb
index 2055602..642f189 100644
--- a/lib/chusaku.rb
+++ b/lib/chusaku.rb
@@ -4,6 +4,8 @@
 
 # Handles core functionality of annotating projects.
 module Chusaku
+  DEFAULT_CONTROLLERS_PATTERN = "**/*_controller.rb".freeze
+
   class << self
     # The main method to run Chusaku. Annotate all actions in a Rails project as
     # follows:
@@ -19,14 +21,20 @@ def call(flags = {})
       @flags = flags
       @routes = Chusaku::Routes.call
       @changed_files = []
-      controllers_pattern = @flags[:controllers_pattern] || "app/controllers/**/*_controller.rb"
+      controllers_pattern = @flags[:controllers_pattern] || DEFAULT_CONTROLLERS_PATTERN
+      controllers_paths = Dir.glob(Rails.root.join(controllers_pattern))
+
+      @routes.each do |controller, actions|
+        next unless controller
+
+        controller_class = "#{controller.underscore.camelize}Controller".constantize
+        action_method_name = actions.keys.first&.to_sym
+        next unless !action_method_name.nil? && controller_class.method_defined?(action_method_name)
 
-      Dir.glob(Rails.root.join(controllers_pattern)).each do |path|
-        controller = %r{controllers/(.*)_controller\.rb}.match(path)[1]
-        actions = @routes[controller]
-        next if actions.nil?
+        source_path = controller_class.instance_method(action_method_name).source_location&.[](0)
+        next unless controllers_paths.include?(source_path)
 
-        annotate_file(path: path, controller: controller, actions: actions.keys)
+        annotate_file(path: source_path, actions: actions)
       end
 
       output_results
@@ -46,16 +54,15 @@ def load_tasks
     # Adds annotations to the given file.
     #
     # @param path [String] Path to file
-    # @param controller [String] Controller name
-    # @param actions [Array<String>] List of valid actions for the controller
+    # @param actions [Hash<String, Hash>] List of valid action data for the controller
     # @return [void]
-    def annotate_file(path:, controller:, actions:)
-      parsed_file = Chusaku::Parser.call(path: path, actions: actions)
+    def annotate_file(path:, actions:)
+      parsed_file = Chusaku::Parser.call(path: path, actions: actions.keys)
       parsed_file[:groups].each_cons(2) do |prev, curr|
         clean_group(prev)
         next unless curr[:type] == :action
 
-        route_data = @routes[controller][curr[:action]]
+        route_data = actions[curr[:action]]
         next unless route_data.any?
 
         annotate_group(group: curr, route_data: route_data)
diff --git a/lib/chusaku/cli.rb b/lib/chusaku/cli.rb
index a5967d1..ec6af8d 100644
--- a/lib/chusaku/cli.rb
+++ b/lib/chusaku/cli.rb
@@ -41,7 +41,8 @@ def call(args = ARGV)
     # @raise [Chusaku::CLI::NotARailsProject] Exception if not Rails project
     # @return [void]
     def check_for_rails_project
-      has_controllers = File.directory?("./app/controllers")
+      controllers_pattern = options[:controllers_pattern] || DEFAULT_CONTROLLERS_PATTERN
+      has_controllers = !Dir.glob(Rails.root.join(controllers_pattern)).empty?
       has_rakefile = File.exist?("./Rakefile")
       raise NotARailsProject unless has_controllers && has_rakefile
     end
diff --git a/lib/chusaku/routes.rb b/lib/chusaku/routes.rb
index 69dfa6b..585c714 100644
--- a/lib/chusaku/routes.rb
+++ b/lib/chusaku/routes.rb
@@ -27,7 +27,19 @@ class << self
       def call
         routes = {}
 
-        Rails.application.routes.routes.each do |route|
+        populate_routes(Rails.application, routes)
+        backfill_routes(routes)
+      end
+
+      private
+
+      def populate_routes(app, routes)
+        app.routes.routes.each do |route|
+          if route.app.engine?
+            populate_routes(route.app.app, routes)
+            next
+          end
+
           controller, action, defaults = extract_data_from(route)
           routes[controller] ||= {}
           routes[controller][action] ||= []
@@ -39,12 +51,8 @@ def call
             action: action,
             defaults: defaults
         end
-
-        backfill_routes(routes)
       end
 
-      private
-
       # Adds formatted route info for the given param combination.
       #
       # @param route [Hash] Route info
diff --git a/test/chusaku_test.rb b/test/chusaku_test.rb
index f703c7a..a6fb8d6 100644
--- a/test/chusaku_test.rb
+++ b/test/chusaku_test.rb
@@ -17,7 +17,7 @@ def test_exit_with_error_on_annotation_flag
     out, _err = capture_io { exit_code = Chusaku.call(error_on_annotation: true) }
 
     assert_equal(1, exit_code)
-    assert_equal(3, File.written_files.count)
+    assert_equal(4, File.written_files.count)
     assert_includes(out, "Exited with status code 1.")
   end
 
@@ -38,10 +38,10 @@ def test_verbose_flag
     out, _err = capture_io { exit_code = Chusaku.call(verbose: true) }
 
     assert_equal(0, exit_code)
-    refute_includes(out, "Annotated test/mock/app/controllers/api/burritos_controller.rb")
-    assert_includes(out, "Annotated test/mock/app/controllers/api/cakes_controller.rb")
-    assert_includes(out, "Annotated test/mock/app/controllers/api/tacos_controller.rb")
-    assert_includes(out, "Annotated test/mock/app/controllers/waterlilies_controller.rb")
+    refute_includes(out, "Annotated #{Rails.root}/app/controllers/api/burritos_controller.rb")
+    assert_includes(out, "Annotated #{Rails.root}/app/controllers/api/cakes_controller.rb")
+    assert_includes(out, "Annotated #{Rails.root}/app/controllers/api/tacos_controller.rb")
+    assert_includes(out, "Annotated #{Rails.root}/app/controllers/waterlilies_controller.rb")
   end
 
   def test_mock_app
@@ -49,60 +49,65 @@ def test_mock_app
 
     capture_io { exit_code = Chusaku.call }
     files = File.written_files
-    base_path = "test/mock/app/controllers"
+    app_path = "#{Rails.root}/app/controllers"
+    engine_path = "#{Rails.root}/engine/app/controllers"
 
     assert_equal(0, exit_code)
-    assert_equal(3, files.count)
-    refute_includes(files, "#{base_path}/api/burritos_controller.rb")
+    assert_equal(4, files.count)
+    refute_includes(files, "#{app_path}/api/burritos_controller.rb")
 
     expected =
       <<~HEREDOC
-        class CakesController < ApplicationController
-          # This route's GET action should be named the same as its PUT action,
-          # even though only the PUT action is named.
-          # @route GET /api/cakes/inherit (inherit)
-          # @route PUT /api/cakes/inherit (inherit)
-          def inherit
+        module Api
+          class CakesController < ApplicationController
+            # This route's GET action should be named the same as its PUT action,
+            # even though only the PUT action is named.
+            # @route GET /api/cakes/inherit (inherit)
+            # @route PUT /api/cakes/inherit (inherit)
+            def inherit
+            end
           end
         end
       HEREDOC
-    assert_equal(expected, files["#{base_path}/api/cakes_controller.rb"])
+    assert_equal(expected, files["#{app_path}/api/cakes_controller.rb"])
 
     expected =
       <<~HEREDOC
-        class TacosController < ApplicationController
-          # @route GET / (root)
-          # @route GET /api/tacos/:id (taco)
-          def show
-          end
-
-          # This is an example of generated annotations that come with Rails 6
-          # scaffolds. These should be replaced by Chusaku annotations.
-          # @route POST /api/tacos (tacos)
-          def create
-          end
-
-          # Update all the tacos!
-          # We should not see a duplicate @route in this comment block.
-          # But this should remain (@route), because it's just words.
-          # @route PUT /api/tacos/:id (taco)
-          # @route PATCH /api/tacos/:id (taco)
-          def update
-          end
-
-          # This route doesn't exist, so it should be deleted.
-          def destroy
-            puts("Tacos are indestructible")
-          end
-
-          private
-
-          def tacos_params
-            params.require(:tacos).permit(:carnitas)
+        module Api
+          class TacosController < ApplicationController
+            # @route GET / (root)
+            # @route GET /api/tacos/:id (taco)
+            def show
+            end
+
+            # This is an example of generated annotations that come with Rails 6
+            # scaffolds. These should be replaced by Chusaku annotations.
+            # @route POST /api/tacos (tacos)
+            def create
+            end
+
+            # Update all the tacos!
+            # We should not see a duplicate @route in this comment block.
+            # But this should remain (@route), because it's just words.
+            # @route PUT /api/tacos/:id (taco)
+            # @route PATCH /api/tacos/:id (taco)
+            def update
+            end
+
+            # This route doesn't exist, so it should be deleted.
+            def destroy
+              puts("Tacos are indestructible")
+            end
+
+            private
+
+            def tacos_params
+              params.require(:tacos).permit(:carnitas)
+            end
           end
         end
       HEREDOC
-    assert_equal(expected, files["#{base_path}/api/tacos_controller.rb"])
+    assert_equal(expected, files["#{app_path}/api/tacos_controller.rb"])
 
     expected =
       <<~HEREDOC
@@ -118,7 +123,17 @@ def one_off
           end
         end
       HEREDOC
-    assert_equal(expected, files["#{base_path}/waterlilies_controller.rb"])
+    assert_equal(expected, files["#{app_path}/waterlilies_controller.rb"])
+
+    expected =
+      <<~HEREDOC
+        class CarsController < EngineController
+          # @route POST /engine/cars (car)
+          def create
+          end
+        end
+      HEREDOC
+    assert_equal(expected, files["#{engine_path}/cars_controller.rb"])
   end
 
   def test_mock_app_with_no_pending_annotations
diff --git a/test/cli_test.rb b/test/cli_test.rb
index 83bd185..c4ecbd6 100644
--- a/test/cli_test.rb
+++ b/test/cli_test.rb
@@ -40,7 +40,7 @@ def test_exit_flag_precedence
 
   def test_project_detection
     _, err = capture_io do
-      assert_equal(1, Chusaku::CLI.new.call([]))
+      assert_equal(1, Chusaku::CLI.new.call(["-c", "**/*_not_controller.rb"]))
     end
     assert_equal(<<~ERR, err)
       Please run chusaku from the root of your project.
diff --git a/test/mock/app/controllers/api/burritos_controller.rb b/test/mock/app/controllers/api/burritos_controller.rb
index 2021fa1..9d28a7d 100644
--- a/test/mock/app/controllers/api/burritos_controller.rb
+++ b/test/mock/app/controllers/api/burritos_controller.rb
@@ -1,5 +1,7 @@
-class BurritosController < ApplicationController
-  # @route POST /api/burritos (burritos)
-  def create
+module Api
+  class BurritosController < ApplicationController
+    # @route POST /api/burritos (burritos)
+    def create
+    end
   end
 end
diff --git a/test/mock/app/controllers/api/cakes_controller.rb b/test/mock/app/controllers/api/cakes_controller.rb
index 8b781a8..c2cc3f1 100644
--- a/test/mock/app/controllers/api/cakes_controller.rb
+++ b/test/mock/app/controllers/api/cakes_controller.rb
@@ -1,6 +1,8 @@
-class CakesController < ApplicationController
-  # This route's GET action should be named the same as its PUT action,
-  # even though only the PUT action is named.
-  def inherit
+module Api
+  class CakesController < ApplicationController
+    # This route's GET action should be named the same as its PUT action,
+    # even though only the PUT action is named.
+    def inherit
+    end
   end
 end
diff --git a/test/mock/app/controllers/api/tacos_controller.rb b/test/mock/app/controllers/api/tacos_controller.rb
index ffc9830..57becee 100644
--- a/test/mock/app/controllers/api/tacos_controller.rb
+++ b/test/mock/app/controllers/api/tacos_controller.rb
@@ -1,31 +1,33 @@
-class TacosController < ApplicationController
-  def show
-  end
+module Api
+  class TacosController < ApplicationController
+    def show
+    end
 
-  # This is an example of generated annotations that come with Rails 6
-  # scaffolds. These should be replaced by Chusaku annotations.
-  # POST /api/tacos
-  # PATCH/PUT /api/tacos/1
-  def create
-  end
+    # This is an example of generated annotations that come with Rails 6
+    # scaffolds. These should be replaced by Chusaku annotations.
+    # POST /api/tacos
+    # PATCH/PUT /api/tacos/1
+    def create
+    end
 
-  # Update all the tacos!
-  # @route this should be deleted, it's not a valid route.
-  # We should not see a duplicate @route in this comment block.
-  # @route PUT /api/tacos/:id (taco)
-  # But this should remain (@route), because it's just words.
-  def update
-  end
+    # Update all the tacos!
+    # @route this should be deleted, it's not a valid route.
+    # We should not see a duplicate @route in this comment block.
+    # @route PUT /api/tacos/:id (taco)
+    # But this should remain (@route), because it's just words.
+    def update
+    end
 
-  # This route doesn't exist, so it should be deleted.
-  # @route DELETE /api/tacos/:id
-  def destroy
-    puts("Tacos are indestructible")
-  end
+    # This route doesn't exist, so it should be deleted.
+    # @route DELETE /api/tacos/:id
+    def destroy
+      puts("Tacos are indestructible")
+    end
 
-  private
+    private
 
-  def tacos_params
-    params.require(:tacos).permit(:carnitas)
+    def tacos_params
+      params.require(:tacos).permit(:carnitas)
+    end
   end
 end
diff --git a/test/mock/app/controllers/application_controller.rb b/test/mock/app/controllers/application_controller.rb
new file mode 100644
index 0000000..f6a886f
--- /dev/null
+++ b/test/mock/app/controllers/application_controller.rb
@@ -0,0 +1,2 @@
+class ApplicationController
+end
diff --git a/test/mock/engine.rb b/test/mock/engine.rb
new file mode 100644
index 0000000..c19126e
--- /dev/null
+++ b/test/mock/engine.rb
@@ -0,0 +1,46 @@
+# This file overrides Rails methods such that we can test without installing
+# multiple versions of Rails in the test suite. If different versions of Rails
+# begin treating route generation differently, new overrides should be written
+# for each version.
+#
+# The mocks used should reflect the files located in `test/mock/app/`.
+
+require "pathname"
+require_relative "route_helper"
+require_relative "engine/app/controllers/engine_controller"
+require_relative "engine/app/controllers/cars_controller"
+
+module Engine
+  extend RouteHelper
+
+  class << self
+    # Lets us call `Engine.app.routes.routes` without a skeleton Rails
+    # app.
+    #
+    # @return [Minitest::Mock] Mocked `Engine.app`
+    def app
+      routes = []
+
+      routes.push \
+        mock_route \
+          controller: "cars",
+          action: "create",
+          verb: "POST",
+          path: "/engine/cars(.:format)",
+          name: "car"
+
+      engine_app = Minitest::Mock.new
+      app_routes = Minitest::Mock.new
+      app_routes.expect(:routes, routes.compact)
+      engine_app.expect(:routes, app_routes)
+      engine_app
+    end
+
+    # Lets us call `Engine.engine?` without a skeleton Rails app.
+    #
+    # @return [Boolean]
+    def engine?
+      true
+    end
+  end
+end
diff --git a/test/mock/engine/app/controllers/cars_controller.rb b/test/mock/engine/app/controllers/cars_controller.rb
new file mode 100644
index 0000000..b7bcc3d
--- /dev/null
+++ b/test/mock/engine/app/controllers/cars_controller.rb
@@ -0,0 +1,4 @@
+class CarsController < EngineController
+  def create
+  end
+end
diff --git a/test/mock/engine/app/controllers/engine_controller.rb b/test/mock/engine/app/controllers/engine_controller.rb
new file mode 100644
index 0000000..7140c9e
--- /dev/null
+++ b/test/mock/engine/app/controllers/engine_controller.rb
@@ -0,0 +1,2 @@
+class EngineController
+end
diff --git a/test/mock/rails.rb b/test/mock/rails.rb
index 3c2771a..7264ea1 100644
--- a/test/mock/rails.rb
+++ b/test/mock/rails.rb
@@ -6,8 +6,18 @@
 # The mocks used should reflect the files located in `test/mock/app/`.
 
 require "pathname"
+require_relative "engine"
+require_relative "route_helper"
+
+require_relative "app/controllers/application_controller"
+require_relative "app/controllers/waterlilies_controller"
+require_relative "app/controllers/api/burritos_controller"
+require_relative "app/controllers/api/cakes_controller"
+require_relative "app/controllers/api/tacos_controller"
 
 module Rails
+  extend RouteHelper
+
   class << self
     # Lets us call `Rails.application.routes.routes` without a skeleton Rails
     # app.
@@ -101,6 +111,10 @@ def application
           verb: "GET",
           path: "/one-off",
           name: nil
+      routes.push \
+        mock_engine \
+          engine: Engine,
+          path: "/engine"
 
       app = Minitest::Mock.new
       app_routes = Minitest::Mock.new
@@ -109,50 +123,11 @@ def application
       app
     end
 
-    # Define an allowlist of controller/actions that will be mocked.
-    #
-    # @param route_allowlist [Array<String>] In format "controller#action"
-    # @return [void]
-    def set_route_allowlist(route_allowlist)
-      @@route_allowlist = route_allowlist
-    end
-
     # Lets us call `Rails.root` without a skeleton Rails app.
     #
     # @return [Pathname] Pathname object like Rails.root
     def root
-      Pathname.new("test/mock")
-    end
-
-    private
-
-    # Stored procedure for mocking a new route.
-    #
-    # @param controller [String] Mocked controller name
-    # @param action [String] Mocked action name
-    # @param verb [String] HTTP verb
-    # @param path [String] Mocked Rails path
-    # @param name [String] Mocked route name
-    # @param defaults [Hash] Mocked default params
-    # @return [Minitest::Mock, nil] Mocked route
-    def mock_route(controller:, action:, verb:, path:, name:, defaults: {})
-      @@route_allowlist ||= []
-      return if @@route_allowlist.any? && @@route_allowlist.index("#{controller}##{action}").nil?
-
-      route = Minitest::Mock.new
-      route.expect(:defaults, {controller: controller, action: action, **defaults})
-      route.expect(:verb, verb)
-      route_path = Minitest::Mock.new
-
-      # We'll be calling these particular methods more than once to test for
-      # duplicate-removal, hence wrapping in `.times` block.
-      2.times do
-        route_path.expect(:spec, path)
-        route.expect(:path, route_path)
-        route.expect(:name, name)
-      end
-
-      route
+      Pathname.new("test/mock").realpath
     end
   end
 end
diff --git a/test/mock/route_helper.rb b/test/mock/route_helper.rb
new file mode 100644
index 0000000..3b61db9
--- /dev/null
+++ b/test/mock/route_helper.rb
@@ -0,0 +1,59 @@
+module RouteHelper
+  # Define an allowlist of controller/actions that will be mocked.
+  #
+  # @param route_allowlist [Array<String>] In format "controller#action"
+  # @return [void]
+  def set_route_allowlist(route_allowlist)
+    @@route_allowlist = route_allowlist
+  end
+
+  # Stored procedure for mocking a new route.
+  #
+  # @param controller [String] Mocked controller name
+  # @param action [String] Mocked action name
+  # @param verb [String] HTTP verb
+  # @param path [String] Mocked Rails path
+  # @param name [String] Mocked route name
+  # @param defaults [Hash] Mocked default params
+  # @return [Minitest::Mock, nil] Mocked route
+  def mock_route(controller:, action:, verb:, path:, name:, defaults: {})
+    @@route_allowlist ||= []
+    return if @@route_allowlist.any? && @@route_allowlist.index("#{controller}##{action}").nil?
+
+    app = Minitest::Mock.new
+    app.expect(:engine?, false)
+
+    route = Minitest::Mock.new
+    route.expect(:defaults, {controller: controller, action: action, **defaults})
+    route.expect(:verb, verb)
+    route.expect(:app, app)
+    route_path = Minitest::Mock.new
+
+    # We'll be calling these particular methods more than once to test for
+    # duplicate-removal, hence wrapping in `.times` block.
+    2.times do
+      route_path.expect(:spec, path)
+      route.expect(:path, route_path)
+      route.expect(:name, name)
+    end
+
+    route
+  end
+
+  # Stored procedure to mock a new engine.
+  #
+  # @param engine [Module] Mocked engine module
+  # @param path [String] Path for mocked engine
+  # @return [Minitest::Mock]
+  def mock_engine(engine:, path:)
+    route = Minitest::Mock.new
+
+    # We'll be calling these particular methods more than once to test for
+    # duplicate-removal, hence wrapping in `.times` block.
+    2.times do
+      route.expect(:app, engine)
+    end
+
+    route
+  end
+end
diff --git a/test/routes_test.rb b/test/routes_test.rb
index cda3ac5..003c52e 100644
--- a/test/routes_test.rb
+++ b/test/routes_test.rb
@@ -37,6 +37,11 @@ def test_mock_rails
           "one_off" => [
             {verb: "GET", path: "/one-off", name: nil, defaults: {}}
           ]
+        },
+        "cars" => {
+          "create" => [
+            {verb: "POST", path: "/engine/cars", name: "car", defaults: {}}
+          ]
         }
       }