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

Delegate indexed_name to setup of the joined model class #966

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

heaven
Copy link
Contributor

@heaven heaven commented Feb 15, 2020

This should be way more reliable than trying to guess the name, which can hardly work if the original field was configured with the :as option.

…re reliable and doesn't require the :as option when adding a joined field config.
@serggl
Copy link
Collaborator

serggl commented Feb 16, 2020

is there away to have a spec for this? it's kind of hard to understand the changes made here

@heaven
Copy link
Contributor Author

heaven commented Feb 16, 2020

The change is that previously I tried to build the indexed_name for joined fields like this was done for regular fields, which didn't work well in case when the field from the joined model was defined with :as option.

Example:

class User < ActiveRecord::Base
  has_one :profile

  searchable do
    join(:name,
      :prefix => :profile,
      :target => 'Profile',
      :type => :text,
      :join => { :from => :member_id, :to => :id })
  end
end

class Profile < ActiveRecord::Base
  belongs_to :user

  searchable do
    text :name, :as => :name_custom_ngram_filter_factory
  end
end

Previously, when searching User by profile_name field Sunspot would try using the name_text field name while it should be name_custom_ngram_filter_factory.

Since we don't know all the options original field was created with when defining joined fields, it's easier to find an already configured field in Profile's setup and get the indexed_name from it.

I'll take a look at the test, probably I can add additional scenarios, like the one described above.

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 this pull request may close these issues.

None yet

2 participants