Annotate controllers in Rails engines and packs (#48)

~~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 (錦華) <[email protected]>

.gitignore

@@ -7,5 +7,7 @@
 /spec/reports/
 /tmp/
 
+.DS_Store
+
 .ruby-version
 Gemfile.lock

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)

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

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

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,71 +38,76 @@ 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
     exit_code = 0
 
     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

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.

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

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

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

test/mock/app/controllers/application_controller.rb

@@ -0,0 +1,2 @@
+class ApplicationController
+end

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