pat / thinking-sphinx

Sphinx/Manticore plugin for ActiveRecord/Rails
http://freelancing-gods.com/thinking-sphinx
MIT License
1.63k stars 468 forks source link

Support for anonymous model classes #1172

Closed kalsan closed 4 years ago

kalsan commented 4 years ago

Fore more fine-grained model control, I instantiate the model in an anonymous class that inherits from the original model class. However thinking-sphinx is unable to handle this situation since model.class is nil.

A concrete error scenario is using thinking-sphinx together with the gem RailsOps (https://github.com/sitrox/rails_ops).

To support this, two changes are necessary to thinking-sphinx. They can be monkey-patched in the the gem as follows:

# EDIT: DO NOT USE, SEE BELOW
# Monkey patch no. 1 for supporting anonymous classes. Otherwise incompatible with rails_ops
class ThinkingSphinx::IndexSet
  def self.reference_name(klass)
    @cached_results ||= {}
    result = @cached_results[klass.model_name.name] ||= klass.model_name.name.underscore.to_sym
    return result
  end
end

# Monkey patch no. 2 for supporting anonymous classes. Otherwise incompatible with rails_ops
class ThinkingSphinx::RealTime::Translator
  def result
    @result ||= owner.try(name)
    @result ||= owner.model_name.name if name == :name && stack == [:class]
    return @result
  end
end

It would be great if this or a better solution could be implemented in the upcoming release of thinking-sphinx. It should be no trouble to implement it and the change would allow support for patterns with anonymous classes.

Edit: Monkey patch 1 is flawed, use this instead:

class ThinkingSphinx::IndexSet
  def self.reference_name(klass)
    @cached_results ||= {}
    if klass.name
      @cached_results[klass.name] = klass.name.underscore.to_sym
      return @cached_results[klass.name]
    else
      @cached_results[klass.model_name.name] ||= klass.model_name.name.underscore.to_sym
      return @cached_results[klass.model_name.name]
    end
  end
end
pat commented 4 years ago

So, I've just pushed commit 16f2e66e8611f29a824ac8555a16ecdc1d42116d that deals with the first monkey-patch to some extent… custom IndexSet subclasses have been allowed for some time, but reference_name was only being invoked through the original class. I've changed that, so you'll be able to do the following:

# this can all go within an initialiser if you like…

class AnonymousModelsIndexSet < ThinkingSphinx::IndexSet
  def self.reference_name(klass)
    # implement this however you like
  end
end

Rails.application.config.to_prepare do
  ThinkingSphinx::Configuration.instance.index_set_class = AnonymousModelsIndexSet
end

I've not yet figured out something for the second monkey-patch though… there's no existing customisation around that. I don't want to add support for rails_ops itself, because that ties TS to rails_ops internals. So, I'll think a bit further and see if I can find a neater way to allow further customisation to happen.

pat commented 4 years ago

Okay, I think I've got an approach for the second monkey-patch, as per f33642dd5eb2d3d7cb3fab1969356e5cfbe2565e:

Instead of changing the Translator, you can instead replace the sphinx_internal_class attribute by defining it again in your index definition, pointing to a method that'll return what you prefer:

# in the index definition:
has custom_class_column, as: :sphinx_internal_class, type: :string, :facet => true

# in the model:
def custom_class_column
  self.class.model_name.name
end

Both of these changes are in the develop branch currently, and will be part of the forthcoming v5.0.0 release (hopefully in the next week or so).

kalsan commented 4 years ago

Hi Pat

Thanks for your quick reaction! The adjustments sound great :-) I'll upgrade to 5.0.0 as soon as it's out. Looking forward to that!

Cheers Kalsan

pat commented 4 years ago

Hi Kalsan - I just wanted to let you know that the newly released v5.0.0 release of Thinking Sphinx includes the two changes discussed here: reference_name on custom IndexSet classes is respected, and fields/attributes can be overridden in index definitions.

There's also a significant change with needing to specify callbacks - though this is more of a big deal for those not using real-time indices: https://github.com/pat/thinking-sphinx/releases/tag/v5.0.0

kalsan commented 4 years ago

Hi Pat

This is awesome, thanks a lot! The patch seems no longer required. I still have another question about callbacks which I will post separately.

Thank you! Kalsan