Fix DocType._get_actions preventing delete (django-es#370)

* DocType._get_actions: do not call should_index_object if action is delete

* flake8

Author: seljin

django_elasticsearch_dsl/documents.py

@@ -51,8 +51,10 @@
     models.UUIDField: KeywordField,
 }
 
+
 class DocType(DSLDocument):
     _prepared_fields = []
+
     def __init__(self, related_instance_to_ignore=None, **kwargs):
         super(DocType, self).__init__(**kwargs)
         self._related_instance_to_ignore = related_instance_to_ignore
@@ -125,8 +127,8 @@ def prepare(self, instance):
         """
         data = {
             name: prep_func(instance)
-                for name, field, prep_func in self._prepared_fields
-            }
+            for name, field, prep_func in self._prepared_fields
+        }
         return data
 
     @classmethod
@@ -188,7 +190,7 @@ def _prepare_action(self, object_instance, action):
 
     def _get_actions(self, object_list, action):
         for object_instance in object_list:
-            if self.should_index_object(object_instance):
+            if action == 'delete' or self.should_index_object(object_instance):
                 yield self._prepare_action(object_instance, action)
 
     def _bulk(self, *args, **kwargs):
@@ -201,7 +203,7 @@ def _bulk(self, *args, **kwargs):
 
     def should_index_object(self, obj):
         """
