Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the task preloading process configurable #867

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions app/models/maintenance_tasks/default_task_loader.rb
Original file line number Diff line number Diff line change
@@ -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
24 changes: 0 additions & 24 deletions app/models/maintenance_tasks/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,6 @@ def named(name)
task
end

# Returns a list of concrete classes that inherit from the Task
# superclass.
#
# @return [Array<Class>] 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
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion app/models/maintenance_tasks/task_data_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
6 changes: 6 additions & 0 deletions lib/maintenance_tasks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
#
Expand Down
27 changes: 18 additions & 9 deletions lib/maintenance_tasks/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
9 changes: 9 additions & 0 deletions test/dummy/lib/not_autoloaded_task.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

class NotAutoloadedTask < MaintenanceTasks::Task
def collection
[1, 2]
end

def process(_); end
end
47 changes: 47 additions & 0 deletions test/lib/maintenance_tasks/cli_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
39 changes: 39 additions & 0 deletions test/models/maintenance_tasks/default_task_loader_test.rb
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test fails in Rails 6.
Has it possibly identified an issue with the gem in older Rails versions? I haven't altered the recursive loading algorithm, just moved it from Task.load_constants to DefaultTaskLoader.load_constants. 🤔

Copy link
Contributor Author

@davidstosik davidstosik Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failure:
MaintenanceTasks::DefaultTaskLoaderTest#test_.load_all_loads_all_autoloadable_Task_subclasses_within_the_Maintenance_namespace [/home/runner/work/maintenance_tasks/maintenance_tasks/vendor/bundle/ruby/3.0.0/gems/activesupport-6.0.6.1/lib/active_support/inflector/methods.rb:284]:
unexpected invocation: Maintenance.const_get("Nested")
unsatisfied expectations:
- expected exactly once, invoked never: Maintenance.const_get(:Nested)
- expected exactly once, invoked never: Maintenance.const_get(:ImportPostsTask)
- expected exactly once, invoked never: Maintenance.const_get(:ErrorTask)
- expected exactly once, invoked never: Maintenance.const_get(:EnqueueErrorTask)
- expected exactly once, invoked never: Maintenance.const_get(:CancelledEnqueueTask)
- expected exactly once, invoked never: Maintenance.const_get(:CallbackTestTask)
- expected exactly once, invoked never: Maintenance.const_get(:BatchImportPostsTask)

Wonder if it's just because of a different order... 🤔

Update: nothing's broken, it's just that the order in which I define the call expects matters, and different versions of Rails produce different orders.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like String vs Symbol, "Nested" vs :Nested, no?

Copy link
Contributor Author

@davidstosik davidstosik Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the order returned by .constants seems to differ depending on the version of Rails.
This would work in Ruby 6.1 for example:

diff --git i/app/models/maintenance_tasks/default_task_loader.rb w/app/models/maintenance_tasks/default_task_loader.rb
index 14f1624..fed0ea5 100644
--- i/app/models/maintenance_tasks/default_task_loader.rb
+++ w/app/models/maintenance_tasks/default_task_loader.rb
@@ -15,7 +15,8 @@ module MaintenanceTasks
       private
 
       def load_constants(namespace)
-        namespace.constants.each do |name|
+        namespace.constants.sort.each do |name|
           object = namespace.const_get(name)
           load_constants(object) if object.instance_of?(Module)
         end
