brewster / elastictastic

Object-document mapper and lightweight API adapter for ElasticSearch
MIT License
88 stars 13 forks source link

Single Table inheritance not working in models #21

Closed abhasg closed 11 years ago

abhasg commented 11 years ago

Hey I wanted to have single table inheritance properties here as I want to use this gem for syncing my database data with elasticsearch and I found that this tool can help me in doing that very easily. But when I tried this for e.g.

class Basse include Elastictastic::Document

field :name end

class Basse::Alka < Basse field :year end

when I am trying to save Basse::Alka class object it is not allowing me to put value for field 'name'. Showing me error

NoMethodError: undefined method []' for nil:NilClass from /home/abhas/.rvm/gems/ruby-1.9.3-p125/gems/elastictastic-0.10.4/lib/elastictastic/properties.rb:246:inserialize_value'

How should I go about it?

outoftime commented 11 years ago

Elastictastic doesn't support STI because Elastictastic model classes map to ElasticSearch types, and there's no concept of subtypes in ElasticSearch that would map cleanly to subclasses. I'm open to ideas for how to implement this cleanly.

abhasg commented 11 years ago

Thanks @outoftime for replying I am aware that there are no subtypes in elasticsearch. But STI kind of structure will help in saving two different kind of documents with some common functionality in same superclass and subclasses and it will be easy to define multiple mappings for same fields in same inherited kind of fields.

I need it for my project and I just wanted to know if somehow I can achieve that or you can include it into the gem which will me solving the problem. As this kind of inclusion will help others also in future.

outoftime commented 11 years ago

Hi -- no, it's not on the roadmap, I'm sorry. As I said, I would consider a patch. For your use case, I would suggest using an ActiveSupport::Concern mixin to define shared functionality (you could define common fields in the "included" block) or using a simple base class and mixing Elastictastic::Document into subclasses (less easy to define shared fields this way, although you could just define a class method to do it).

In either case, this wouldn't really be single-type inheritance; rather, it would be a way for different types to share some schema/behavior. I think this would make much more sense with how ES types work (you can search multiple types, after all). I'll give some thought to whether Elastictastic can provide more explicit support for that pattern.

abhasg commented 11 years ago

thanks for the idea I was also thinking in same direction initially, but now I have one more restriction that I have to create different index for different models actually. I don't think that can be happen with the current code is it? I will try to customize the code and try to achieve if it can be done lets see.

Pleas let me know if you will be working in this direction.

outoftime commented 11 years ago

Yes, you can do that -- use the #in_index method on scopes, e.g.:

Post.in_index('some_index').new Post.in_index('some_index').query(term: {title: 'Hi'})

abhasg commented 11 years ago

oops yes that one I can do but the problem with it is every place I have to change it in the code when I have to change index or something. Don't you thing there should be a method in class where you can just define the index name and it will be there for that collection of documents? I think that will be better for code management.

outoftime commented 11 years ago

I agree. It would be trivial to patch the ::default_scope method in Elastictastic::BasicDocument to delegate to a ::default_index method which each class singleton can override. Want to take a crack at that and submit a pull request?

outoftime commented 11 years ago

You'd want to grep through the code for Index.default -- I see at least one other place where it's called directly.

abhasg commented 11 years ago

ya I tried but it was giving me some errors so for myself I made some changes in the other place you just mentioned by keeping the index name to the model name only. It will be really helpful if you can tell what changes I have to do for that.

outoftime commented 11 years ago

Well then I might as well just do it myself : ) -- I will try to patch it later today.

On Oct 24, 2012, at 8:31, Abhas Goyal notifications@github.com wrote:

ya I tried but it was giving me some errors so for myself I made some changes in the other place you just mentioned by keeping the index name to the model name only. It will be really helpful if you can tell what changes I have to do for that.

— Reply to this email directly or view it on GitHubhttps://github.com/brewster/elastictastic/issues/21#issuecomment-9737533.

abhasg commented 11 years ago

thanx for the help @outoftime will wait for including your patch so that my work will easy.

abhasg commented 11 years ago

hey did you able to go forward with this request??

outoftime commented 11 years ago

Yes -- sorry for the delay. Version 0.10.5 is available, which allows you to do:

class MyClass

  include Elastictastic::Document
  self.default_index = 'some_index'
  # etc.

end