From 5b2761fad775347b10e06ce925a6a8b10ad40d6c Mon Sep 17 00:00:00 2001 From: David Stosik Date: Fri, 18 Aug 2023 22:33:47 +0900 Subject: [PATCH] Make the task preloading process configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In ordered to be displayed in the UI, tasks have to be loaded by the time `MaintenanceTasks::Task.descendants` is called. To achieve that, the `Task.load_constants` method was previously implemented, and called from `Task.available_tasks`. A few inconvenients of that method are: - It could easily be abused and used in lower levels of the gem, where ideally it should ne be necessary to load all tasks to achieve the gem's goal. See for example my previous PR [Avoid loading all tasks to validate a `Run#task_name`](https://github.com/Shopify/maintenance_tasks/pull/866). - It can introduce unnecessary overhead. For example, displaying the CLI's basic help message with `maintenance_tasks help` will necessitate all tasks to be loaded (even though the help message does not display anything related to the available tasks), and this loading time will delay the appearance of that help message. Similarly, a single test case that want to test a single task's behaviour, while never displaying the whole list of available tasks, would be negatively impacted by a misuse of `Task.available_tasks`. - It does not allow an application to define an alternative way to load all constants. As a matter of fact, the Shopify Core monolith monkey-patched that method to allow multiple "task modules" instead of a single one. With a configurable `task_loader`, monkey-patching would not be necessary. (Which is good. 🙈) This PR introduces `MaintenanceTasks.task_loader`, a configuration that allows applications to change the logic applied to ensure all task are loaded before being displayed. As a bonus: - it changes how the CLI generates the long description containing all task names, so that tasks get loaded at the last minute, only when necessary - it gets rid of `Task.available_tasks` (its only use was for UI purposes, so I changed it to a direct call to `Task.descendants`) - it introduces some tests of the loading behaviour --- README.md | 33 +++++++++++++ .../maintenance_tasks/default_task_loader.rb | 25 ++++++++++ app/models/maintenance_tasks/task.rb | 24 ---------- .../maintenance_tasks/task_data_index.rb | 5 +- lib/maintenance_tasks.rb | 6 +++ lib/maintenance_tasks/cli.rb | 27 +++++++---- test/dummy/lib/not_autoloaded_task.rb | 9 ++++ test/lib/maintenance_tasks/cli_test.rb | 47 +++++++++++++++++++ .../default_task_loader_test.rb | 39 +++++++++++++++ .../maintenance_tasks/task_data_index_test.rb | 8 ++++ test/models/maintenance_tasks/task_test.rb | 22 --------- 11 files changed, 189 insertions(+), 56 deletions(-) create mode 100644 app/models/maintenance_tasks/default_task_loader.rb create mode 100644 test/dummy/lib/not_autoloaded_task.rb create mode 100644 test/models/maintenance_tasks/default_task_loader_test.rb diff --git a/README.md b/README.md index 99f5c8fe..20547d54 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,10 @@ Please ensure your application is using information, please consult the [Rails guides on autoloading and reloading constants](https://guides.rubyonrails.org/autoloading_and_reloading_constants.html). +You might be able to define a different +[_task loader_](#customizing-the-way-the-gem-loads-all-tasks) that would allow +you to circumvent this limitation. + ## Usage The typical Maintenance Tasks workflow is as follows: @@ -716,6 +720,35 @@ module Maintenance end ``` +#### Customizing the way the gem loads all tasks + +For the UI (Web and CLI) to be able to display all available tasks, their +respective classes must have been previously loaded. This can be an issue +for applications running without +[eager loading](https://guides.rubyonrails.org/configuring.html#config-eager-load) +(for example a default Rails application uses lazy loading in development +mode). + +To work around that, this gem uses a task loader, which can be customized +by changing the `MaintenanceTasks.task_loader` in the configuration: + +```rb +# config/initializers/maintenance_tasks.rb + +MaintenanceTasks.task_loader = -> { DefaultTaskLoader.load_all } +``` + +By using a different proc, you can define a different way to preload all tasks. +You could for example browse for tasks in a given directory: + +```rb +# config/initializers/maintenance_tasks.rb + +MaintenanceTasks.task_loader = -> { + Dir[Rails.root.join("app", "tasks", "**", "*_task.rb")].each { |file| require file } +} +``` + #### Customizing the underlying job class `MaintenanceTasks.job` can be configured to define a Job class for your tasks to diff --git a/app/models/maintenance_tasks/default_task_loader.rb b/app/models/maintenance_tasks/default_task_loader.rb new file mode 100644 index 00000000..14f16247 --- /dev/null +++ b/app/models/maintenance_tasks/default_task_loader.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module MaintenanceTasks + # This class defines how the gem will find and load all tasks defined by the application. + # It can be overridden by the application to change the way tasks are loaded. + class DefaultTaskLoader + class << self + # Recursively browse the MaintenanceTasks.tasks_module namespace to load all defined tasks. + # + # @return [void] + def load_all + load_constants(MaintenanceTasks.tasks_module.safe_constantize) + end + + private + + def load_constants(namespace) + namespace.constants.each do |name| + object = namespace.const_get(name) + load_constants(object) if object.instance_of?(Module) + end + end + end + end +end diff --git a/app/models/maintenance_tasks/task.rb b/app/models/maintenance_tasks/task.rb index 049db9ed..6c7de407 100644 --- a/app/models/maintenance_tasks/task.rb +++ b/app/models/maintenance_tasks/task.rb @@ -41,15 +41,6 @@ def named(name) task end - # Returns a list of concrete classes that inherit from the Task - # superclass. - # - # @return [Array] the list of classes. - def available_tasks - load_constants - descendants - end - # Make this Task a task that handles CSV. # # @param in_batches [Integer] optionally, supply a batch size if the CSV @@ -168,21 +159,6 @@ def after_cancel(*filter_list, &block) def after_error(*filter_list, &block) set_callback(:error, :after, *filter_list, &block) end - - private - - def load_constants - namespace = MaintenanceTasks.tasks_module.safe_constantize - return unless namespace - - load_const = lambda do |root| - root.constants.each do |name| - object = root.const_get(name) - load_const.call(object) if object.instance_of?(Module) - end - end - load_const.call(namespace) - end end # The contents of a CSV file to be processed by a Task. diff --git a/app/models/maintenance_tasks/task_data_index.rb b/app/models/maintenance_tasks/task_data_index.rb index 7ad5c9cb..06f82d32 100644 --- a/app/models/maintenance_tasks/task_data_index.rb +++ b/app/models/maintenance_tasks/task_data_index.rb @@ -25,7 +25,10 @@ class << self def available_tasks tasks = [] - task_names = Task.available_tasks.map(&:name) + # Ensure all tasks are loaded before listing them. + MaintenanceTasks.task_loader.call + + task_names = Task.descendants.map(&:name) active_runs = Run.with_attached_csv.active.where(task_name: task_names) active_runs.each do |run| diff --git a/lib/maintenance_tasks.rb b/lib/maintenance_tasks.rb index d7ff79b8..6ff7989f 100644 --- a/lib/maintenance_tasks.rb +++ b/lib/maintenance_tasks.rb @@ -21,6 +21,12 @@ module MaintenanceTasks # @return [String] the name of the module. mattr_accessor :tasks_module, default: "Maintenance" + # @!atribute task_loader + # @scope class + # + # A Proc that loads all the Tasks to be used by the application. + mattr_accessor :task_loader, default: -> { DefaultTaskLoader.load_all } + # @!attribute job # @scope class # diff --git a/lib/maintenance_tasks/cli.rb b/lib/maintenance_tasks/cli.rb index ac5ba1ce..9ffe5395 100644 --- a/lib/maintenance_tasks/cli.rb +++ b/lib/maintenance_tasks/cli.rb @@ -16,15 +16,6 @@ def exit_on_failure? desc "perform [TASK NAME]", "Runs the given Maintenance Task" - long_desc <<-LONGDESC - `maintenance_tasks perform` will run the Maintenance Task specified by the - [TASK NAME] argument. - - Available Tasks: - - #{MaintenanceTasks::Task.available_tasks.join("\n\n")} - LONGDESC - # Specify the CSV file to process for CSV Tasks desc = "Supply a CSV file to be processed by a CSV Task, "\ "--csv path/to/csv/file.csv" @@ -50,6 +41,24 @@ def perform(name) say_status(:error, error.message, :red) end + # `long_desc` only allows us to use a static string as "long description". + # By redefining the `#long_description` method on the "perform" Command + # object instead, we make it dynamic, thus delaying the task loading + # process until it's actually required. + commands["perform"].define_singleton_method(:long_description) do + # Ensure all tasks are loaded before listing them. + MaintenanceTasks.task_loader.call + + <<~LONGDESC + `maintenance_tasks perform` will run the Maintenance Task specified by the + [TASK NAME] argument. + + Available Tasks: + + #{Task.descendants.map(&:name).sort.join("\n\n")} + LONGDESC + end + private def csv_file diff --git a/test/dummy/lib/not_autoloaded_task.rb b/test/dummy/lib/not_autoloaded_task.rb new file mode 100644 index 00000000..9c233ad7 --- /dev/null +++ b/test/dummy/lib/not_autoloaded_task.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class NotAutoloadedTask < MaintenanceTasks::Task + def collection + [1, 2] + end + + def process(_); end +end diff --git a/test/lib/maintenance_tasks/cli_test.rb b/test/lib/maintenance_tasks/cli_test.rb index 795ebfd8..768cfe84 100644 --- a/test/lib/maintenance_tasks/cli_test.rb +++ b/test/lib/maintenance_tasks/cli_test.rb @@ -84,5 +84,52 @@ class CLITest < ActiveSupport::TestCase CLI.start(["perform", "MyParamsTask", "--arguments", "post_ids:1,2,3"]) end end + + test "`help perform` loads all tasks and displays them" do + # Ensure test tasks have been loaded + MaintenanceTasks.task_loader.call + + dummy_loader = -> {} + MaintenanceTasks.expects(:task_loader).returns(dummy_loader).at_least_once + dummy_loader.expects(:call).at_least_once + + expected_output = <<~TASKS + Available Tasks: + + Maintenance::BatchImportPostsTask + + Maintenance::CallbackTestTask + + Maintenance::CancelledEnqueueTask + + Maintenance::EnqueueErrorTask + + Maintenance::ErrorTask + + Maintenance::ImportPostsTask + + Maintenance::Nested::NestedMore::NestedMoreTask + + Maintenance::Nested::NestedTask + + Maintenance::NoCollectionTask + + Maintenance::ParamsTask + + Maintenance::TestTask + + Maintenance::UpdatePostsInBatchesTask + + Maintenance::UpdatePostsModulePrependedTask + + Maintenance::UpdatePostsTask + + Maintenance::UpdatePostsThrottledTask + TASKS + + assert_output(/#{expected_output.indent(2)}/) do + CLI.start(["help", "perform"]) + end + end end end diff --git a/test/models/maintenance_tasks/default_task_loader_test.rb b/test/models/maintenance_tasks/default_task_loader_test.rb new file mode 100644 index 00000000..8fd8ef90 --- /dev/null +++ b/test/models/maintenance_tasks/default_task_loader_test.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "test_helper" + +module MaintenanceTasks + class DefaultTaskLoaderTest < ActiveSupport::TestCase + include ActiveSupport::Testing::Isolation + + test ".load_all loads all autoloadable Task subclasses within the Maintenance namespace" do + [ + Maintenance::BatchImportPostsTask, + Maintenance::CallbackTestTask, + Maintenance::CancelledEnqueueTask, + Maintenance::EnqueueErrorTask, + Maintenance::ErrorTask, + Maintenance::ImportPostsTask, + Maintenance::Nested, + Maintenance::Nested::NestedMore, + Maintenance::Nested::NestedMore::NestedMoreTask, + Maintenance::Nested::NestedTask, + Maintenance::NoCollectionTask, + Maintenance::ParamsTask, + Maintenance::TestTask, + Maintenance::UpdatePostsInBatchesTask, + Maintenance::UpdatePostsModulePrependedTask, + Maintenance::UpdatePostsTask, + Maintenance::UpdatePostsThrottledTask, + ].each do |constant| + constant + .module_parent + .expects(:const_get) + .with(constant.name.demodulize.to_sym) + .returns(constant) + end + + DefaultTaskLoader.load_all + end + end +end diff --git a/test/models/maintenance_tasks/task_data_index_test.rb b/test/models/maintenance_tasks/task_data_index_test.rb index c51357dc..61db3d7e 100644 --- a/test/models/maintenance_tasks/task_data_index_test.rb +++ b/test/models/maintenance_tasks/task_data_index_test.rb @@ -27,6 +27,14 @@ class TaskDataIndexTest < ActiveSupport::TestCase assert_equal expected, TaskDataIndex.available_tasks.map(&:name) end + test ".available_tasks calls MaintenanceTasks.task_loader to load all tasks" do + dummy_loader = -> {} + MaintenanceTasks.expects(:task_loader).returns(dummy_loader) + dummy_loader.expects(:call) + + TaskDataIndex.available_tasks + end + test "#new sets last_run if one is passed as an argument" do run = Run.create!(task_name: "Maintenance::UpdatePostsTask") task_data = TaskDataIndex.new("Maintenance::UpdatePostsTask", run) diff --git a/test/models/maintenance_tasks/task_test.rb b/test/models/maintenance_tasks/task_test.rb index f36b2333..60c5112f 100644 --- a/test/models/maintenance_tasks/task_test.rb +++ b/test/models/maintenance_tasks/task_test.rb @@ -4,28 +4,6 @@ module MaintenanceTasks class TaskTest < ActiveSupport::TestCase - test ".available_tasks returns list of tasks that inherit from the Task superclass" do - expected = [ - "Maintenance::BatchImportPostsTask", - "Maintenance::CallbackTestTask", - "Maintenance::CancelledEnqueueTask", - "Maintenance::EnqueueErrorTask", - "Maintenance::ErrorTask", - "Maintenance::ImportPostsTask", - "Maintenance::Nested::NestedMore::NestedMoreTask", - "Maintenance::Nested::NestedTask", - "Maintenance::NoCollectionTask", - "Maintenance::ParamsTask", - "Maintenance::TestTask", - "Maintenance::UpdatePostsInBatchesTask", - "Maintenance::UpdatePostsModulePrependedTask", - "Maintenance::UpdatePostsTask", - "Maintenance::UpdatePostsThrottledTask", - ] - assert_equal expected, - MaintenanceTasks::Task.available_tasks.map(&:name).sort - end - test ".named returns the task based on its name" do expected_task = Maintenance::UpdatePostsTask assert_equal expected_task, Task.named("Maintenance::UpdatePostsTask")