Bulk attributes save (#4167)

* history should use settings.SESSION_COOKIE_SAMESITE directions

* Optimize product attributes

* Cache attribute lookups

* simplify QuerysetCache

* Perform bulk update for product attributes

* Added tests for uncovered features.

* added 1 more test

* fixes test

* Added more tests

* multi_option can not be save in bulk

* it should be possible to skip validate_identifier because it will be very hard to import data with oscarapi if this is always required.

* misnamed test

* fixes test

* added test for attribte validation errors

* added test that validates that required attributes are not valid

* more tests

src/oscar/apps/catalogue/abstract_models.py

@@ -1,13 +1,16 @@
 import logging
 import os
 from datetime import date, datetime
-
 from django.conf import settings
 from django.contrib.contenttypes.fields import GenericForeignKey
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.staticfiles.finders import find
 from django.core.cache import cache
-from django.core.exceptions import ImproperlyConfigured, ValidationError
+from django.core.exceptions import (
+    ImproperlyConfigured,
+    ValidationError,
+    ObjectDoesNotExist,
+)
 from django.core.files.base import File
 from django.core.validators import RegexValidator
 from django.db import models
@@ -590,6 +593,10 @@ class AbstractProduct(models.Model):
         super().save(*args, **kwargs)
         self.attr.save()
 
+    def refresh_from_db(self, using=None, fields=None):
+        super().refresh_from_db(using, fields)
+        self.attr.invalidate()
+
     # Properties
 
     @property
@@ -968,63 +975,73 @@ class AbstractProductAttribute(models.Model):
         if self.type == self.BOOLEAN and self.required:
             raise ValidationError(_("Boolean attribute should not be required."))
 
-    def _save_file(self, value_obj, value):
-        # File fields in Django are treated differently, see
-        # django.db.models.fields.FileField and method save_form_data
+    def _get_value_obj(self, product, value):
+        try:
+            return product.attribute_values.get(attribute=self)
+        except ObjectDoesNotExist:
+            # FileField uses False for announcing deletion of the file
+            # not creating a new value
+            delete_file = self.is_file and value is False
+            if value is None or value == "" or delete_file:
+                return None
+            return product.attribute_values.create(attribute=self)
+
+    def _bind_value_file(self, value_obj, value):
         if value is None:
             # No change
-            return
+            return value_obj
         elif value is False:
-            # Delete file
-            value_obj.delete()
+            return None
         else:
             # New uploaded file
             value_obj.value = value
-            value_obj.save()
+            return value_obj
 
-    def _save_multi_option(self, value_obj, value):
+    def _bind_value_multi_option(self, value_obj, value):
         # ManyToMany fields are handled separately
         if value is None:
-            value_obj.delete()
-            return
+            return None
         try:
             count = value.count()
         except (AttributeError, TypeError):
             count = len(value)
         if count == 0:
-            value_obj.delete()
+            return None
         else:
             value_obj.value = value
-            value_obj.save()
+            return value_obj
 
-    def _save_value(self, value_obj, value):
+    def _bind_value(self, value_obj, value):
         if value is None or value == "":
-            value_obj.delete()
-            return
-        if value != value_obj.value:
-            value_obj.value = value
-            value_obj.save()
-
-    def save_value(self, product, value):
-        ProductAttributeValue = get_model("catalogue", "ProductAttributeValue")
-        try:
-            value_obj = product.attribute_values.get(attribute=self)
-        except ProductAttributeValue.DoesNotExist:
-            # FileField uses False for announcing deletion of the file
-            # not creating a new value
-            delete_file = self.is_file and value is False
-            if value is None or value == "" or delete_file:
-                return
-            value_obj = ProductAttributeValue.objects.create(
-                product=product, attribute=self
-            )
+            return None
+        value_obj.value = value
+        return value_obj
 
+    def bind_value(self, value_obj, value):
+        """
+        bind_value will bind the value passed to the value_obj, if the bind_value
+        return None, that means the value_obj is supposed to be deleted.
+        """
         if self.is_file:
-            self._save_file(value_obj, value)
+            return self._bind_value_file(value_obj, value)
         elif self.is_multi_option:
-            self._save_multi_option(value_obj, value)
+            return self._bind_value_multi_option(value_obj, value)
         else:
-            self._save_value(value_obj, value)
+            return self._bind_value(value_obj, value)
+
+    def save_value(self, product, value):
+        value_obj = self._get_value_obj(product, value)
+
+        if value_obj is None:
+            return None
+
+        updated_value_obj = self.bind_value(value_obj, value)
+        if updated_value_obj is None:
+            value_obj.delete()
+        elif updated_value_obj.is_dirty:
+            updated_value_obj.save()
+
+        return updated_value_obj
 
     def validate_value(self, value):
         validator = getattr(self, "_validate_%s" % self.type)
@@ -1158,28 +1175,43 @@ class AbstractProductAttributeValue(models.Model):
     entity_object_id = models.PositiveIntegerField(
         null=True, blank=True, editable=False
     )
+    _dirty = False
+
+    @cached_property
+    def type(self):
+        return self.attribute.type
+
+    @property
+    def value_field_name(self):
+        return "value_%s" % self.type
 
     def _get_value(self):
-        value = getattr(self, "value_%s" % self.attribute.type)
+        value = getattr(self, self.value_field_name)
         if hasattr(value, "all"):
             value = value.all()
         return value
 
     def _set_value(self, new_value):
-        attr_name = "value_%s" % self.attribute.type
+        attr_name = self.value_field_name
 
         if self.attribute.is_option and isinstance(new_value, str):
             # Need to look up instance of AttributeOption
             new_value = self.attribute.option_group.options.get(option=new_value)
         elif self.attribute.is_multi_option:
             getattr(self, attr_name).set(new_value)
+            self._dirty = True
             return
 
         setattr(self, attr_name, new_value)
+        self._dirty = True
         return
 
     value = property(_get_value, _set_value)
 
+    @property
+    def is_dirty(self):
+        return self._dirty
+
     class Meta:
         abstract = True
         app_label = "catalogue"
@@ -1204,8 +1236,11 @@ class AbstractProductAttributeValue(models.Model):
         e.g. image attribute values, declare a _image_as_text property and
         return something appropriate.
         """
-        property_name = "_%s_as_text" % self.attribute.type
-        return getattr(self, property_name, self.value)
+        try:
+            property_name = "_%s_as_text" % self.type
+            return getattr(self, property_name, self.value)
+        except ValueError:
+            return ""
 
     @property
     def _multi_option_as_text(self):
@@ -1241,7 +1276,7 @@ class AbstractProductAttributeValue(models.Model):
         return e.g. an ``<img>`` tag.  Defaults to the ``_as_text``
         representation.
         """
-        property_name = "_%s_as_html" % self.attribute.type
+        property_name = "_%s_as_html" % self.type
         return getattr(self, property_name, self.value_as_text)
 
     @property

src/oscar/apps/catalogue/managers.py

@@ -101,6 +101,10 @@ class ProductQuerySet(models.query.QuerySet):
         product_options = Option.objects.filter(product=OuterRef("pk"))
         return (
             self.select_related("product_class")
+            .annotate(
+                has_product_class_options=Exists(product_class_options),
+                has_product_options=Exists(product_options),
+            )
             .prefetch_related(
                 "children",
                 "product_options",
@@ -108,10 +112,6 @@ class ProductQuerySet(models.query.QuerySet):
                 "stockrecords",
                 "images",
             )
-            .annotate(
-                has_product_class_options=Exists(product_class_options),
-                has_product_options=Exists(product_options),
-            )
         )
 
     def browsable(self):

src/oscar/apps/catalogue/product_attributes.py

@@ -1,5 +1,46 @@
+from copy import deepcopy
+from django.db import models
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.utils.translation import gettext_lazy as _
+from django.utils.functional import cached_property
+
+
+class QuerysetCache(dict):
+    def __init__(self, queryset):
+        self._queryset = queryset
+        self._queryset_iterator = queryset.iterator()
+
+    def queryset(self):
+        return self._queryset
+
+    def get(self, code, default=None):
+        try:
+            return self[code]
+        except KeyError:
+            for instance in self._queryset_iterator:
+                instance_code = instance.code
+                self[instance_code] = instance
+                if instance_code == code:
+                    return instance
+
+        return default
+
+
+class AttributesQuerysetCache:
+    def __init__(self, product):
+        self.product = product
+
+    @cached_property
+    def attributes(self):
+        return QuerysetCache(self.product.get_product_class().attributes.all())
+
+    @cached_property
+    def attribute_values(self):
+        return QuerysetCache(
+            self.product.get_attribute_values()
+            .select_related("attribute")
+            .annotate(code=models.F("attribute__code"))
+        )
 
 
 class ProductAttributesContainer:
@@ -15,20 +56,48 @@ class ProductAttributesContainer:
         product.attr.refresh()
     """
 
-    # pylint: disable=access-member-before-definition
-    def __setstate__(self, state):
-        self.__dict__.setdefault("_product", None)
-        self.__dict__.setdefault("_initialized", False)
-        self.__dict__.setdefault("_dirty", set())
-        self.__dict__ = state
+    RESERVED_ATTRIBUTES = {
+        "_cache",
+        "_dirty",
+        "initialized",
+        "_initialized",
+        "_product",
+        "get_all_attributes",
+        "get_attribute_by_code",
+        "get_value_by_attribute",
+        "get_values",
+        "initialize",
+        "product",
+        "refresh",
+        "save",
+        "set",
+        "update",
+        "validate_attributes",
+    }
 
     def __init__(self, product):
         # use __dict__ directly to avoid triggering __setattr__, which would
         # cause a recursion error on _initialized.
         self.__dict__.update(
-            {"_product": product, "_initialized": False, "_dirty": set()}
+            {
+                "_product": product,
+                "_initialized": False,
+                "_dirty": set(),
+                "_cache": None,
+            }
         )
 
+    def __deepcopy__(self, memo):
+        cpy = ProductAttributesContainer(self.product)
+        memo[id(self)] = cpy
+        if self.initialized:
+            # Only copy attributes for initialized containers
+            for key, value in self.__dict__.items():
+                if key != "_cache":
+                    cpy.__dict__[key] = deepcopy(value, memo)
+
+        return cpy
+
     @property
     def product(self):
         return self._product
@@ -37,21 +106,28 @@ class ProductAttributesContainer:
     def initialized(self):
         return self._initialized
 
-    @initialized.setter
-    def initialized(self, value):
-        # use __dict__ directly to avoid triggering __setattr__, which would
-        # cause a recursion error.
-        self.__dict__["_initialized"] = value
+    @property
+    def cache(self):
+        if self.__dict__["_cache"] is None:
+            self.__dict__["_cache"] = AttributesQuerysetCache(self.product)
+        return self.__dict__["_cache"]
 
     def initialize(self):
-        self.initialized = True
+        self.__dict__["_initialized"] = True
         # initialize should not overwrite any values that have allready been set
         attrs = self.__dict__
-        for v in self.get_values().select_related("attribute"):
+        for v in self.get_values():
             attrs.setdefault(v.attribute.code, v.value)
 
+    def invalidate(self):
+        "Invalidate any stored data, queried from the database"
+        self.__dict__["_cache"] = None
+        self.__dict__["_initialized"] = False
+
     def refresh(self):
-        for v in self.get_values().select_related("attribute"):
+        "Reload any queried data from the database, discarding local changes"
+        self.__dict__["_cache"] = None
+        for v in self.get_values():
             setattr(self, v.attribute.code, v.value)
 
     def __getattribute__(self, name):
@@ -72,9 +148,29 @@ class ProductAttributesContainer:
         )
 
     def __setattr__(self, name, value):
+        if name in self.RESERVED_ATTRIBUTES:
+            raise ValidationError(
+                "%s is a reserved name and cannot be used as an attribute"
+            )
+
         self._dirty.add(name)
         super().__setattr__(name, value)
 
+    def set(self, name, value, validate_identifier=True):
+        if not validate_identifier or name.isidentifier():
+            self.__setattr__(name, value)
+        else:
+            raise ValidationError(
+                _(
+                    "%s is not a valid identifier, but attribute codes must be valid python identifiers"
+                    % name
+                )
+            )
+
+    def update(self, adict):
+        self._dirty.update(adict.keys())
+        self.__dict__.update(adict)
+
     def validate_attributes(self):
         for attribute in self.get_all_attributes():
             value = getattr(self, attribute.code, None)
@@ -94,21 +190,30 @@ class ProductAttributesContainer:
                     )
 
     def get_values(self):
-        return self.product.get_attribute_values()
+        return self.cache.attribute_values.queryset()
 
     def get_value_by_attribute(self, attribute):
-        return self.get_values().get(attribute=attribute)
+        return self.cache.attribute_values.get(attribute.code)
 
     def get_all_attributes(self):
-        return self.product.get_product_class().attributes.all()
+        return self.cache.attributes.queryset()
 
     def get_attribute_by_code(self, code):
-        return self.get_all_attributes().get(code=code)
+        return self.cache.attributes.get(code)
 
     def __iter__(self):
         return iter(self.get_values())
 
     def save(self):
+        if not self.initialized and not self._dirty:
+            return  # no need to save untouched attr lists
+
+        ProductAttributeValue = self.product.attribute_values.model
+
+        to_be_updated = []
+        update_fields = set()
+        to_be_deleted = []
+        to_be_created = []
         for attribute in self.get_all_attributes():
             if hasattr(self, attribute.code):
                 value = getattr(self, attribute.code)
@@ -121,9 +226,57 @@ class ProductAttributesContainer:
                     # needed and should always be saved.
                     try:
                         attribute_value_current = self.get_value_by_attribute(attribute)
-                        if attribute_value_current.value == value:
+                        if (
+                            attribute_value_current is not None
+                            and attribute_value_current.value == value
+                        ):
                             continue  # no new value needs to be saved
                     except ObjectDoesNotExist:
                         pass  # there is no existing value, so a value needs to be saved.
 
-                attribute.save_value(self.product, value)
+                if attribute.is_multi_option:  # multi_option can not be bulk saved
+                    attribute.save_value(self.product, value)
+                else:
+                    value_obj = self.get_value_by_attribute(attribute)
+                    if (
+                        value_obj is None or value_obj.product != self.product
+                    ):  # it doesn't exist yet so should be created
+                        new_value_obj = ProductAttributeValue(
+                            attribute=attribute, product=self.product
+                        )
+
+                        bound_value_obj = attribute.bind_value(new_value_obj, value)
+                        if bound_value_obj is not None:
+                            assert not bound_value_obj.pk
+                            to_be_created.append(bound_value_obj)
+                    else:
+                        bound_value_obj = attribute.bind_value(value_obj, value)
+                        if bound_value_obj is None:
+                            to_be_deleted.append(value_obj.pk)
+                        else:
+                            if bound_value_obj.attribute.is_file:
+                                # with bulk_create the file is save just fine, but
+                                # with buld_update, it's not, so we have to performa
+                                # that manually
+                                bound_value_obj._meta.get_field(
+                                    bound_value_obj.value_field_name
+                                ).pre_save(bound_value_obj, False)
+
+                            to_be_updated.append(bound_value_obj)
+                            update_fields.add(bound_value_obj.value_field_name)
+
+        # now save all the attributes in bulk
+        if to_be_deleted:
+            self.product.attribute_values.filter(pk__in=to_be_deleted).delete()
+        if to_be_updated:
+            self.product.attribute_values.bulk_update(
+                to_be_updated, update_fields, batch_size=500
+            )
+        if to_be_created:
+            self.product.attribute_values.bulk_create(
+                to_be_created, batch_size=500, ignore_conflicts=False
+            )
+
+        # after this the current data is nolonger valid and should be refetched
+        # from the database
+        self.invalidate()

src/oscar/apps/customer/history.py

@@ -13,6 +13,7 @@ class CustomerHistoryManager:
         "max_age": settings.OSCAR_RECENTLY_VIEWED_COOKIE_LIFETIME,
         "secure": settings.OSCAR_RECENTLY_VIEWED_COOKIE_SECURE,
         "httponly": True,
+        "samesite": settings.SESSION_COOKIE_SAMESITE,
     }
     max_products = settings.OSCAR_RECENTLY_VIEWED_PRODUCTS
 

tests/integration/catalogue/test_attributes.py

@@ -235,8 +235,52 @@ class TestTextAttributes(TestCase):
 
 class TestFileAttributes(TestCase):
     def setUp(self):
-        self.attr = factories.ProductAttributeFactory(type="file")
+        self.attr = factories.ProductAttributeFactory(type="file", code="file")
 
     def test_validate_file_values(self):
         file_field = SimpleUploadedFile("test_file.txt", b"Test")
         self.assertIsNone(self.attr.validate_value(file_field))
+
+    def test_erase_file(self):
+        product = factories.ProductFactory()
+        product.product_class.attributes.add(self.attr)
+
+        # save file attribute
+        file_field = SimpleUploadedFile("test_file.txt", b"Test")
+        self.assertIsNone(self.attr.validate_value(file_field))
+        self.attr.save_value(product, file_field)
+        self.assertTrue(self.attr.is_file)
+
+        product = Product.objects.get(pk=product.pk)
+
+        self.assertIsNotNone(
+            product.attr.file, "There should be something saved into the file attribute"
+        )
+        self.assertIn(
+            file_field.name,
+            product.attr.file.name,
+            "The save file should have the correct filename",
+        )
+
+        # set file attribute to None, which does nothing
+        product.attr.file = None
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        self.assertIsNotNone(
+            product.attr.file,
+            "There file should not be None, even though we set it to that",
+        )
+        self.assertIn(
+            file_field.name,
+            product.attr.file.name,
+            "The save file should still have the correct filename",
+        )
+
+        # set file attribute to False, which will erase it
+        product.attr.file = False
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        with self.assertRaises(AttributeError):
+            self.assertIn(file_field.name, product.attr.file.name)

tests/integration/catalogue/test_category.py

@@ -90,6 +90,20 @@ class TestMovingACategory(TestCase):
         gothic = Category.objects.get(name="Gothic")
         self.assertEqual("Books > Non-fiction > Horror > Gothic", gothic.full_name)
 
+    def test_fix_tree(self):
+        "fix_tree should rearrange the incorrect nodes and not cause any errors"
+        cat = Category.objects.get(path="00010002")
+        pk = cat.pk
+        self.assertEqual(cat.path, "00010002")
+
+        Category.objects.filter(pk=pk).update(path="00010050")
+        cat.refresh_from_db()
+        self.assertEqual(cat.path, "00010050")
+
+        Category.fix_tree(fix_paths=True)
+        cat.refresh_from_db()
+        self.assertEqual(cat.path, "00010003")
+
 
 class TestCategoryFactory(TestCase):
     def test_can_create_single_level_category(self):

tests/unit/catalogue/test_product_attributes.py

@@ -1,3 +1,4 @@
+import unittest
 from copy import deepcopy
 
 from django.core.exceptions import ValidationError
@@ -29,11 +30,19 @@ class ProductAttributeTest(TestCase):
             name="name",
             code="name",
         )
-        self.weight_attrs = ProductAttributeFactory(
+        self.weight_attr = ProductAttributeFactory(
             type=ProductAttribute.INTEGER,
             name="weight",
             code="weight",
             product_class=product_class,
+            required=True,
+        )
+        self.richtext_attr = ProductAttributeFactory(
+            type=ProductAttribute.RICHTEXT,
+            name="html",
+            code="html",
+            product_class=product_class,
+            required=False,
         )
 
         # create the parent product
@@ -202,11 +211,141 @@ class ProductAttributeTest(TestCase):
             "so it saved, even when the parent has the same value",
         )
 
+    def test_delete_attribute_value(self):
+        "Attributes should be deleted when they are nulled"
+        self.assertEqual(self.product.attr.weight, 3)
+        self.product.attr.weight = None
+        self.product.save()
+
+        p = Product.objects.get(pk=self.product.pk)
+        with self.assertRaises(AttributeError):
+            p.attr.weight  # pylint: disable=pointless-statement
+
+    def test_validate_attribute_value(self):
+        self.test_delete_attribute_value()
+        with self.assertRaises(ValidationError):
+            self.product.attr.validate_attributes()
+
     def test_deepcopy(self):
         "Deepcopy should not cause a recursion error"
         deepcopy(self.product)
         deepcopy(self.child_product)
 
+    def test_set(self):
+        "Attributes should be settable from a string key"
+        self.product.attr.set("weight", 101)
+        self.assertEqual(self.product.attr._dirty, {"weight"})
+        self.product.attr.save()
+
+        p = Product.objects.get(pk=self.product.pk)
+
+        self.assertEqual(p.attr.weight, 101)
+
+    def test_set_error(self):
+        "set should only accept attributes which are valid python identifiers"
+        with self.assertRaises(ValidationError):
+            self.product.attr.set("bina-weight", 101)
+
+        with self.assertRaises(ValidationError):
+            self.product.attr.set("8_oepla", "oegaranos")
+
+        with self.assertRaises(ValidationError):
+            self.product.attr.set("set", "validate_identifier=True")
+
+        with self.assertRaises(ValidationError):
+            self.product.attr.set("save", "raise=True")
+
+    def test_update(self):
+        "Attributes should be updateble from a dictionary"
+        self.product.attr.update({"weight": 808, "name": "a banana"})
+        self.assertEqual(self.product.attr._dirty, {"weight", "name"})
+        self.product.attr.save()
+
+        p = Product.objects.get(pk=self.product.pk)
+
+        self.assertEqual(p.attr.weight, 808)
+        self.assertEqual(p.attr.name, "a banana")
+
+    def test_validate_attributes(self):
+        "validate_attributes should raise ValidationError on erroneous inputs"
+        self.product.attr.validate_attributes()
+        self.product.attr.weight = "koe"
+        with self.assertRaises(ValidationError):
+            self.product.attr.validate_attributes()
+
+    def test_get_attribute_by_code(self):
+        at = self.product.attr.get_attribute_by_code("weight")
+        self.assertEqual(at.code, "weight")
+        self.assertEqual(at.product_class, self.product.get_product_class())
+
+        self.assertIsNone(self.product.attr.get_attribute_by_code("stoubafluppie"))
+
+    def test_attribute_html(self):
+        self.product.attr.html = "<h1>Hi</h1>"
+        self.product.save()
+
+        value = self.product.attr.get_value_by_attribute(self.richtext_attr)
+        html = value.value_as_html
+        self.assertEqual(html, "<h1>Hi</h1>")
+        self.assertTrue(hasattr(html, "__html__"))
+
+
+class MultiOptionTest(TestCase):
+    fixtures = ["productattributes"]
+    maxDiff = None
+
+    def test_multi_option_recursion_error(self):
+        product = Product.objects.get(pk=4)
+        with self.assertRaises(ValueError):
+            product.attr.set("subkinds", "harrie")
+            product.save()
+
+    def test_value_as_html(self):
+        product = Product.objects.get(pk=4)
+        # pylint: disable=unused-variable
+        (
+            additional_info,
+            available,
+            facets,
+            hypothenusa,
+            kind,
+            releasedate,
+            starttime,
+            subkinds,
+            subtitle,
+        ) = product.attr.get_values().order_by("id")
+
+        self.assertTrue(
+            additional_info.value_as_html.startswith(
+                '<p style="margin: 0px; font-stretch: normal; font-size: 12px;'
+            )
+        )
+        self.assertEqual(available.value_as_html, "Yes")
+        self.assertEqual(kind.value_as_html, "bombastic")
+        self.assertEqual(subkinds.value_as_html, "grand, verocious, megalomane")
+        self.assertEqual(subtitle.value_as_html, "kekjo")
+
+    @unittest.skip("The implementation is wrong, which makes these tests fail")
+    def test_broken_value_as_html(self):
+        product = Product.objects.get(pk=4)
+        # pylint: disable=unused-variable
+        (
+            additional_info,
+            available,
+            facets,
+            hypothenusa,
+            kind,
+            releasedate,
+            starttime,
+            subkinds,
+            subtitle,
+        ) = product.attr.get_values().order_by("id")
+
+        self.assertEqual(starttime.value_as_html, "2018-11-16T09:15:00+00:00")
+        self.assertEqual(facets.value_as_html, "4")
+        self.assertEqual(releasedate.value_as_html, "2018-11-16")
+        self.assertEqual(hypothenusa.value_as_html, "2.4567")
+
 
 class ProductAttributeQuerysetTest(TestCase):
     fixtures = ["productattributes"]