Fix bulk enqueue for unmigrated 'error_event'; add `GoodJob.migrated?…

…` check method; use custom enum implementation (#1011)

.gitignore

@@ -5,6 +5,7 @@ vendor/bundle
 spec/test_app/log/*.log
 spec/test_app/storage/
 spec/test_app/tmp/
+spec/tmp/example_app
 .env
 doc
 .yardoc

README.md

@@ -703,7 +703,9 @@ GoodJob follows semantic versioning, though updates may be encouraged through de
 
 #### Upgrading minor versions
 
-Upgrading between minor versions (e.g. v1.4 to v1.5) should not introduce breaking changes, but can introduce new deprecation warnings and database migration notices.
+Upgrading between minor versions (e.g. v1.4 to v1.5) should not introduce breaking changes, but can introduce new deprecation warnings and database migration warnings.
+
+Database migrations introduced in minor releases are _not required_ to be applied until the next major release. If you would like apply newly introduced migrations immediately, assert `GoodJob.migrated?` in your application's test suite.
 
 To perform upgrades to the GoodJob database tables:
 

app/models/concerns/good_job/error_events.rb

@@ -14,15 +14,32 @@ module ErrorEvents
       ERROR_EVENT_DISCARDED = 'discarded',
     ].freeze
 
-    included do
-      enum error_event: {
-        ERROR_EVENT_INTERRUPTED => 0,
-        ERROR_EVENT_UNHANDLED => 1,
-        ERROR_EVENT_HANDLED => 2,
-        ERROR_EVENT_RETRIED => 3,
-        ERROR_EVENT_RETRY_STOPPED => 4,
-        ERROR_EVENT_DISCARDED => 5,
-      }.freeze, _prefix: :error_event
+    ERROR_EVENT_ENUMS = {
+      ERROR_EVENT_INTERRUPTED => 0,
+      ERROR_EVENT_UNHANDLED => 1,
+      ERROR_EVENT_HANDLED => 2,
+      ERROR_EVENT_RETRIED => 3,
+      ERROR_EVENT_RETRY_STOPPED => 4,
+      ERROR_EVENT_DISCARDED => 5,
+    }.freeze
+
+    # TODO: GoodJob v4 can make this an `enum` once migrations are guaranteed.
+    def error_event
+      return unless self.class.columns_hash['error_event']
+
+      enum = super
+      return unless enum
+
+      ERROR_EVENT_ENUMS.key(enum)
+    end
+
+    def error_event=(event)
+      return unless self.class.columns_hash['error_event']
+
+      enum = ERROR_EVENT_ENUMS[event]
+      raise(ArgumentError, "Invalid error_event: #{event}") if event && !enum
+
+      super(enum)
     end
   end
 end

app/models/good_job/execution.rb

@@ -384,7 +384,7 @@ def perform
                 error: interrupt_error_string,
                 finished_at: Time.current,
               }
-              discrete_execution_attrs[:error_event] = GoodJob::DiscreteExecution.error_events[GoodJob::DiscreteExecution::ERROR_EVENT_INTERRUPTED] if self.class.error_event_migrated?
+              discrete_execution_attrs[:error_event] = GoodJob::ErrorEvents::ERROR_EVENT_ENUMS[GoodJob::ErrorEvents::ERROR_EVENT_INTERRUPTED] if self.class.error_event_migrated?
               discrete_executions.where(finished_at: nil).where.not(performed_at: nil).update_all(discrete_execution_attrs) # rubocop:disable Rails/SkipsModelValidations
             end
           end

app/models/good_job/job.rb

@@ -228,9 +228,12 @@ def discard_job(message)
 
         update_execution = proc do
           execution.update(
-            finished_at: Time.current,
-            error: GoodJob::Execution.format_error(job_error),
-            error_event: :discarded
+            {
+              finished_at: Time.current,
+              error: GoodJob::Execution.format_error(job_error),
+            }.tap do |attrs|
+              attrs[:error_event] = ERROR_EVENT_DISCARDED if self.class.error_event_migrated?
+            end
           )
         end
 

lib/good_job.rb

@@ -246,5 +246,14 @@ def self.deprecator
     end
   end
 
+  # Whether all GoodJob migrations have been applied.
+  # For use in tests/CI to validate GoodJob is up-to-date.
+  # @return [Boolean]
+  def self.migrated?
+    # Always update with the most recent migration check
+    GoodJob::Execution.reset_column_information
+    GoodJob::Execution.error_event_migrated?
+  end
+
   ActiveSupport.run_load_hooks(:good_job, self)
 end

lib/good_job/adapter.rb

@@ -64,7 +64,15 @@ def enqueue_all(active_jobs)
 
       inline_executions = []
       GoodJob::Execution.transaction(requires_new: true, joinable: false) do
-        results = GoodJob::Execution.insert_all(executions.map(&:attributes), returning: %w[id active_job_id]) # rubocop:disable Rails/SkipsModelValidations
+        execution_attributes = executions.map do |execution|
+          if GoodJob::Execution.error_event_migrated?
+            execution.attributes
+          else
+            execution.attributes.except('error_event')
+          end
+        end
+
+        results = GoodJob::Execution.insert_all(execution_attributes, returning: %w[id active_job_id]) # rubocop:disable Rails/SkipsModelValidations
 
         job_id_to_provider_job_id = results.each_with_object({}) { |result, hash| hash[result['active_job_id']] = result['id'] }
         active_jobs.each do |active_job|

spec/generators/good_job/update_generator_spec.rb

@@ -23,6 +23,15 @@
       quiet do
         run_in_example_app 'rails db:migrate'
       end
+
+      # Check that `GoodJob.pending_migrations?` is updated
+      expect(GoodJob.migrated?).to be true
+      quiet do
+        run_in_example_app 'rails db:rollback'
+        expect(GoodJob.migrated?).to be false
+        run_in_example_app 'rails db:migrate'
+      end
+      expect(GoodJob.migrated?).to be true
     end
   end
 

spec/integration/complex_jobs_spec.rb

@@ -11,6 +11,11 @@
   end
 
   describe 'Job with retry stopped but no block' do
+    after do
+      # This spec will intentionally raise an error on the thread.
+      THREAD_ERRORS.clear
+    end
+
     specify do
       stub_const "TestJob", (Class.new(ActiveJob::Base) do
         retry_on StandardError, wait: 0, attempts: 1
@@ -37,7 +42,6 @@ def perform
       )
 
       expect(THREAD_ERRORS.size).to eq 1
-      THREAD_ERRORS.clear
     end
   end
 

spec/support/example_app_helper.rb

@@ -53,6 +53,14 @@ def within_example_app
       good_job_processes
       good_job_settings
     ]
+    models = [
+      GoodJob::Job,
+      GoodJob::BatchRecord,
+      GoodJob::Execution,
+      GoodJob::DiscreteExecution,
+      GoodJob::Process,
+      GoodJob::Setting,
+    ]
     quiet do
       tables.each do |table_name|
         ActiveRecord::Migration.drop_table(table_name) if ActiveRecord::Base.connection.table_exists?(table_name)
@@ -61,6 +69,7 @@ def within_example_app
 
       setup_example_app
       run_in_test_app("bin/rails db:environment:set RAILS_ENV=test")
+      models.each(&:reset_column_information)
     end
 
     yield
@@ -74,6 +83,7 @@ def within_example_app
       ActiveRecord::Base.connection.execute("TRUNCATE schema_migrations")
 
       run_in_test_app("bin/rails db:schema:load db:environment:set RAILS_ENV=test")
+      models.each(&:reset_column_information)
     end
   end
 

spec/test_app/Rakefile

@@ -22,3 +22,18 @@ namespace :heroku do
     Rake::Task['db:seed'].invoke
   end
 end
+
+def remove_version_from_schema_rb
+  puts "GoodJob: Sanitizing Rails version from schema.rb...\n"
+  schema_rb = Rails.root.join("db/schema.rb").to_s
+  contents = File.read(schema_rb)
+  contents.sub!(/ActiveRecord::Schema\[\d\.\d\]/, "ActiveRecord::Schema")
+  File.write(schema_rb, contents)
+end
+
+Rake::Task['db:migrate'].enhance do
+  remove_version_from_schema_rb
+end
+Rake::Task['db:rollback'].enhance do
+  remove_version_from_schema_rb
+end