diff --git i/test/models/maintenance_tasks/default_task_loader_test.rb w/test/models/maintenance_tasks/default_task_loader_test.rb
index 8fd8ef9..b029518 100644
--- i/test/models/maintenance_tasks/default_task_loader_test.rb
+++ w/test/models/maintenance_tasks/default_task_loader_test.rb
@@ -8,6 +8,9 @@ module MaintenanceTasks
 
     test ".load_all loads all autoloadable Task subclasses within the Maintenance namespace" do
       [
+        Maintenance::Nested::NestedMore::NestedMoreTask,
+        Maintenance::Nested::NestedTask,
+        Maintenance::Nested::NestedMore,
         Maintenance::BatchImportPostsTask,
         Maintenance::CallbackTestTask,
         Maintenance::CancelledEnqueueTask,
@@ -15,9 +18,6 @@ module MaintenanceTasks
         Maintenance::ErrorTask,
         Maintenance::ImportPostsTask,
         Maintenance::Nested,
-        Maintenance::Nested::NestedMore,
-        Maintenance::Nested::NestedMore::NestedMoreTask,
-        Maintenance::Nested::NestedTask,
         Maintenance::NoCollectionTask,
         Maintenance::ParamsTask,
         Maintenance::TestTask,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is blowing up before we even attempt to load the tasks:

test ".load_all loads all autoloadable Task subclasses within the Maintenance namespace" do
  [
    ...
   ].each do |constant|
    module_parent = constant.module_parent
    module_parent
      .expects(:const_get)
      .with(constant.name.demodulize.to_sym)
      .returns(constant)
  end

  puts "load all tasks!!!"

  DefaultTaskLoader.load_all
end
➜  maintenance_tasks git:(sto/configurable-task-loader) ✗ BUNDLE_GEMFILE=gemfiles/rails_6_1.gemfile be rake test TEST=test/models/maintenance_tasks/default_task_loader_test.rb
Run options: --seed 29484

# Running:

F

Failure:
MaintenanceTasks::DefaultTaskLoaderTest#test_.load_all_loads_all_autoloadable_Task_subclasses_within_the_Maintenance_namespace [/Users/adriannachang/.gem/ruby/3.2.1/gems/activesupport-6.1.7.6/lib/active_support/inflector/methods.rb:287]:
unexpected invocation: Maintenance.const_get("Nested")
unsatisfied expectations:
- expected exactly once, invoked never: Maintenance.const_get(:Nested)
- expected exactly once, invoked never: Maintenance.const_get(:ImportPostsTask)
- expected exactly once, invoked never: Maintenance.const_get(:ErrorTask)
- expected exactly once, invoked never: Maintenance.const_get(:EnqueueErrorTask)
- expected exactly once, invoked never: Maintenance.const_get(:CancelledEnqueueTask)
- expected exactly once, invoked never: Maintenance.const_get(:CallbackTestTask)
- expected exactly once, invoked never: Maintenance.const_get(:BatchImportPostsTask)



rails test test/models/maintenance_tasks/default_task_loader_test.rb:9

I think the issue is that, in setting up the expectations for the nested task constants (Maintenance::Nested::NestedMore, Maintenance::Nested::NestedMore::NestedMoreTask, etc.), we end up performing a const_get on Maintenance::Nested when we do constant.module_parent, which triggers the expectation on Maintenance::Nested that's just been set.

Copy link
Contributor Author

@davidstosik davidstosik Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Note it only blows up on Rails 6.1. :)
I managed to fix it (for Rails 6.1) by changing the order, but obviously it's gonna start failing in Rails 7.
Ideally I'd want to write expectations that do not care about the order in which const_get calls happen, but I haven't figured it out...

I think what I really need is something Mocha doesn't provide: spies.
I want to spy on const_get calls without changing their original behaviours.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right -- sorry I thought the issue was more complex than it was, but it really is just whether the Maintenance::Nested expectation is set before or after the other Nested constant expectations...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @davidstosik if I change the order to what you proposed in #867 (comment), it passes against all the Rails versions for me? Were you seeing failures locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrianna Huh, wow, this is weird, you seem to be right. Although both orders work with Rails 7, it seems only a specific order works with Rails 6.1?

For fun (and because I hadn't imagined the right order would work for all supported Rails versions), I've also implemented a spy. Here it is, just to show what it could look like:

# frozen_string_literal: true

require "test_helper"

module MaintenanceTasks
  class DefaultTaskLoaderTest < ActiveSupport::TestCase
    class ConstGetSpy < SimpleDelegator
      cattr_reader :calls, default: Hash.new { |h, k| h[k] = [] }

      def const_get(name)
        calls[spied_module] << name

        return_value = super

        if return_value.is_a?(Module)
          self.class.new(return_value)
        else
          return_value
        end
      end

      def instance_of?(klass)
        spied_module.instance_of?(klass)
      end

      private

      def spied_module
        __getobj__
      end
    end

    SPIED_MAINTENANCE_TASKS_MODULE = ConstGetSpy.new(Maintenance)

    test ".load_all loads all autoloadable Task subclasses within the Maintenance namespace" do
      original_tasks_module = MaintenanceTasks.tasks_module
      MaintenanceTasks.tasks_module = "MaintenanceTasks::DefaultTaskLoaderTest::SPIED_MAINTENANCE_TASKS_MODULE"

      expected_calls_to_const_get = {
        Maintenance => [
          :BatchImportPostsTask,
          :CallbackTestTask,
          :CancelledEnqueueTask,
          :EnqueueErrorTask,
          :ErrorTask,
          :ImportPostsTask,
          :Nested,
          :NoCollectionTask,
          :ParamsTask,
          :TestTask,
          :UpdatePostsInBatchesTask,
          :UpdatePostsModulePrependedTask,
          :UpdatePostsTask,
          :UpdatePostsThrottledTask,
        ],
        Maintenance::Nested => [
          :NestedMore,
          :NestedTask,
        ],
        Maintenance::Nested::NestedMore => [
          :NestedMoreTask,
        ]
      }

      DefaultTaskLoader.load_all

      assert_equal(expected_calls_to_const_get, ConstGetSpy.calls.transform_values(&:sort))

      MaintenanceTasks.tasks_module = original_tasks_module
    end
  end
end

(I saw your more general comment about the PR, will reply next. 🙇🏻‍♂️ )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat, thanks for sharing!! ❤️

[
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
8 changes: 8 additions & 0 deletions test/models/maintenance_tasks/task_data_index_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
22 changes: 0 additions & 22 deletions test/models/maintenance_tasks/task_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down