diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 000000000..a6b05bcc0 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,3 @@ +# Team Sol is taking ownership over delayed job infrastructure. +# This will notify us of any requested changes to anything +* @cultureamp/sol diff --git a/.github/pull_request_checklist.md b/.github/pull_request_checklist.md new file mode 100644 index 000000000..f1b2e58a6 --- /dev/null +++ b/.github/pull_request_checklist.md @@ -0,0 +1,27 @@ +# Pull Request Checklist Details + +Here are some useful questions to ask before merging a Pull Request. + +## [Security](https://cultureamp.atlassian.net/wiki/spaces/SEC/pages/963641883/Security+Partnership+Process) +- [Do we need an AppSec review?](https://cultureamp.atlassian.net/servicedesk/customer/portal/5/group/39/create/461) +- Have we modified authentication, authorization, filtering? +- Could there be any PII leaks through tracking or logging? + +## Acceptance Criteria +- What is the acceptance criteria? +- How can we verify the PR has the desired effect? +- Are we sure there are no undesired side effects? + +## Tests +- Do we have appropriate tests? +- How much of the new code do they cover? +- What gaps do you need to call out? + +## Logging +- Are we logging the right things? +- Are we using structured logging? + +## Involve Others +- [Do we need a Change Management Request?](https://cultureamp.atlassian.net/servicedesk/customer/portal/30) +- Does the tech lead need to see this? +- Do other teams need to see this? diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..3f6f6a102 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,35 @@ + + +## Purpose + + +## Context + + +## Changes + + +## Verification + + +## Checklist +- [Security](pull_request_checklist.md#security) +- [Acceptance Criteria](pull_request_checklist.md#acceptance%20criteria) +- [Tests](pull_request_checklist.md#tests) +- [Logging](pull_request_checklist.md#logging) +- [Involve Others](pull_request_checklist.md#involve%20others) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml deleted file mode 100644 index 3e451964d..000000000 --- a/.github/workflows/ci.yml +++ /dev/null @@ -1,92 +0,0 @@ -name: CI - -on: - push: - branches: [ master ] - pull_request: - branches: [ master ] - -jobs: - test: - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - ruby: ['2.7', '3.0', '3.1', jruby-head, ruby-head] - rails_version: - - '6.0.0' - - '6.1.0' - - '7.0.0' - - 'edge' - include: - # Rails 5.2 - - ruby: 2.6 - rails_version: '5.2.0' - - ruby: 2.7 - rails_version: '5.2.0' - - ruby: jruby-9.2 - rails_version: '5.2.0' - - # Ruby 2.6 - - ruby: 2.6 - rails_version: '6.0.0' - - ruby: 2.6 - rails_version: '6.1.0' - - # jruby-9.2 - - ruby: jruby-9.2 - rails_version: '6.0.0' - - ruby: jruby-9.2 - rails_version: '6.1.0' - - # jruby-9.3 - - ruby: jruby-9.3 - rails_version: '7.0.0' - - ruby: jruby-9.3 - rails_version: 'edge' - - # - # The past - # - # EOL Active Record - - ruby: 2.2 - rails_version: '3.2.0' - - ruby: 2.1 - rails_version: '4.1.0' - - ruby: 2.4 - rails_version: '4.2.0' - - ruby: 2.4 - rails_version: '5.0.0' - - ruby: 2.5 - rails_version: '5.1.0' - - continue-on-error: ${{ matrix.rails_version == 'edge' || endsWith(matrix.ruby, 'head') }} - - steps: - - uses: actions/checkout@v2 - - uses: ruby/setup-ruby@v1 - env: - RAILS_VERSION: ${{ matrix.rails_version }} - with: - ruby-version: ${{ matrix.ruby }} - bundler-cache: true # runs 'bundle install' and caches installed gems automatically - - name: Run tests - env: - RAILS_VERSION: ${{ matrix.rails_version }} - run: bundle exec rspec - - name: Coveralls Parallel - uses: coverallsapp/github-action@master - with: - github-token: ${{ secrets.github_token }} - flag-name: run-${{ matrix.ruby }}-${{ matrix.rails_version }} - parallel: true - - finish: - needs: test - runs-on: ubuntu-latest - steps: - - name: Coveralls Finished - uses: coverallsapp/github-action@master - with: - github-token: ${{ secrets.github_token }} - parallel-finished: true diff --git a/.github/workflows/rubocop.yml b/.github/workflows/rubocop.yml index 739473987..88e753644 100644 --- a/.github/workflows/rubocop.yml +++ b/.github/workflows/rubocop.yml @@ -8,10 +8,10 @@ jobs: steps: - uses: actions/checkout@v2 - - name: Set up Ruby 2.7 + - name: Set up Ruby 3.0 uses: ruby/setup-ruby@v1 with: - ruby-version: 2.7 + ruby-version: 3.0 - name: Generate lockfile for cache key run: bundle lock - name: Cache gems diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml new file mode 100644 index 000000000..0aba061cf --- /dev/null +++ b/.github/workflows/ruby.yml @@ -0,0 +1,22 @@ +name: Tests +on: + push: + branches: [ master ] + pull_request: + branches: [ master ] +jobs: + tests: + strategy: + matrix: + version: [2.7.5, 3.0.4] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.version }} + - name: Install dependencies + run: bundle install + - name: Run tests + run: bundle exec rspec diff --git a/.rubocop.yml b/.rubocop.yml index 3b0bd2701..8a6451a9d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -13,10 +13,7 @@ AllCops: - 'Gemfile' - 'Rakefile' - 'delayed_job.gemspec' - TargetRubyVersion: 2.1 - -RedundantBlockCall: - Enabled: false + TargetRubyVersion: 3.0 BlockLength: Enabled: false @@ -133,9 +130,6 @@ Style/SymbolArray: SymbolProc: Enabled: false -TrailingCommaInLiteral: - Enabled: false - TrailingCommaInArguments: Enabled: false @@ -144,3 +138,15 @@ YAMLLoad: ZeroLengthPredicate: Enabled: false + +Style/IfUnlessModifier: + Enabled: false + +Style/CaseLikeIf: + Enabled: false + +Gemspec/RequiredRubyVersion: + Enabled: false + +Migration/DepartmentName: + Enabled: false diff --git a/Gemfile b/Gemfile index c66ff6e7e..7ad99ae91 100644 --- a/Gemfile +++ b/Gemfile @@ -70,7 +70,7 @@ group :test do end group :rubocop do - gem 'rubocop', '>= 0.25', '< 0.49' + gem 'rubocop', '~>1.27.0' end gemspec diff --git a/catalog-info-component.yaml b/catalog-info-component.yaml new file mode 100644 index 000000000..17383051b --- /dev/null +++ b/catalog-info-component.yaml @@ -0,0 +1,19 @@ +apiVersion: backstage.io/v1alpha1 +kind: Component +metadata: + name: delayed_job + description: Forked delayed_job gem with ruby 3 support + links: + - title: Github + url: https://github.com/cultureamp/delayed_job + tags: + - users-internal + - camp-engagement + - data-none + annotations: + github.com/project-slug: cultureamp/delayed_job + github.com/team-slug: cultureamp/sol +spec: + type: library + owner: sol + lifecycle: development \ No newline at end of file diff --git a/catalog-info.yaml b/catalog-info.yaml new file mode 100644 index 000000000..57ffa8245 --- /dev/null +++ b/catalog-info.yaml @@ -0,0 +1,9 @@ +apiVersion: backstage.io/v1alpha1 +kind: Location +metadata: + name: delayed_job-location + tags: + - camp-engagement +spec: + targets: + - ./catalog-info-component.yaml # this is your main component file \ No newline at end of file diff --git a/delayed_job.gemspec b/delayed_job.gemspec index ee290405e..1c65aa125 100644 --- a/delayed_job.gemspec +++ b/delayed_job.gemspec @@ -15,7 +15,7 @@ Gem::Specification.new do |spec| spec.test_files = Dir.glob('spec/**/*') spec.version = '4.1.10' spec.metadata = { - 'changelog_uri' => 'https://github.com/collectiveidea/delayed_job/blob/master/CHANGELOG.md', + 'changelog_uri' => 'https://github.com/collectiveidea/delayed_job/blob/master/CHANGELOG.md', 'bug_tracker_uri' => 'https://github.com/collectiveidea/delayed_job/issues', 'source_code_uri' => 'https://github.com/collectiveidea/delayed_job' } diff --git a/lib/delayed/backend/shared_spec.rb b/lib/delayed/backend/shared_spec.rb index 39f497670..e3acd1136 100644 --- a/lib/delayed/backend/shared_spec.rb +++ b/lib/delayed/backend/shared_spec.rb @@ -1,6 +1,7 @@ require File.expand_path('../../../../spec/sample_jobs', __FILE__) require 'active_support/core_ext/numeric/time' +require 'active_support/core_ext/kernel/reporting' shared_examples_for 'a delayed_job backend' do let(:worker) { Delayed::Worker.new } diff --git a/lib/delayed/message_sending.rb b/lib/delayed/message_sending.rb index a2808fd39..34cc4f6ee 100644 --- a/lib/delayed/message_sending.rb +++ b/lib/delayed/message_sending.rb @@ -7,8 +7,8 @@ def initialize(payload_class, target, options) end # rubocop:disable MethodMissing - def method_missing(method, *args) - Job.enqueue({:payload_object => @payload_class.new(@target, method.to_sym, args)}.merge(@options)) + def method_missing(method, *args, **kwargs) + Job.enqueue({:payload_object => @payload_class.new(@target, method.to_sym, args, kwargs)}.merge(@options)) end # rubocop:enable MethodMissing end diff --git a/lib/delayed/performable_mailer.rb b/lib/delayed/performable_mailer.rb index 8535c452d..7c5d6d7b0 100644 --- a/lib/delayed/performable_mailer.rb +++ b/lib/delayed/performable_mailer.rb @@ -3,7 +3,7 @@ module Delayed class PerformableMailer < PerformableMethod def perform - mailer = object.send(method_name, *args) + mailer = super mailer.respond_to?(:deliver_now) ? mailer.deliver_now : mailer.deliver end end diff --git a/lib/delayed/performable_method.rb b/lib/delayed/performable_method.rb index 96a28b056..528f45680 100644 --- a/lib/delayed/performable_method.rb +++ b/lib/delayed/performable_method.rb @@ -1,8 +1,9 @@ module Delayed class PerformableMethod attr_accessor :object, :method_name, :args + attr_writer :kwargs - def initialize(object, method_name, args) + def initialize(object, method_name, args, kwargs = {}) raise NoMethodError, "undefined method `#{method_name}' for #{object.inspect}" unless object.respond_to?(method_name, true) if object.respond_to?(:persisted?) && !object.persisted? @@ -11,6 +12,7 @@ def initialize(object, method_name, args) self.object = object self.args = args + self.kwargs = kwargs self.method_name = method_name.to_sym end @@ -22,22 +24,51 @@ def display_name end end + def kwargs + # Default to a hash so that we can handle deserializing jobs that were + # created before kwargs was available. + @kwargs || {} + end + def perform - object.send(method_name, *args) if object + if RUBY_VERSION >= '3.0' + ruby3_perform + else + ruby2_perform + end end def method(sym) object.method(sym) end - # rubocop:disable MethodMissing - def method_missing(symbol, *args) - object.send(symbol, *args) - end + definition = RUBY_VERSION >= '2.7' ? '...' : '*args, &block' + module_eval <<-RUBY, __FILE__, __LINE__ + 1 + def method_missing(#{definition}) + object.send(#{definition}) + end + RUBY # rubocop:enable MethodMissing def respond_to?(symbol, include_private = false) super || object.respond_to?(symbol, include_private) end + + private + + def ruby2_perform + # In ruby 2, rely on the implicit conversion from a hash to kwargs + return unless object + if kwargs.present? + object.send(method_name, *args, kwargs) + else + object.send(method_name, *args) + end + end + + def ruby3_perform + # In ruby 3 we need to explicitly separate regular args from the keyword-args. + object.send(method_name, *args, **kwargs) if object + end end end diff --git a/lib/delayed/psych_ext.rb b/lib/delayed/psych_ext.rb index 00350a453..049a18d12 100644 --- a/lib/delayed/psych_ext.rb +++ b/lib/delayed/psych_ext.rb @@ -5,7 +5,8 @@ def encode_with(coder) coder.map = { 'object' => object, 'method_name' => method_name, - 'args' => args + 'args' => args, + 'kwargs' => kwargs } end end diff --git a/lib/delayed/worker.rb b/lib/delayed/worker.rb index 1a352f775..846bba749 100644 --- a/lib/delayed/worker.rb +++ b/lib/delayed/worker.rb @@ -4,6 +4,7 @@ require 'active_support/core_ext/class/attribute_accessors' require 'active_support/hash_with_indifferent_access' require 'active_support/core_ext/hash/indifferent_access' +require 'active_support/core_ext/kernel/reporting' require 'logger' require 'benchmark' diff --git a/lib/generators/delayed_job/templates/script b/lib/generators/delayed_job/templates/script old mode 100644 new mode 100755 diff --git a/spec/message_sending_spec.rb b/spec/message_sending_spec.rb index ca58edcc1..281f8b287 100644 --- a/spec/message_sending_spec.rb +++ b/spec/message_sending_spec.rb @@ -64,12 +64,17 @@ def spin; end context 'delay' do class FairyTail - attr_accessor :happy_ending + attr_accessor :happy_ending, :ogre, :dead def self.princesses; end def tell @happy_ending = true end + + def defeat(ogre_params, dead: true) + @ogre = ogre_params + @dead = dead + end end after do @@ -143,5 +148,14 @@ def tell end.to change(fairy_tail, :happy_ending).from(nil).to(true) end.not_to(change { Delayed::Job.count }) end + + it 'can handle a mix of params and kwargs' do + Delayed::Worker.delay_jobs = false + fairy_tail = FairyTail.new + expect do + fairy_tail.delay.defeat({:name => 'shrek'}, :dead => false) + end.to change(fairy_tail, :ogre).from(nil).to(:name => 'shrek'). + and(change(fairy_tail, :dead).from(nil).to(false)) + end end end diff --git a/spec/performable_method_spec.rb b/spec/performable_method_spec.rb index a4d700322..bf6f7f42f 100644 --- a/spec/performable_method_spec.rb +++ b/spec/performable_method_spec.rb @@ -1,4 +1,5 @@ require 'helper' +require 'action_controller/metal/strong_parameters' if ActionPack::VERSION::MAJOR >= 5 describe Delayed::PerformableMethod do describe 'perform' do @@ -22,6 +23,117 @@ end end + describe 'perform with hash object' do + before do + @method = Delayed::PerformableMethod.new('foo', :count, [{:o => true}]) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:count).with({:o => true}) + @method.perform + end + end + + describe 'perform with hash object and kwargs' do + before do + @method = Delayed::PerformableMethod.new('foo', :count, [{:o => true}], :o2 => false) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:count).with({:o => true}, :o2 => false) + @method.perform + end + end + + describe 'perform with many hash objects' do + before do + @method = Delayed::PerformableMethod.new('foo', :count, [{:o => true}, {:o2 => true}]) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:count).with({:o => true}, {:o2 => true}) + @method.perform + end + end + + if ActionPack::VERSION::MAJOR >= 5 + describe 'perform with params object' do + before do + @params = ActionController::Parameters.new(:person => { + :name => 'Francesco', + :age => 22, + :role => 'admin' + }) + + @method = Delayed::PerformableMethod.new('foo', :count, [@params]) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:count).with(@params) + @method.perform + end + end + + describe 'perform with sample object and params object' do + before do + @params = ActionController::Parameters.new(:person => { + :name => 'Francesco', + :age => 22, + :role => 'admin' + }) + + klass = Class.new do + def test_method(_o1, _o2) + true + end + end + + @method = Delayed::PerformableMethod.new(klass.new, :test_method, ['o', @params]) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:test_method).with('o', @params) + @method.perform + end + + it 'calls the method on the object (real)' do + expect(@method.perform).to be true + end + end + end + + describe 'perform with sample object and hash object' do + before do + @method = Delayed::PerformableMethod.new('foo', :count, ['o', {:o => true}]) + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:count).with('o', {:o => true}) + @method.perform + end + end + + describe 'perform with hash to named parameters' do + before do + klass = Class.new do + def test_method(name:, any:) + true if name && any + end + end + + @method = Delayed::PerformableMethod.new(klass.new, :test_method, [], :name => 'name', :any => 'any') + end + + it 'calls the method on the object' do + expect(@method.object).to receive(:test_method).with(:name => 'name', :any => 'any') + @method.perform + end + + it 'calls the method on the object (real)' do + expect(@method.perform).to be true + end + end + it "raises a NoMethodError if target method doesn't exist" do expect do Delayed::PerformableMethod.new(Object, :method_that_does_not_exist, [])