-        Overwriting this method and returning a boolean value 
+        Overwriting this method and returning a boolean value
         should determine whether the object should be indexed.
         """
         return True

django_elasticsearch_dsl/fields.py

@@ -1,4 +1,3 @@
-import collections
 from types import MethodType
 
 from django.core.exceptions import ObjectDoesNotExist
@@ -108,7 +107,7 @@ def _get_inner_field_data(self, obj, field_value_to_ignore=None):
                     obj, field_value_to_ignore
                 )
         else:
-            for name, field in self._doc_class._doc_type.mapping.properties._params.get('properties', {}).items(): # noqa
+            for name, field in self._doc_class._doc_type.mapping.properties._params.get('properties', {}).items():  # noqa
                 if not isinstance(field, DEDField):
                     continue
 

django_elasticsearch_dsl/registries.py

@@ -5,7 +5,7 @@
 
 from django.core.exceptions import ObjectDoesNotExist
 from django.core.exceptions import ImproperlyConfigured
-from elasticsearch_dsl import Field, AttrDict
+from elasticsearch_dsl import AttrDict
 from six import itervalues, iterkeys, iteritems
 
 from django_elasticsearch_dsl.exceptions import RedeclaredFieldError

tests/documents.py

@@ -1,5 +1,5 @@
 from elasticsearch_dsl import analyzer
-from django_elasticsearch_dsl import Document, Index, fields
+from django_elasticsearch_dsl import Document, fields
 from django_elasticsearch_dsl.registries import registry
 
 from .models import Ad, Category, Car, Manufacturer, Article
@@ -160,6 +160,7 @@ class Index:
         name = 'test_articles'
         settings = index_settings
 
+
 @registry.register_document
 class ArticleWithSlugAsIdDocument(Document):
     class Django:

tests/test_documents.py

@@ -3,18 +3,17 @@
 
 from django.db import models
 from django.utils.translation import ugettext_lazy as _
-from elasticsearch_dsl import GeoPoint, MetaField, InnerDoc
-from mock import patch, Mock, PropertyMock
+from elasticsearch_dsl import GeoPoint, InnerDoc
+from mock import patch, Mock
 
 from django_elasticsearch_dsl import fields
-from django_elasticsearch_dsl.documents import DocType, Document
+from django_elasticsearch_dsl.documents import DocType
 from django_elasticsearch_dsl.exceptions import (ModelFieldNotMappedError,
                                                  RedeclaredFieldError)
 from django_elasticsearch_dsl.registries import registry
 from tests import ES_MAJOR_VERSION
 
 from .models import Article
-from .documents import ArticleDocument, ArticleWithSlugAsIdDocument
 
 
 class Car(models.Model):
@@ -72,7 +71,6 @@ def test_auto_refresh_default(self):
         self.assertTrue(CarDocument.django.auto_refresh)
 
     def test_ignore_signal_added(self):
-
         @registry.register_document
         class CarDocument2(DocType):
             class Django:
@@ -138,20 +136,20 @@ def test_mapping(self):
 
         self.assertEqual(
             CarDocument._doc_type.mapping.to_dict(), {
-                    'properties': {
-                        'name': {
-                            'type': text_type
-                        },
-                        'color': {
-                            'type': text_type
-                        },
-                        'type': {
-                            'type': text_type
-                        },
-                        'price': {
-                            'type': 'double'
-                        }
+                'properties': {
+                    'name': {
+                        'type': text_type
+                    },
+                    'color': {
+                        'type': text_type
+                    },
+                    'type': {
+                        'type': text_type
+                    },
+                    'price': {
+                        'type': 'double'
                     }
+                }
             }
         )
 
@@ -359,7 +357,7 @@ class Django:
         bulk = "django_elasticsearch_dsl.documents.bulk"
         parallel_bulk = "django_elasticsearch_dsl.documents.parallel_bulk"
         with patch(bulk) as mock_bulk, patch(parallel_bulk) as mock_parallel_bulk:
-            doc.update([car3, car2, car3])
+            doc.update([car1, car2, car3])
             self.assertEqual(
                 3, len(list(mock_bulk.call_args_list[0][1]['actions']))
             )
@@ -390,13 +388,13 @@ def test_init_prepare_correct(self):
 
         expect = {
             'color': ("<class 'django_elasticsearch_dsl.fields.TextField'>",
-                    ("<class 'method'>", "<type 'instancemethod'>")), # py3, py2
+                      ("<class 'method'>", "<type 'instancemethod'>")),  # py3, py2
             'type': ("<class 'django_elasticsearch_dsl.fields.TextField'>",
-                    ("<class 'functools.partial'>","<type 'functools.partial'>")),
+                     ("<class 'functools.partial'>", "<type 'functools.partial'>")),
             'name': ("<class 'django_elasticsearch_dsl.fields.TextField'>",
-                    ("<class 'functools.partial'>","<type 'functools.partial'>")),
+                     ("<class 'functools.partial'>", "<type 'functools.partial'>")),
             'price': ("<class 'django_elasticsearch_dsl.fields.DoubleField'>",
-                    ("<class 'functools.partial'>","<type 'functools.partial'>")),
+                      ("<class 'functools.partial'>", "<type 'functools.partial'>")),
         }
 
         for name, field, prep in d._prepared_fields:
@@ -412,8 +410,8 @@ def test_init_prepare_results(self):
         car = Car()
         setattr(car, 'name', "Tusla")
         setattr(car, 'price', 340123.21)
-        setattr(car, 'color', "polka-dots") # Overwritten by prepare function
-        setattr(car, 'pk', 4701) # Ignored, not in document
+        setattr(car, 'color', "polka-dots")  # Overwritten by prepare function
+        setattr(car, 'pk', 4701)  # Ignored, not in document
         setattr(car, 'type', "imaginary")
 
         self.assertEqual(d.prepare(car), {'color': 'blue', 'type': 'imaginary', 'name': 'Tusla', 'price': 340123.21})
@@ -425,8 +423,8 @@ def test_init_prepare_results(self):
         with patch.object(CarDocument, '_fields', 33):
             d.prepare(m)
         self.assertEqual(sorted([tuple(x) for x in m.method_calls], key=lambda _: _[0]),
-                         [('name', (), {}),  ('price', (), {}), ('type', (), {})]
-        )
+                         [('name', (), {}), ('price', (), {}), ('type', (), {})]
+                         )
 
     # Mock the elasticsearch connection because we need to execute the bulk so that the generator
     # got iterated and generate_id called.
@@ -438,6 +436,7 @@ def test_default_generate_id_is_called(self, _):
             id=124594,
             slug='some-article',
         )
+
         @registry.register_document
         class ArticleDocument(DocType):
             class Django:

tests/test_fields.py

@@ -124,11 +124,11 @@ def test_get_value_from_instance_without_properties(self):
         )
 
         self.assertEqual(field.get_value_from_instance(instance),
-            {
-                'first_name': "foo",
-                'last_name': "bar"
-            }
-        )
+                         {
+                             'first_name': "foo",
+                             'last_name': "bar"
+                         }
+                         )
 
     def test_get_value_from_instance_with_inner_objectfield(self):
         field = ObjectField(attr='person', properties={
@@ -167,12 +167,12 @@ def test_get_value_from_instance_with_inner_objectfield_without_properties(self)
         ))
 
         self.assertEqual(field.get_value_from_instance(instance),
-            {
-                'first_name': "foo",
-                'last_name': "bar",
-                'additional': {'age': 12}
-            }
-        )
+                         {
+                             'first_name': "foo",
+                             'last_name': "bar",
+                             'additional': {'age': 12}
+                         }
+                         )
 
     def test_get_value_from_instance_with_none_inner_objectfield(self):
         field = ObjectField(attr='person', properties={
@@ -233,17 +233,17 @@ def test_get_value_from_iterable_without_properties(self):
         )
 
         self.assertEqual(field.get_value_from_instance(instance),
-            [
-                {
-                    'first_name': "foo1",
-                    'last_name': "bar1",
-                },
-                {
-                    'first_name': "foo2",
-                    'last_name': "bar2",
-                }
-            ]
-        )
+                         [
+                             {
+                                 'first_name': "foo1",
+                                 'last_name': "bar1",
+                             },
+                             {
+                                 'first_name': "foo2",
+                                 'last_name': "bar2",
+                             }
+                         ]
+                         )
 
 
 class NestedFieldTestCase(TestCase):
@@ -282,17 +282,6 @@ def test_get_mapping(self):
         }, field.to_dict())
 
 
-class TextFieldTestCase(TestCase):
-    def test_get_mapping(self):
-        field = TextField()
-
-        expected_type = 'string' if ES_MAJOR_VERSION == 2 else 'text'
-
-        self.assertEqual({
-            'type': expected_type,
-        }, field.to_dict())
-
-
 class CompletionFieldTestCase(TestCase):
     def test_get_mapping(self):
         field = CompletionField()

tests/test_integration.py

@@ -17,7 +17,6 @@
     car_index,
     CarDocument,
     CarWithPrepareDocument,
-    ManufacturerDocument,
     ArticleDocument,
     ArticleWithSlugAsIdDocument,
     index_settings
@@ -359,8 +358,8 @@ def test_default_document_id(self):
             slug=article_slug,
         )
 
-        # saving should create two documents (in the two indices): one with the 
-        # Django object's id as the ES doc _id, and the other with the slug 
+        # saving should create two documents (in the two indices): one with the
+        # Django object's id as the ES doc _id, and the other with the slug
         # as the ES _id
         article.save()
 
@@ -377,8 +376,8 @@ def test_custom_document_id(self):
             slug=article_slug,
         )
 
-        # saving should create two documents (in the two indices): one with the 
-        # Django object's id as the ES doc _id, and the other with the slug 
+        # saving should create two documents (in the two indices): one with the
+        # Django object's id as the ES doc _id, and the other with the slug
         # as the ES _id
         article.save()