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

Use parent's index_name and document_type from child models #344

Closed
wants to merge 1 commit into from
Closed

Use parent's index_name and document_type from child models #344

wants to merge 1 commit into from

Conversation

yuku
Copy link

@yuku yuku commented Mar 6, 2015

Suppose there are three models:

class Company < ActiveRecord::Base
  include Elasticsearch::Model
end
class TechCompany < Company; end
class TradingCompany < Company; end

then previously they respond to "document_type" as follows:

Company.document_type        #=> 'company'
TechCompany.document_type    #=> 'tech_company'
TradingCompany.document_type #=> 'trading_company'

This commit changes them as:

Company.document_type        #=> 'company'
TechCompany.document_type    #=> 'company'
TradingCompany.document_type #=> 'company'

So, we can use search scope like this:

Company.search('*')  # Returns both TechCompany and TradingCompany

# Make `TechCompany.search` automatically add a filter such as
# `{ must: { term: { type: 'TechCompany' } } }`
TechCompany.search('*')  # Returns TechCompany only

@karmi
Copy link
Contributor

karmi commented Mar 6, 2015

Interesting idea! Couple of notes, I'm travelling these days: we definitely need unit tests for this, and this is probably related to all the "single-table inheritance" (STI) issues being reported for Rails integration. Can you look at them? It would be great to have integration tests covering these...

@yuku yuku changed the title Use parent's document_type from child models Use parent's index_name and document_type from child models Mar 8, 2015
@karmi
Copy link
Contributor

karmi commented Apr 8, 2015

Hello, I'll need some time to process the patch -- there are couple of things which stick out, for instance, we deliberately don't add active_record as a dependency to the main .gemspec, but keep it in separate Gemfiles, etc.

Suppose there are three models:

    class Company < ActiveRecord::Base
      include Elasticsearch::Model
    end
    class TechCompany < Company; end
    class TradingCompany < Company; end

then previously they respond to "document_type" as follows:

    Company.document_type        #=> 'company'
    TechCompany.document_type    #=> 'tech_company'
    TradingCompany.document_type #=> 'trading_company'

This commit changes them as:

    Company.document_type        #=> 'company'
    TechCompany.document_type    #=> 'company'
    TradingCompany.document_type #=> 'company'

They respond to "index_name" in the same manner.
@yuku
Copy link
Author

yuku commented Apr 9, 2015

Hi, sorry for my late reply.

I miss understood the code base - I thought elasticsearch-model is used by only elasticsearch-rails. elasticsearch-persistence also uses it!

we deliberately don't add active_record as a dependency to the main .gemspec, but keep it in separate Gemfiles

As I noted above, I added the dependency due to my wrong understanding. I've just removed it and updated the patch.

I want to write tests for this as soon as possible. (I'm busy now to write an article for Japanese famous tech magazine about "How to use elasticsearch in Ruby". It refers both this repository and elasticsearch-ruby 😏 )

@karmi
Copy link
Contributor

karmi commented Apr 9, 2015

@yuku-t Great news about the article! :) Take your time, there's no rush. Just ping me please when you have something ready, and ping me again if I get sidetracked and fail to respond.

BTW get in touch with @johtani for community support in Japan!

@karmi karmi added the waiting label Apr 28, 2015
@karmi
Copy link
Contributor

karmi commented May 22, 2015

Hi @yuku-t, did you had some time to return to this? Can you also please review #144 and #333?

@yuku
Copy link
Author

yuku commented May 25, 2015

Hi, I finished the work last weekend. I check the failing tests and referred issues.

@Tonkpils
Copy link

I'm assuming these two are related? #332

@Tonkpils
Copy link

So, I tested out this change set and the issue with this approach is that it ignores the index_name and document_type of the base_class if it's set. It will always use the model name instead of the proper base_class index/document.

karmi pushed a commit that referenced this pull request Jun 28, 2016
This patch adds support for inheriting index_name and document_type on an opt-in basis:

    Elasticsearch::Model.inheritance_enabled = true

    class Animal < ActiveRecord::Base
      document_type 'mammal'
      index_name 'mammals'
    end

    class Dog < Animal
    end

    Animal.document_type   # 'mammal'
    Animal.index_name      # 'mammals'
    Dog.document_type      # 'mammal'
    Dog.index_name         # 'mammals'

Closes #332

Related: #28, #92, #170, #344
@yuku yuku closed this May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants