django-es / django-elasticsearch-dsl

This is a package that allows indexing of django models in elasticsearch with elasticsearch-dsl-py.
Other
1.03k stars 264 forks source link

DocType inheritance #132

Open krelst opened 6 years ago

krelst commented 6 years ago

It would be nice to be able to have a mixin with some common fields you use in every document. This way the mixin can be used to avoid having to declare the common fields on all documents. E.g. I am developing a multi-tenant application, so I would like to add the tenant id to every document.

An example:

from django_elasticsearch_dsl import DocType, fields

class DefaultDocumentFieldsMixin(object):
    tenant_id = fields.KeywordField()

    def prepare_tenant_id(self, instance):
        return get_active_tenant().id

class CarDocument(DefaultDocumentFieldsMixin, DocType):
     pass

At this moment this doesn't seem to be working, as the tenant_id is not added to any of the documents I use this mixin. Is this intended behaviour, or actually a bug? Is there another way to get to achieve the same behaviour?

safwanrahman commented 6 years ago

I think you can do something like this

class DefaultDocumentType(DocType):
    tenant_id = fields.KeywordField()

    def prepare_tenant_id(self, instance):
        return get_active_tenant().id

and use DefaultDocumentType everywhere you do need.

krelst commented 6 years ago

I tried to do that first but then you get an error, because the Meta class is missing:

  File "...", line 9, in <module>
    from core.utils.es_utils import DefaultDocumentType
  File "...", line 66, in <module>
    class DefaultDocumentType(DocType):
  File "...", line 66, in __new__
    model = attrs['Meta'].model
KeyError: 'Meta'
safwanrahman commented 6 years ago

have you tried placing the mixin after the DocType class?

class CarDocument(DocType, DefaultDocumentFieldsMixin):
     pass
safwanrahman commented 6 years ago

@krelst Did it work?

krelst commented 6 years ago

Yes I already tried that as well. But unfortunately this doesn't solve the issue; none of the DefaultDocumentFieldsMixin fields get added to the index (so tenant_id is not in the mapping of the index)

safwanrahman commented 6 years ago

I think its better to open issue in elasticsearch-dsl package then. the meta class is making all the magics!

krelst commented 6 years ago

They seem to support inheritance: https://github.com/elastic/elasticsearch-dsl-py/blob/master/examples/parent_child.py#L48

safwanrahman commented 6 years ago

It seems to be a bug in this end maybe add a meta class in the abstract docType class?

class DefaultDocumentType(DocType):
    tenant_id = fields.KeywordField()

    class Meta:
        model = None
krelst commented 6 years ago

Also already tried that one, but then of course this package tries to do a lot of stuff with that model attribute...

  File "...", line 91, in __new__
    fields = model._meta.get_fields()
AttributeError: 'NoneType' object has no attribute '_meta'

Just doesn't seem to be supported by this package. It would be good to have a abstract attribute on the Meta class, just as django does with models...

safwanrahman commented 6 years ago

@krelst I understand. so for workaround can you just pass a model class in the model attribute and try with that?

krelst commented 6 years ago

If you pass the django models.Model it doesn't work, but indeed adding one of my model classes does work... Although it's a very ugly hack to make this work! Any idea whether the abstract attribute could be easily implemented?

safwanrahman commented 6 years ago

@krelst It can be implemented. But need some modification in the metaclass that we are modifying. I am trying to make it compatable with latest version of elasticsearch-dsl and I hope that will solve the issue you are facing. I can not give any commitment when it can be fixed, but till then I think use the hack to workarount it! sorry!

MichalGumkowski commented 6 years ago

I know it may be a little too late, but maybe that will solve the problem for the future generations. I may not be an expert, but Meta should be treated as an independent object and you should not depend on inheritance in that particular case (it works in the same way with DRF's serializers). Have you tried to define it in desired class (CarDocument)?

That way you can treat DefaultDocumentFieldsMixin as just a mixin (inherit from an object), and have full control over final class.

ar7n commented 5 years ago

@krelst Use elasticsearch_dsl.document.DocType as parent for your mixin

thomasrosales commented 2 years ago

Hi guys! How are you? I've used the approach of @ar7n and it's working to the use case of creating a base document to inheritance common fields amongst different models:

# from django_elasticsearch_dsl import fields
# from elasticsearch_dsl.document import DocType

class BaseDocument(DocType):
    a_field= fields.TextField(
        attr="ticker",
        fields={"keyword": fields.KeywordField()},
    )
    b_field= fields.TextField(
        attr="ticker_name",
        fields={"keyword": fields.KeywordField()},
    )
from django_elasticsearch_dsl import DocType, Index, fields

@INDEX.doc_type
class ModelDocument(BaseDocument, DocType):
      class Meta:
          model = Model

Terminal output:

(virtual-env-nDkKHiQU) λ python src\manage.py search_index --models model.Model --create
Creating index 'dev_model'

As a suggestion, maybe it's a good idea to add it to the documentation of django_elasticsearch_dsl for everyone that has a similar use case.