Add rails g good_job:update command to add idempotent migration fil…

…es, including active_job_id, concurrency_key, cron_key columns

.github/workflows/test.yml

@@ -8,7 +8,7 @@ jobs:
   lint:
     name: Lint
     runs-on: ubuntu-latest
-    timeout-minutes: 15
+    timeout-minutes: 20
     env:
       BUNDLE_JOBS: 4
       BUNDLE_RETRY: 3

.rubocop.yml

@@ -56,6 +56,10 @@ Rails/Inquiry:
   Exclude:
     - spec/**/*
 
+Rails/SkipsModelValidations:
+  Exclude:
+    - lib/generators/good_job/templates/update/migrations/**/*
+
 RSpec/AnyInstance:
   Enabled: false
 

README.md

@@ -38,6 +38,7 @@ For more of the story of GoodJob, read the [introductory blog post](https://isla
     - [Configuration options](#configuration-options)
     - [Global options](#global-options)
     - [Dashboard](#dashboard)
+    - [Updating](#updating)
 - [Go deeper](#go-deeper)
     - [Exceptions, retries, and reliability](#exceptions-retries-and-reliability)
         - [Exceptions](#exceptions)
@@ -318,6 +319,22 @@ GoodJob includes a Dashboard as a mountable `Rails::Engine`.
     end
     ```
 
+### Updating
+
+GoodJob follows semantic versioning, though updates may be encouraged through deprecation warnings in minor versions.
+
+To apply updates:
+
+```bash
+bin/rails g good_job:update
+```
+
+...and run the resulting migration:
+
+```bash
+bin/rails db:migrate
+```
+
 ## Go deeper
 
 ### Exceptions, retries, and reliability

lib/generators/good_job/install_generator.rb

@@ -1,13 +1,9 @@
 require 'rails/generators'
 require 'rails/generators/active_record'
-
 module GoodJob
   #
-  # Implements the Rails generator used for setting up GoodJob in a Rails
-  # application. Run it with +bin/rails g good_job:install+ in your console.
-  #
-  # This generator is primarily dedicated to stubbing out a migration that adds
-  # a table to hold GoodJob's queued jobs in your database.
+  # Rails generator used for setting up GoodJob in a Rails application.
+  # Run it with +bin/rails g good_job:install+ in your console.
   #
   class InstallGenerator < Rails::Generators::Base
     include Rails::Generators::Migration
@@ -16,17 +12,11 @@ class << self
       delegate :next_migration_number, to: ActiveRecord::Generators::Base
     end
 
-    source_paths << File.join(File.dirname(__FILE__), "templates")
+    source_paths << File.join(File.dirname(__FILE__), "templates/install")
 
-    # Generates the actual migration file and places it on disk.
+    # Generates monolithic migration file that contains all database changes.
     def create_migration_file
-      migration_template 'migration.rb.erb', 'db/migrate/create_good_jobs.rb', migration_version: migration_version
-    end
-
-    private
-
-    def migration_version
-      "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]"
+      migration_template 'migrations/create_good_jobs.rb.erb', 'db/migrate/create_good_jobs.rb'
     end
   end
 end

lib/generators/good_job/templates/install/migrations/create_good_jobs.rb.erb

@@ -0,0 +1,27 @@
+class CreateGoodJobs < ActiveRecord::Migration[5.2]
+  def change
+    enable_extension 'pgcrypto'
+
+    create_table :good_jobs, id: :uuid do |t|
+      t.text :queue_name
+      t.integer :priority
+      t.jsonb :serialized_params
+      t.timestamp :scheduled_at
+      t.timestamp :performed_at
+      t.timestamp :finished_at
+      t.text :error
+
+      t.timestamps
+
+      t.uuid :active_job_id
+      t.text :concurrency_key
+      t.text :cron_key
+    end
+
+    add_index :good_jobs, :scheduled_at, where: "(finished_at IS NULL)", name: "index_good_jobs_on_scheduled_at"
+    add_index :good_jobs, [:queue_name, :scheduled_at], where: "(finished_at IS NULL)", name: :index_good_jobs_on_queue_name_and_scheduled_at
+    add_index :good_jobs, [:active_job_id, :created_at], name: :index_good_jobs_on_active_job_id_and_created_at
+    add_index :good_jobs, :concurrency_key, where: "(finished_at IS NULL)", name: :index_good_jobs_on_concurrency_key_when_unfinished
+    add_index :good_jobs, [:cron_key, :created_at], name: :index_good_jobs_on_cron_key_and_created_at
+  end
+end

lib/generators/good_job/templates/migration.rb.erb → lib/generators/good_job/templates/update/migrations/01_create_good_jobs.rb

@@ -1,4 +1,4 @@
-class CreateGoodJobs < ActiveRecord::Migration<%= migration_version %>
+class CreateGoodJobs < ActiveRecord::Migration[5.2]
   def change
     enable_extension 'pgcrypto'
 
@@ -14,7 +14,7 @@ def change
       t.timestamps
     end
 
-    add_index :good_jobs, :scheduled_at, where: "(finished_at IS NULL)"
-    add_index :good_jobs, [:queue_name, :scheduled_at], where: "(finished_at IS NULL)"
+    add_index :good_jobs, :scheduled_at, where: "(finished_at IS NULL)", name: "index_good_jobs_on_scheduled_at"
+    add_index :good_jobs, [:queue_name, :scheduled_at], where: "(finished_at IS NULL)", name: :index_good_jobs_on_queue_name_and_scheduled_at
   end
 end

lib/generators/good_job/templates/update/migrations/02_add_active_job_id_concurrency_key_cron_key_to_good_jobs.rb

@@ -0,0 +1,15 @@
+class AddActiveJobIdConcurrencyKeyCronKeyToGoodJobs < ActiveRecord::Migration[5.2]
+  def change
+    reversible do |dir|
+      dir.up do
+        # Ensure this incremental update migration is idempotent
+        # with monolithic install migration.
+        return if connection.column_exists?(:good_jobs, :active_job_id)
+      end
+    end
+
+    add_column :good_jobs, :active_job_id, :uuid
+    add_column :good_jobs, :concurrency_key, :text
+    add_column :good_jobs, :cron_key, :text
+  end
+end

lib/generators/good_job/templates/update/migrations/03_add_active_job_id_index_and_concurrency_key_index_to_good_jobs.rb

@@ -0,0 +1,31 @@
+class AddActiveJobIdIndexAndConcurrencyKeyIndexToGoodJobs < ActiveRecord::Migration[5.2]
+  disable_ddl_transaction!
+
+  UPDATE_BATCH_SIZE = 1_000
+
+  class GoodJobJobs < ActiveRecord::Base
+    self.table_name = "good_jobs"
+  end
+
+  def change
+    reversible do |dir|
+      dir.up do
+        # Ensure this incremental update migration is idempotent
+        # with monolithic install migration.
+        return if connection.index_name_exists?(:good_jobs, :index_good_jobs_on_active_job_id_and_created_at)
+      end
+    end
+
+    add_index :good_jobs, [:active_job_id, :created_at], algorithm: :concurrently, name: :index_good_jobs_on_active_job_id_and_created_at
+    add_index :good_jobs, :concurrency_key, where: "(finished_at IS NULL)", algorithm: :concurrently, name: :index_good_jobs_on_concurrency_key_when_unfinished
+    add_index :good_jobs, [:cron_key, :created_at], algorithm: :concurrently, name: :index_good_jobs_on_cron_key_and_created_at
+
+    reversible do |dir|
+      dir.up do
+        loop do
+          break if GoodJobJobs.where(active_job_id: nil, finished_at: nil).limit(UPDATE_BATCH_SIZE).update_all("active_job_id = (serialized_params->>'job_id')::uuid").zero?
+        end
+      end
+    end
+  end
+end

lib/generators/good_job/update_generator.rb

@@ -0,0 +1,29 @@
+require 'rails/generators'
+require 'rails/generators/active_record'
+
+module GoodJob
+  #
+  # Rails generator used for updating GoodJob in a Rails application.
+  # Run it with +bin/rails g good_job:update+ in your console.
+  #
+  class UpdateGenerator < Rails::Generators::Base
+    include Rails::Generators::Migration
+
+    class << self
+      delegate :next_migration_number, to: ActiveRecord::Generators::Base
+    end
+
+    TEMPLATES = File.join(File.dirname(__FILE__), "templates/update")
+    source_paths << TEMPLATES
+
+    # Generates incremental migration files unless they already exist.
+    # All migrations should be idempotent e.g. +add_index+ is guarded with +if_index_exists?+
+    def update_migration_files
+      migration_templates = Dir.children(File.join(TEMPLATES, 'migrations')).sort
+      migration_templates.each do |template_file|
+        destination_file = template_file.match(/^\d*_(.*\.rb)/)[1] # 01_create_good_jobs.rb.erb => create_good_jobs.rb
+        migration_template "migrations/#{template_file}", "db/migrate/#{destination_file}", skip: true
+      end
+    end
+  end
+end

lib/good_job/job.rb

@@ -196,13 +196,30 @@ def self.next_scheduled_at(after: nil, limit: 100, now_limit: nil)
     #   The new {Job} instance representing the queued ActiveJob job.
     def self.enqueue(active_job, scheduled_at: nil, create_with_advisory_lock: false)
       ActiveSupport::Notifications.instrument("enqueue_job.good_job", { active_job: active_job, scheduled_at: scheduled_at, create_with_advisory_lock: create_with_advisory_lock }) do |instrument_payload|
-        good_job = GoodJob::Job.new(
+        good_job_args = {
           queue_name: active_job.queue_name.presence || DEFAULT_QUEUE_NAME,
           priority: active_job.priority || DEFAULT_PRIORITY,
           serialized_params: active_job.serialize,
           scheduled_at: scheduled_at,
-          create_with_advisory_lock: create_with_advisory_lock
-        )
+          create_with_advisory_lock: create_with_advisory_lock,
+        }
+
+        if column_names.include?('active_job_id')
+          good_job_args[:active_job_id] = active_job.job_id
+        else
+          ActiveSupport::Deprecation.warn(<<~DEPRECATION)
+            GoodJob has pending database migrations. To create the migration files, run:
+
+                rails generate good_job:update
+
+            To apply the migration files, run:
+
+                rails db:migrate
+
+          DEPRECATION
+        end
+
+        good_job = GoodJob::Job.new(**good_job_args)
 
         instrument_payload[:good_job] = good_job
 

spec/lib/generators/good_job/install_generator_spec.rb

@@ -1,17 +1,17 @@
 require 'rails_helper'
 require 'generators/good_job/install_generator'
 
-describe GoodJob::InstallGenerator, type: :generator do
-  after { remove_example_app }
+describe GoodJob::InstallGenerator, type: :generator, skip_if_java: true do
+  around do |example|
+    quiet { setup_example_app }
+    example.run
+    remove_example_app
+  end
 
   it 'creates a migration for good_jobs table' do
-    expect do
-      setup_example_app
-    end.to output(/.*/).to_stderr_from_any_process
-
-    expect do
+    quiet do
       run_in_example_app 'rails g good_job:install'
-    end.to output(/.*/).to_stdout_from_any_process
+    end
 
     expect(Dir.glob("#{example_app_path}/db/migrate/[0-9]*_create_good_jobs.rb")).not_to be_empty
   end

spec/lib/generators/good_job/update_generator_spec.rb

@@ -0,0 +1,52 @@
+require 'rails_helper'
+require 'generators/good_job/update_generator'
+
+describe GoodJob::UpdateGenerator, type: :generator, skip_if_java: true do
+  context 'when running the generator alone' do
+    around do |example|
+      within_example_app do
+        example.run
+      end
+    end
+
+    it 'creates migrations for good_jobs table' do
+      quiet do
+        run_in_example_app 'rails g good_job:update'
+      end
+
+      expect(Dir.glob("#{example_app_path}/db/migrate/[0-9]*_create_good_jobs.rb")).not_to be_empty
+      expect(Dir.glob("#{example_app_path}/db/migrate/[0-9]*_add_active_job_id_index_and_concurrency_key_index_to_good_jobs.rb")).not_to be_empty
+
+      quiet do
+        run_in_example_app 'rails db:migrate'
+      end
+    end
+  end
+
+  it 'produces an idempotentent schema.rb when run with install generator' do
+    install_schema = ""
+    update_after_install_schema = ""
+    only_update_schema = ""
+
+    within_example_app do
+      quiet { run_in_example_app 'rails g good_job:install; rails db:migrate' }
+      install_schema = File.read example_app_path.join('db', 'schema.rb')
+
+      quiet { run_in_example_app 'rails g good_job:update; rails db:migrate' }
+      update_after_install_schema = File.read example_app_path.join('db', 'schema.rb')
+    end
+
+    expect(normalize_schema(update_after_install_schema)).to eq normalize_schema(install_schema)
+
+    within_example_app do
+      quiet { run_in_example_app 'rails g good_job:update; rails db:migrate' }
+      only_update_schema = File.read example_app_path.join('db', 'schema.rb')
+    end
+
+    expect(normalize_schema(only_update_schema)).to eq normalize_schema(install_schema)
+  end
+
+  def normalize_schema(text)
+    text.sub(/ActiveRecord::Schema\.define\(version: ([\d_]*)\)/, 'ActiveRecord::Schema.define(version: SCHEMA_VERSION)')
+  end
+end

spec/lib/good_job/job_spec.rb

@@ -28,6 +28,7 @@ def perform(result_value = nil, raise_error: false)
       end.to change(described_class, :count).by(1)
 
       expect(good_job).to have_attributes(
+        active_job_id: a_kind_of(String),
         serialized_params: a_kind_of(Hash),
         queue_name: 'test',
         priority: 50,

spec/support/example_app_helper.rb

@@ -2,10 +2,11 @@
 
 module ExampleAppHelper
   def setup_example_app
-    root_path = example_app_path.join('..')
+    FileUtils.rm_rf(example_app_path)
 
+    root_path = example_app_path.join('..')
     FileUtils.cd(root_path) do
-      `rails new #{app_name} -d postgresql --no-assets --skip-action-text --skip-action-mailer --skip-action-mailbox --skip-action-cable --skip-sprockets --skip-listen --skip-javascript --skip-turbolinks --skip-system-test --skip-test-unit --skip-bootsnap --skip-spring --skip-active-storage`
+      system("rails new #{app_name} -d postgresql --no-assets --skip-action-text --skip-action-mailer --skip-action-mailbox --skip-action-cable --skip-git --skip-sprockets --skip-listen --skip-javascript --skip-turbolinks --skip-system-test --skip-test-unit --skip-bootsnap --skip-spring --skip-active-storage")
     end
 
     File.open("#{example_app_path}/Gemfile", 'a') do |f|
@@ -19,16 +20,49 @@ def run_in_example_app(*args)
     end
   end
 
+  def run_in_test_app(*args)
+    FileUtils.cd(Rails.root) do
+      system(*args) || raise("Command #{args} failed")
+    end
+  end
+
   def remove_example_app
     FileUtils.rm_rf(example_app_path)
   end
 
+  def within_example_app
+    # Will be running database migrations from the newly created Example App
+    # but doing so in the existing database. This resets the database so that
+    # newly created migrations can be run, then resets it back.
+    #
+    # Ideally this would happen in a different database, but that seemed like
+    # a lot of work to do in Github Actions.
+    quiet do
+      ActiveRecord::Migration.drop_table(:good_jobs) if ActiveRecord::Base.connection.table_exists?(:good_jobs)
+      ActiveRecord::Base.connection.execute("TRUNCATE schema_migrations")
+
+      setup_example_app
+      run_in_test_app("bin/rails db:environment:set RAILS_ENV=test")
+    end
+
+    yield
+
+    quiet do
+      remove_example_app
+
+      ActiveRecord::Migration.drop_table(:good_jobs) if ActiveRecord::Base.connection.table_exists?(:good_jobs)
+      ActiveRecord::Base.connection.execute("TRUNCATE schema_migrations")
+
+      run_in_test_app("bin/rails db:schema:load db:environment:set RAILS_ENV=test")
+    end
+  end
+
   def example_app_path
-    Rails.root.join('..', app_name)
+    Rails.root.join('../tmp', app_name)
   end
 
   def app_name
-    'example_app'
+    'test_app'
   end
 end