Unknown key :on

[knirirr]

On setting up a new application (Rails 6.0.0-rc1) I can't start/stop/reindex sunspot, even with a simple configuration like this in my model:

searchable do
  text :name 
end

This results in:

ArgumentError: Unknown key: :on. Valid keys are: :if, :unless, :prepend

The location of the problem would appear to be here:

https://github.com/sunspot/sunspot/blob/master/sunspot_rails/lib/sunspot/rails/searchable.rb#L104

Indeed, if I change :on to :if then I can start/reindex etc. The question is whether this is appropriate to do.

[shrirambalakrishnan]

+1. I am facing this issue too.
Please provide a solution to this issue.

[mlh758]

What is the on supposed to be achieving here? It seems like it is trying to say "only trigger the remove from index operation on the destroy lifecycle event" but that seems to defeat the purpose of having a configurable lifecycle event as shown in the code here. The default after_destroy should only happen on the destroy lifecycle event anyway, and it might restrict people from picking other events like after_validation although I'm not sure why someone would do that.

Edit: The on makes total sense if you assume users want to pick a different hook but still in the destroy chain of events.

[mlh758]

That said, the edge guide still shows using the on keyword here to limit to specific lifecycle events. Could this be a bug in the Rails RC?

[tenshiAMD]

@knirirr @mlh758 @shrirambalakrishnan

This might be the fix for this issue #940.

[mlh758]

That PR trades on for if which fulfill different purposes https://edgeguides.rubyonrails.org/active_record_callbacks.html#conditional-callbacks

The on option is also still documented in the edge guides. I haven't had a chance look into why it wouldn't be working now.

[tenshiAMD]

@mlh758, it seems to work just fine, I made a monkey patch for this issue as a workaround based on this PR #940

config/initializers/sunspot_monkey_patch.rb

Sunspot::Rails::Searchable::ActsAsMethods.module_eval do
  def searchable(options = {}, &block)
    Sunspot.setup(self, &block)

    if searchable?
      sunspot_options[:include].concat(Sunspot::Util::Array(options[:include]))
    else
      extend Sunspot::Rails::Searchable::ClassMethods
      include  Sunspot::Rails::Searchable::InstanceMethods

      class_attribute :sunspot_options

      unless options[:auto_index] == false
        before_save :mark_for_auto_indexing_or_removal

        __send__ Sunspot::Rails.configuration.auto_index_callback,
                 :perform_index_tasks,
                 :if => :persisted?
      end

      unless options[:auto_remove] == false
        if ::Rails::VERSION::MAJOR >= 6
          __send__ Sunspot::Rails.configuration.auto_remove_callback,
                   proc { |searchable| searchable.remove_from_index },
                   :if => :destroy
        else
          __send__ Sunspot::Rails.configuration.auto_remove_callback,
                   proc { |searchable| searchable.remove_from_index },
                   :on => :destroy
        end
      end
      options[:include] = Sunspot::Util::Array(options[:include])

      self.sunspot_options = options
    end
  end
end

[mlh758]

They don't do the same thing though.

The if filter executes a predicate method and then continues with the operation if that method returns something truthy. In your monkey patch that means it calls destroy on the record. The on filter takes a list of lifecycle events that the callback should be executed on. validate could get called on create and update for example, maybe you only want your hook to be called on create.

[mlh758]

I created a Rails 6.0.0 RC1 project, made a Widget model like this:

class Widget < ApplicationRecord
  before_validation :normalize_name, on: :create

  def normalize_name
    self.name = name.underscore.downcase
  end
end

Then ran:

w = Widget.new
w.name = 'SomeName'
w.save
w.name # prints 'some_name'
w.name = 'NewName'
w.save
w.name # prints 'NewName'

If I change that line to before_validation :normalize_name, if: :create

Then try the above sequence of commands I get:

NoMethodError (undefined method `create' for #<Widget id: nil, name: "SomeName">)

This also shows that on should be a valid operation on callbacks still in Rails 6.

[mlh758]

Found this mentioned in rails/rails#35329

Introduced in rails/rails#30919

I don't see anything obviously wrong with how we're using it here though:

class Widget < ApplicationRecord
  after_commit :log_name, on: :destroy

  def log_name
    puts name
  end
end

does what you would expect and only logs the name when a Widget is destroyed. No errors.

[mlh758]

Have you overridden the default auto_remove_callback to a after_save or something? That would trigger this error.

Edit: I'm dumb, our default is after_destroy not after_commit. That's probably what we need to change.

Alternatively, could we remove the on and leave it as after_destroy by default. Overriding that option would become riskier without the filter though. I also wonder if after_commit would have any performance implication with large transactions.