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

Error when providing a class argument to ActiveJob for classes that implement permitted? #48561

Closed
adnanrh opened this issue Jun 22, 2023 · 3 comments · Fixed by #48566
Closed

Comments

@adnanrh
Copy link

adnanrh commented Jun 22, 2023

Steps to reproduce

  1. Write a class MyClass that implements a class method named permitted?
  2. Write a job BuggyJob with a perform method that accepts a class as a parameter
  3. Enqueue a BuggyJob job with MyClass as an argument: BuggyJob.perform_later(MyClass)

The issue seems to be here: https://github.com/rails/rails/blob/main/activejob/lib/active_job/arguments.rb#L93
It seems in the switch statement to choose the correct serializer for the argument, we attempt to call serialize_indifferent_hash on argument.to_h if it responds to permitted?. I suppose the assumption here is that if the argument responds to permitted?, it should be an instance of ActionController::Parameters? In our app, we have a module that defines a class method named permitted? for some other business logic and it's used in a few classes.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activejob", "~> 7.0.0"
end

require "minitest/autorun"
require "active_job"

class MyClass
  def self.permitted?
    # noop
  end
end

class BuggyJob < ActiveJob::Base
  def perform(klass)
    puts "performed"
  end
end

class BuggyJobTest < ActiveJob::TestCase
  def test_stuff
    error = assert_raises(NoMethodError) { BuggyJob.perform_later(MyClass) }
    assert_match /undefined method `to_h' for MyClass:Class/, error.message
  end
end

Expected behavior

A BuggyJob job should successfully be queued, with the MyClass argument serialized using ActiveJob::Serializers::ModuleSerializer.

Actual behavior

A NoMethodError is raised:

Failed enqueuing BuggyJob to DelayedJob(default): NoMethodError (undefined method `to_h' for MyClass:Class
Did you mean?  to_s)
.../ruby/3.0.0/gems/activejob-6.1.7.3/lib/active_job/arguments.rb:120:in `serialize_argument': undefined method `to_h' for MyClass:Class (NoMethodError)
Did you mean?  to_s

System configuration

Rails version: Tested on 6.1.x, 7.0.x

Ruby version: Tested on 3.0.4

@adnanrh adnanrh changed the title Error when providing class arguments to ActiveJob for classes that implement "permitted?" Error when providing a class argument to ActiveJob for classes that implement "permitted?" Jun 22, 2023
@adnanrh adnanrh changed the title Error when providing a class argument to ActiveJob for classes that implement "permitted?" Error when providing a class argument to ActiveJob for classes that implement permitted? Jun 22, 2023
@abaldwin88
Copy link
Contributor

Removing these two lines allows BuggyJob.perform_later(MyClass) to be called in the test case without an error being raised

when -> (arg) { arg.respond_to?(:permitted?) }
serialize_indifferent_hash(argument.to_h)

@sampatbadhe
Copy link
Contributor

How about checking argument is responding to to_h along with permitted?

when -> (arg) { arg.respond_to?(:permitted?) }

-        when -> (arg) { arg.respond_to?(:permitted?) }
+        when -> (arg) { arg.respond_to?(:permitted?) && arg.respond_to?(:to_h) }

@abaldwin88
Copy link
Contributor

It'd be nice to explicitly check whether the class is ActionController::Parameters ?

The drawback with that approach though is ActiveJob would become tightly coupled to ActionController. We'd need to require "action_controller/metal/strong_parameters".

Adding to_h seems like the best course. I'll open a PR and add you as a co-author @sampatbadhe

abaldwin88 added a commit to abaldwin88/rails that referenced this issue Jun 23, 2023
Resolves rails#48561
Co-authored-by: Sampat Badhe <sampatbadhe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants