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

Support for anonymous model classes #1172

Closed
kalsan opened this issue Jun 22, 2020 · 5 comments
Closed

Support for anonymous model classes #1172

kalsan opened this issue Jun 22, 2020 · 5 comments

Comments

@kalsan
Copy link
Contributor

kalsan commented Jun 22, 2020

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 added a commit that referenced this issue Jun 24, 2020
This ensures that if IndexSet is customised, that customised class gets this method called as well, rather than the original implementation.

Of course, if `reference_name` isn’t customised, then it’s the original implementation that does get invoked, and that’s fine. This just keeps things consistent - always use whatever configuration’s index_set_class is set to.

This was prompted by thinking through how to support anonymous models as discussed in #1172 by @kalsan.
@pat
Copy link
Owner

pat commented Jun 24, 2020

So, I've just pushed commit 16f2e66 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 added a commit that referenced this issue Jun 24, 2020
This gives people the option of changing the data of the default templates fields and attributes. I suspect the cases for this are very rare, but one such situation is when anonymous models are in play, as requested by @kalsan in #1172.
@pat
Copy link
Owner

pat commented Jun 24, 2020

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

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
Copy link
Contributor Author

kalsan commented Jun 25, 2020

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
Copy link
Owner

pat commented Jul 20, 2020

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

@pat pat closed this as completed Jul 20, 2020
@kalsan
Copy link
Contributor Author

kalsan commented Jul 21, 2020

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

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

No branches or pull requests

2 participants