Optimize attribute values (#321)

* Implemented faster attributes save

* renamed for clarity

oscarapi/serializers/admin/product.py

@@ -145,12 +145,15 @@ def update(self, instance, validated_data):
             if (
                 self.partial
             ):  # we need to clean up all the attributes with wrong product class
+                attribute_codes = product_class.attributes.values_list(
+                    "code", flat=True
+                )
                 for attribute_value in instance.attribute_values.exclude(
                     attribute__product_class=product_class
                 ):
                     code = attribute_value.attribute.code
                     if (
-                        code in pclass_option_codes
+                        code in attribute_codes
                     ):  # if the attribute exist also on the new product class, update the attribute
                         attribute_value.attribute = product_class.attributes.get(
                             code=code

oscarapi/serializers/fields.py

@@ -1,6 +1,7 @@
 # pylint: disable=W0212, W0201, W0632
 import logging
 import operator
+import warnings
 
 from os.path import basename, join
 from urllib.parse import urlsplit, parse_qs
@@ -15,8 +16,10 @@
 from rest_framework.fields import get_attribute
 
 from oscar.core.loading import get_model, get_class
+from oscarapi.utils.deprecations import RemovedInOScarAPI4
 
 from oscarapi import settings
+from oscarapi.utils.attributes import AttributeFieldBase, attribute_details
 from oscarapi.utils.loading import get_api_class
 from oscarapi.utils.exists import bound_unique_together_get_or_create
 from .exceptions import FieldError
@@ -27,7 +30,6 @@
 create_from_breadcrumbs = get_class("catalogue.categories", "create_from_breadcrumbs")
 entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value")
 RetrieveFileMixin = get_api_class(settings.FILE_DOWNLOADER_MODULE, "RetrieveFileMixin")
-attribute_details = operator.itemgetter("code", "value")
 
 
 class TaxIncludedDecimalField(serializers.DecimalField):
@@ -93,7 +95,7 @@ def use_pk_only_optimization(self):
         return False
 
 
-class AttributeValueField(serializers.Field):
+class AttributeValueField(AttributeFieldBase, serializers.Field):
     """
     This field is used to handle the value of the ProductAttributeValue model
 
@@ -103,80 +105,56 @@ class AttributeValueField(serializers.Field):
     """
 
     def __init__(self, **kwargs):
+        warnings.warn(
+            "AttributeValueField is deprecated and will be removed in a future version of oscarapi",
+            RemovedInOScarAPI4,
+            stacklevel=2,
+        )
         # this field always needs the full object
         kwargs["source"] = "*"
-        kwargs["error_messages"] = {
-            "no_such_option": _("{code}: Option {value} does not exist."),
-            "invalid": _("Wrong type, {error}."),
-            "attribute_validation_error": _(
-                "Error assigning `{value}` to {code}, {error}."
-            ),
-            "attribute_required": _("Attribute {code} is required."),
-            "attribute_missing": _(
-                "No attribute exist with code={code}, "
-                "please define it in the product_class first."
-            ),
-            "child_without_parent": _(
-                "Can not find attribute if product_class is empty and "
-                "parent is empty as well, child without parent?"
-            ),
-        }
         super(AttributeValueField, self).__init__(**kwargs)
 
     def get_value(self, dictionary):
         # return all the data because this field uses everything
         return dictionary
 
+    def to_product_attribute(self, data):
+        if "product" in data:
+            # we need the attribute to determine the type of the value
+            return ProductAttribute.objects.get(
+                code=data["code"], product_class__products__id=data["product"]
+            )
+        elif "product_class" in data and data["product_class"] is not None:
+            return ProductAttribute.objects.get(
+                code=data["code"], product_class__slug=data.get("product_class")
+            )
+        elif "parent" in data:
+            return ProductAttribute.objects.get(
+                code=data["code"], product_class__products__id=data["parent"]
+            )
+
+    def to_attribute_type_value(self, attribute, code, value):
+        internal_value = super().to_attribute_type_value(attribute, code, value)
+        if attribute.type in [
+            attribute.IMAGE,
+            attribute.FILE,
+        ]:
+            image_field = ImageUrlField()
+            image_field._context = self.context
+            internal_value = image_field.to_internal_value(value)
+
+        return internal_value
+
     def to_internal_value(self, data):  # noqa
         assert "product" in data or "product_class" in data or "parent" in data
 
         try:
             code, value = attribute_details(data)
             internal_value = value
 
-            if "product" in data:
-                # we need the attribute to determine the type of the value
-                attribute = ProductAttribute.objects.get(
-                    code=code, product_class__products__id=data["product"]
-                )
-            elif "product_class" in data and data["product_class"] is not None:
-                attribute = ProductAttribute.objects.get(
-                    code=code, product_class__slug=data.get("product_class")
-                )
-            elif "parent" in data:
-                attribute = ProductAttribute.objects.get(
-                    code=code, product_class__products__id=data["parent"]
-                )
+            attribute = self.to_product_attribute(data)
 
-            if attribute.required and value is None:
-                self.fail("attribute_required", code=code)
-
-            # some of these attribute types need special processing, or their
-            # validation will fail
-            if attribute.type == attribute.OPTION:
-                internal_value = attribute.option_group.options.get(option=value)
-            elif attribute.type == attribute.MULTI_OPTION:
-                if attribute.required and not value:
-                    self.fail("attribute_required", code=code)
-                internal_value = attribute.option_group.options.filter(option__in=value)
-                if len(value) != internal_value.count():
-                    non_existing = set(value) - set(
-                        internal_value.values_list("option", flat=True)
-                    )
-                    non_existing_as_error = ",".join(sorted(non_existing))
-                    self.fail("no_such_option", value=non_existing_as_error, code=code)
-            elif attribute.type == attribute.DATE:
-                date_field = serializers.DateField()
-                internal_value = date_field.to_internal_value(value)
-            elif attribute.type == attribute.DATETIME:
-                date_field = serializers.DateTimeField()
-                internal_value = date_field.to_internal_value(value)
-            elif attribute.type == attribute.ENTITY:
-                internal_value = entity_internal_value(attribute, value)
-            elif attribute.type in [attribute.IMAGE, attribute.FILE]:
-                image_field = ImageUrlField()
-                image_field._context = self.context
-                internal_value = image_field.to_internal_value(value)
+            internal_value = self.to_attribute_type_value(attribute, code, value)
 
             # the rest of the attribute types don't need special processing
             try:

oscarapi/serializers/product.py

@@ -2,6 +2,7 @@
 
 import logging
 from copy import deepcopy
+from django.db.models.manager import Manager
 from django.utils.translation import gettext as _
 
 from rest_framework import serializers
@@ -15,7 +16,7 @@
 from oscarapi.utils.files import file_hash
 from oscarapi.utils.exists import find_existing_attribute_option_group
 from oscarapi.utils.accessors import getitems
-
+from oscarapi.utils.attributes import AttributeConverter
 from oscarapi.serializers.fields import DrillDownHyperlinkedIdentityField
 from oscarapi.serializers.utils import (
     OscarModelSerializer,
@@ -195,6 +196,71 @@ class Meta:
 
 
 class ProductAttributeValueListSerializer(UpdateListSerializer):
+    # pylint: disable=unused-argument
+    def shortcut_to_internal_value(self, data, productclass, attributes):
+        difficult_attributes = {
+            at.code: at
+            for at in productclass.attributes.filter(
+                type__in=[
+                    ProductAttribute.OPTION,
+                    ProductAttribute.MULTI_OPTION,
+                    ProductAttribute.DATE,
+                    ProductAttribute.DATETIME,
+                    ProductAttribute.ENTITY,
+                ]
+            )
+        }
+        cv = AttributeConverter(self.context)
+        internal_value = []
+        for item in data:
+            code, value = getitems(item, "code", "value")
+            if code is None:  # delegate error state to child serializer
+                internal_value.append(self.child.to_internal_value(item))
+
+            if code in difficult_attributes:
+                attribute = difficult_attributes[code]
+                converted_value = cv.to_attribute_type_value(attribute, code, value)
+                internal_value.append(
+                    {
+                        "value": converted_value,
+                        "attribute": attribute,
+                        "product_class": productclass,
+                    }
+                )
+            else:
+                internal_value.append(
+                    {
+                        "value": value,
+                        "attribute": code,
+                        "product_class": productclass,
+                    }
+                )
+
+        return internal_value
+
+    def to_internal_value(self, data):
+        productclasses = set()
+        attributes = set()
+
+        for item in data:
+            product_class, code = getitems(item, "product_class", "code")
+            if product_class:
+                productclasses.add(product_class)
+            attributes.add(code)
+
+        # if all attributes belong to the same productclass, everything is just
+        # as expected and we can take a shortcut by only resolving the
+        # productclass to the model instance and nothing else.
+        try:
+            if len(productclasses) == 1 and all(attributes):
+                (product_class,) = productclasses
+                pc = ProductClass.objects.get(slug=product_class)
+                return self.shortcut_to_internal_value(data, pc, attributes)
+        except ProductClass.DoesNotExist:
+            pass
+
+        return super().to_internal_value(data)
+
     def get_value(self, dictionary):
         values = super(ProductAttributeValueListSerializer, self).get_value(dictionary)
         if values is empty:
@@ -205,6 +271,44 @@ def get_value(self, dictionary):
             dict(value, product_class=product_class, parent=parent) for value in values
         ]
 
+    def to_representation(self, data):
+        if isinstance(data, Manager):
+            # use a cached query from product.attr to get the attributes instead
+            # if an silly .all() that clones the queryset and performs a new query
+            _, product = self.get_name_and_rel_instance(data)
+            iterable = product.attr.get_values()
+        else:
+            iterable = data
+
+        return [self.child.to_representation(item) for item in iterable]
+
+    def update(self, instance, validated_data):
+        assert isinstance(instance, Manager)
+
+        _, product = self.get_name_and_rel_instance(instance)
+
+        attr_codes = []
+        product.attr.initialize()
+        for validated_datum in validated_data:
+            # leave all the attribute saving to the ProductAttributesContainer instead
+            # of the child serializers
+            attribute, value = getitems(validated_datum, "attribute", "value")
+            if hasattr(
+                attribute, "code"
+            ):  # if the attribute is a model instance use the code
+                product.attr.set(attribute.code, value, validate_identifier=False)
+                attr_codes.append(attribute.code)
+            else:
+                product.attr.set(attribute, value, validate_identifier=False)
+                attr_codes.append(attribute)
+
+        # if we don't clear the dirty attributes all parent attributes
+        # are marked as explicitly set, so they will be copied to the
+        # child product.
+        product.attr._dirty.clear()  # pylint: disable=protected-access
+        product.attr.save()
+        return list(product.attr.get_values().filter(attribute__code__in=attr_codes))
+
 
 class ProductAttributeValueSerializer(OscarModelSerializer):
     # we declare the product as write_only since this serializer is meant to be

oscarapi/tests/unit/testproduct.py

@@ -994,6 +994,7 @@ def test_switch_product_class_patch(self):
         they may cause errors.
         """
         product = Product.objects.get(pk=3)
+        self.assertEqual(product.attribute_values.count(), 11)
         ser = AdminProductSerializer(
             data={
                 "product_class": "t-shirt",
@@ -1006,6 +1007,7 @@ def test_switch_product_class_patch(self):
         )
         self.assertTrue(ser.is_valid(), "Something wrong %s" % ser.errors)
         obj = ser.save()
+
         self.assertEqual(
             obj.attribute_values.count(),
             2,

oscarapi/utils/attributes.py

@@ -0,0 +1,78 @@
+import operator
+from django.utils.translation import gettext_lazy as _
+from rest_framework import serializers
+from rest_framework.fields import MISSING_ERROR_MESSAGE
+from rest_framework.exceptions import ErrorDetail
+from oscarapi.utils.loading import get_api_class
+
+attribute_details = operator.itemgetter("code", "value")
+entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value")
+
+
+class AttributeFieldBase:
+    default_error_messages = {
+        "no_such_option": _("{code}: Option {value} does not exist."),
+        "invalid": _("Wrong type, {error}."),
+        "attribute_validation_error": _(
+            "Error assigning `{value}` to {code}, {error}."
+        ),
+        "attribute_required": _("Attribute {code} is required."),
+        "attribute_missing": _(
+            "No attribute exist with code={code}, "
+            "please define it in the product_class first."
+        ),
+        "child_without_parent": _(
+            "Can not find attribute if product_class is empty and "
+            "parent is empty as well, child without parent?"
+        ),
+    }
+
+    def to_attribute_type_value(self, attribute, code, value):
+        internal_value = value
+        # pylint: disable=no-member
+        if attribute.required and value is None:
+            self.fail("attribute_required", code=code)
+
+        # some of these attribute types need special processing, or their
+        # validation will fail
+        if attribute.type == attribute.OPTION:
+            internal_value = attribute.option_group.options.get(option=value)
+        elif attribute.type == attribute.MULTI_OPTION:
+            if attribute.required and not value:
+                self.fail("attribute_required", code=code)
+            internal_value = attribute.option_group.options.filter(option__in=value)
+            if len(value) != internal_value.count():
+                non_existing = set(value) - set(
+                    internal_value.values_list("option", flat=True)
+                )
+                non_existing_as_error = ",".join(sorted(non_existing))
+                self.fail("no_such_option", value=non_existing_as_error, code=code)
+        elif attribute.type == attribute.DATE:
+            date_field = serializers.DateField()
+            internal_value = date_field.to_internal_value(value)
+        elif attribute.type == attribute.DATETIME:
+            date_field = serializers.DateTimeField()
+            internal_value = date_field.to_internal_value(value)
+        elif attribute.type == attribute.ENTITY:
+            internal_value = entity_internal_value(attribute, value)
+
+        return internal_value
+
+
+class AttributeConverter(AttributeFieldBase):
+    def __init__(self, context):
+        self.context = context
+        self.errors = []
+
+    def fail(self, key, **kwargs):
+        """
+        An implementation of fail that collects errors instead of raising them
+        """
+        try:
+            msg = self.default_error_messages[key]
+        except KeyError:
+            class_name = self.__class__.__name__
+            msg = MISSING_ERROR_MESSAGE.format(class_name=class_name, key=key)
+            raise AssertionError(msg)
+        message_string = msg.format(**kwargs)
+        self.errors.append(ErrorDetail(message_string, code=key))

oscarapi/utils/deprecations.py

@@ -0,0 +1,2 @@
+class RemovedInOScarAPI4(PendingDeprecationWarning):
+    pass