From dc8af587409a8d2ddfefa5faff06939c13527c42 Mon Sep 17 00:00:00 2001 From: Nishiki Date: Sun, 10 Nov 2024 14:18:51 -0800 Subject: [PATCH] fix: inherited actions (#58) # Issue Closes #57. # Overview This PR updates Chusaku to account for controller actions that are passed down through object-oriented class inheritance. The high-level changes include: - Store `#source_location` data in `Chusaku::Routes` as `source_path`. - Map source paths to corresponding actions data and loop over that to annotate files. - Update tests. --- lib/chusaku.rb | 40 ++++-- lib/chusaku/routes.rb | 77 +++++++++-- test/chusaku_test.rb | 20 ++- .../app/controllers/api/cakes_controller.rb | 2 +- .../controllers/api/croissants_controller.rb | 2 + .../app/controllers/pastries_controller.rb | 5 + test/mock/rails.rb | 16 +++ test/routes_test.rb | 124 ++++++++++++++++-- 8 files changed, 245 insertions(+), 41 deletions(-) create mode 100644 test/mock/app/controllers/api/croissants_controller.rb create mode 100644 test/mock/app/controllers/pastries_controller.rb diff --git a/lib/chusaku.rb b/lib/chusaku.rb index a004be8..edbcf97 100644 --- a/lib/chusaku.rb +++ b/lib/chusaku.rb @@ -29,14 +29,7 @@ def call(flags = {}) .new(Rails.root.join(controllers_pattern)) .exclude(Rails.root.join(exclusion_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) - - source_path = controller_class.instance_method(action_method_name).source_location&.[](0) + source_paths_map.each do |source_path, actions| next unless controllers_paths.include?(source_path) annotate_file(path: source_path, actions: actions) @@ -56,6 +49,34 @@ def load_tasks private + # Maps source paths to their respective routes. + # + # Example output: + # + # { + # "/path/to/users_controller.rb" => { + # "edit" => [...], + # "update" => [...] + # } + # } + # + # @return [Hash] Source paths mapped to their respective routes + def source_paths_map + map = {} + + @routes.each do |controller, actions| + actions.each do |action, data| + data.each do |datum| + map[datum[:source_path]] ||= {} + map[datum[:source_path]][action] ||= [] + map[datum[:source_path]][action].push(datum) + end + end + end + + map + end + # Adds annotations to the given file. # # @param path [String] Path to file @@ -110,8 +131,9 @@ def annotate_group(group:, route_data:) # @param path [String] Rails path for route # @param name [String] Name used in route helpers # @param defaults [Hash] Default parameters for route + # @param source_path [String] Path to controller file # @return [String] "@route {} ()" - def annotate_route(verb:, path:, name:, defaults:) + def annotate_route(verb:, path:, name:, defaults:, source_path:) annotation = "@route #{verb} #{path}" if defaults&.any? defaults_str = diff --git a/lib/chusaku/routes.rb b/lib/chusaku/routes.rb index 585c714..83410a9 100644 --- a/lib/chusaku/routes.rb +++ b/lib/chusaku/routes.rb @@ -9,16 +9,40 @@ class << self # { # "users" => { # "edit" => [ - # {verb: "GET", path: "/users/:id", name: "edit_user"} + # { + # verb: "GET", + # path: "/users/:id", + # name: "edit_user", + # defaults: {}, + # source_path: "/path/to/users_controller.rb" + # } # ], # "update" => [ - # {verb: "PATCH", path: "/users", name: "edit_user"}, - # {verb: "PUT", path: "/users", name: "edit_user"} + # { + # verb: "PATCH", + # path: "/users", + # name: "edit_user", + # defaults: {}, + # source_path: "/path/to/users_controller.rb" + # }, + # { + # verb: "PUT", + # path: "/users", + # name: "edit_user", + # defaults: {}, + # source_path: "/path/to/users_controller.rb" + # } # ] # }, # "empanadas" => { # "create" => [ - # {verb: "POST", path: "/empanadas", name: nil} + # { + # verb: "POST", + # path: "/empanadas", + # name: nil, + # defaults: {}, + # source_path: "/path/to/empanadas_controller.rb" + # } # ] # } # } @@ -33,6 +57,12 @@ def call private + # Recursively populate the routes hash with information from the given Rails + # application. Accounts for Rails engines. + # + # @param app [Rails::Application] Result of `Rails.application` + # @param routes [Hash] Collection of all route info + # @return [void] def populate_routes(app, routes) app.routes.routes.each do |route| if route.app.engine? @@ -40,7 +70,7 @@ def populate_routes(app, routes) next end - controller, action, defaults = extract_data_from(route) + controller, action, defaults, source_path = extract_data_from(route) routes[controller] ||= {} routes[controller][action] ||= [] @@ -49,7 +79,8 @@ def populate_routes(app, routes) routes: routes, controller: controller, action: action, - defaults: defaults + defaults: defaults, + source_path: source_path end end @@ -60,11 +91,17 @@ def populate_routes(app, routes) # @param controller [String] Controller key # @param action [String] Action key # @param defaults [Hash] Default parameters for route + # @param source_path [String] Path to controller file # @return [void] - def add_info_for(route:, routes:, controller:, action:, defaults:) + def add_info_for(route:, routes:, controller:, action:, defaults:, source_path:) verbs_for(route).each do |verb| - routes[controller][action] - .push(format(route: route, verb: verb, defaults: defaults)) + routes[controller][action].push \ + format( + route: route, + verb: verb, + defaults: defaults, + source_path: source_path + ) routes[controller][action].uniq! end end @@ -88,13 +125,15 @@ def verbs_for(route) # @param route [ActionDispatch::Journey::Route] Route given by Rails # @param verb [String] HTTP verb # @param defaults [Hash] Default parameters for route + # @param source_path [String] Path to controller file # @return [Hash] { verb => String, path => String, name => String } - def format(route:, verb:, defaults:) + def format(route:, verb:, defaults:, source_path:) { verb: verb, path: route.path.spec.to_s.gsub("(.:format)", ""), name: route.name, - defaults: defaults + defaults: defaults, + source_path: source_path } end @@ -143,16 +182,26 @@ def backfill_routes(routes) routes end - # Given a route, extract the controller and action strings. + # Given a route, extract the controller & action strings as well as defaults + # hash and source path. # # @param route [ActionDispatch::Journey::Route] Route instance - # @return [Array] (String, String, Hash) + # @return [Array] (String, String, Hash, String) def extract_data_from(route) defaults = route.defaults.dup controller = defaults.delete(:controller) action = defaults.delete(:action) - [controller, action, defaults] + controller_class = controller ? "#{controller.underscore.camelize}Controller".constantize : nil + action_method_name = action&.to_sym + source_path = + if !action_method_name.nil? && controller_class&.method_defined?(action_method_name) + controller_class.instance_method(action_method_name).source_location&.[](0) + else + "" + end + + [controller, action, defaults, source_path] end end end diff --git a/test/chusaku_test.rb b/test/chusaku_test.rb index 36e541f..1150ba1 100644 --- a/test/chusaku_test.rb +++ b/test/chusaku_test.rb @@ -17,7 +17,7 @@ out, _err = capture_io { exit_code = Chusaku.call(error_on_annotation: true) } assert_equal(1, exit_code) - assert_equal(4, File.written_files.count) + assert_equal(5, File.written_files.count) assert_includes(out, "Exited with status code 1.") end @@ -53,13 +53,13 @@ engine_path = "#{Rails.root}/engine/app/controllers" assert_equal(0, exit_code) - assert_equal(4, files.count) + assert_equal(5, files.count) refute_includes(files, "#{app_path}/api/burritos_controller.rb") expected = <<~HEREDOC module Api - class CakesController < ApplicationController + class CakesController < PastriesController # 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) @@ -109,6 +109,18 @@ def tacos_params HEREDOC assert_equal(expected, files["#{app_path}/api/tacos_controller.rb"]) + expected = + <<~HEREDOC + class PastriesController < ApplicationController + # This should be annotated for each child controller that inherits from this class. + # @route GET /api/cakes (cakes) + # @route GET /api/croissants (croissants) + def index + end + end + HEREDOC + assert_equal(expected, files["#{app_path}/pastries_controller.rb"]) + expected = <<~HEREDOC class WaterliliesController < ApplicationController @@ -165,7 +177,7 @@ def create out, = capture_io { exit_code = Chusaku.call(args) } assert_equal(0, exit_code) - assert_equal(3, File.written_files.count) + assert_equal(4, File.written_files.count) assert_equal("Chusaku has finished running.\n", out) end end diff --git a/test/mock/app/controllers/api/cakes_controller.rb b/test/mock/app/controllers/api/cakes_controller.rb index c2cc3f1..a78ec5c 100644 --- a/test/mock/app/controllers/api/cakes_controller.rb +++ b/test/mock/app/controllers/api/cakes_controller.rb @@ -1,5 +1,5 @@ module Api - class CakesController < ApplicationController + class CakesController < PastriesController # This route's GET action should be named the same as its PUT action, # even though only the PUT action is named. def inherit diff --git a/test/mock/app/controllers/api/croissants_controller.rb b/test/mock/app/controllers/api/croissants_controller.rb new file mode 100644 index 0000000..050fe17 --- /dev/null +++ b/test/mock/app/controllers/api/croissants_controller.rb @@ -0,0 +1,2 @@ +class Api::CroissantsController < PastriesController +end diff --git a/test/mock/app/controllers/pastries_controller.rb b/test/mock/app/controllers/pastries_controller.rb new file mode 100644 index 0000000..d483902 --- /dev/null +++ b/test/mock/app/controllers/pastries_controller.rb @@ -0,0 +1,5 @@ +class PastriesController < ApplicationController + # This should be annotated for each child controller that inherits from this class. + def index + end +end diff --git a/test/mock/rails.rb b/test/mock/rails.rb index 7264ea1..cbfd34f 100644 --- a/test/mock/rails.rb +++ b/test/mock/rails.rb @@ -10,9 +10,11 @@ require_relative "route_helper" require_relative "app/controllers/application_controller" +require_relative "app/controllers/pastries_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/croissants_controller" require_relative "app/controllers/api/tacos_controller" module Rails @@ -47,6 +49,20 @@ def application verb: "PUT", path: "/api/cakes/inherit(.:format)", name: "inherit" + routes.push \ + mock_route \ + controller: "api/cakes", + action: "index", + verb: "GET", + path: "/api/cakes(.:format)", + name: "cakes" + routes.push \ + mock_route \ + controller: "api/croissants", + action: "index", + verb: "GET", + path: "/api/croissants(.:format)", + name: "croissants" routes.push \ mock_route \ controller: "api/tacos", diff --git a/test/routes_test.rb b/test/routes_test.rb index 471799b..6ee45b5 100644 --- a/test/routes_test.rb +++ b/test/routes_test.rb @@ -6,41 +6,139 @@ { "api/burritos" => { "create" => [ - {verb: "POST", path: "/api/burritos", name: "burritos", defaults: {}} + { + verb: "POST", + path: "/api/burritos", + name: "burritos", + defaults: {}, + source_path: Api::BurritosController.instance_method(:create).source_location.first + } ] }, "api/cakes" => { "inherit" => [ - {verb: "GET", path: "/api/cakes/inherit", name: "inherit", defaults: {}}, - {verb: "PUT", path: "/api/cakes/inherit", name: "inherit", defaults: {}} + { + verb: "GET", + path: "/api/cakes/inherit", + name: "inherit", + defaults: {}, + source_path: Api::CakesController.instance_method(:inherit).source_location.first + }, + { + verb: "PUT", + path: "/api/cakes/inherit", + name: "inherit", + defaults: {}, + source_path: Api::CakesController.instance_method(:inherit).source_location.first + } + ], + "index" => [ + { + verb: "GET", + path: "/api/cakes", + name: "cakes", + defaults: {}, + source_path: Api::CakesController.instance_method(:index).source_location.first + } + ] + }, + "api/croissants" => { + "index" => [ + { + verb: "GET", + path: "/api/croissants", + name: "croissants", + defaults: {}, + source_path: Api::CroissantsController.instance_method(:index).source_location.first + } ] }, "api/tacos" => { "show" => [ - {verb: "GET", path: "/", name: "root", defaults: {}}, - {verb: "GET", path: "/api/tacos/:id", name: "taco", defaults: {}} + { + verb: "GET", + path: "/", + name: "root", + defaults: {}, + source_path: Api::TacosController.instance_method(:show).source_location.first + }, + { + verb: "GET", + path: "/api/tacos/:id", + name: "taco", + defaults: {}, + source_path: Api::TacosController.instance_method(:show).source_location.first + } ], "create" => [ - {verb: "POST", path: "/api/tacos", name: "tacos", defaults: {}} + { + verb: "POST", + path: "/api/tacos", + name: "tacos", + defaults: {}, + source_path: Api::TacosController.instance_method(:create).source_location.first + } ], "update" => [ - {verb: "PUT", path: "/api/tacos/:id", name: "taco", defaults: {}}, - {verb: "PATCH", path: "/api/tacos/:id", name: "taco", defaults: {}} + { + verb: "PUT", + path: "/api/tacos/:id", + name: "taco", + defaults: {}, + source_path: Api::TacosController.instance_method(:update).source_location.first + }, + { + verb: "PATCH", + path: "/api/tacos/:id", + name: "taco", + defaults: {}, + source_path: Api::TacosController.instance_method(:update).source_location.first + } ] }, "waterlilies" => { "show" => [ - {verb: "GET", path: "/waterlilies/:id", name: "waterlilies", defaults: {}}, - {verb: "GET", path: "/waterlilies/:id", name: "waterlilies2", defaults: {}}, - {verb: "GET", path: "/waterlilies/:id", name: "waterlilies_blue", defaults: {blue: true}} + { + verb: "GET", + path: "/waterlilies/:id", + name: "waterlilies", + defaults: {}, + source_path: WaterliliesController.instance_method(:show).source_location.first + }, + { + verb: "GET", + path: "/waterlilies/:id", + name: "waterlilies2", + defaults: {}, + source_path: WaterliliesController.instance_method(:show).source_location.first + }, + { + verb: "GET", + path: "/waterlilies/:id", + name: "waterlilies_blue", + defaults: {blue: true}, + source_path: WaterliliesController.instance_method(:show).source_location.first + } ], "one_off" => [ - {verb: "GET", path: "/one-off", name: nil, defaults: {}} + { + verb: "GET", + path: "/one-off", + name: nil, + defaults: {}, + source_path: WaterliliesController.instance_method(:one_off).source_location.first + } ] }, "cars" => { "create" => [ - {verb: "POST", path: "/engine/cars", name: "car", defaults: {}} + { + verb: "POST", + path: "/engine/cars", + name: "car", + defaults: {}, + source_path: CarsController.instance_method(:create).source_location.first + } ] } }