django-elasticsearch-dsl icon indicating copy to clipboard operation
django-elasticsearch-dsl copied to clipboard

DocType inheritance

Open krelst opened this issue 6 years ago • 15 comments

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?

krelst avatar Sep 22 '18 19:09 krelst

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.

safwanrahman avatar Sep 23 '18 19:09 safwanrahman

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'

krelst avatar Sep 23 '18 19:09 krelst

have you tried placing the mixin after the DocType class?

class CarDocument(DocType, DefaultDocumentFieldsMixin):
     pass

safwanrahman avatar Sep 23 '18 20:09 safwanrahman

@krelst Did it work?

safwanrahman avatar Sep 24 '18 19:09 safwanrahman

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)

krelst avatar Sep 24 '18 19:09 krelst

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

safwanrahman avatar Sep 24 '18 19:09 safwanrahman

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

krelst avatar Sep 24 '18 19:09 krelst

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

safwanrahman avatar Sep 24 '18 20:09 safwanrahman

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...

krelst avatar Sep 24 '18 20:09 krelst

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

safwanrahman avatar Sep 24 '18 20:09 safwanrahman

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?

krelst avatar Sep 24 '18 20:09 krelst

@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!

safwanrahman avatar Sep 24 '18 20:09 safwanrahman

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.

MichalGumkowski avatar Nov 05 '18 11:11 MichalGumkowski

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

ar7n avatar May 14 '19 12:05 ar7n

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.

thomasrosales avatar Jan 27 '22 13:01 thomasrosales