From 353d73592bb7f2fa3296a6f491c30b1ab63bf7fc Mon Sep 17 00:00:00 2001 From: Lars van de Kerkhof Date: Thu, 28 Sep 2023 10:18:01 +0200 Subject: [PATCH] Implemented faster attributes save --- oscarapi/serializers/admin/product.py | 5 +- oscarapi/serializers/fields.py | 98 +++++++++--------------- oscarapi/serializers/product.py | 106 +++++++++++++++++++++++++- oscarapi/tests/unit/testproduct.py | 2 + oscarapi/utils/attributes.py | 78 +++++++++++++++++++ oscarapi/utils/deprecations.py | 2 + 6 files changed, 229 insertions(+), 62 deletions(-) create mode 100644 oscarapi/utils/attributes.py create mode 100644 oscarapi/utils/deprecations.py diff --git a/oscarapi/serializers/admin/product.py b/oscarapi/serializers/admin/product.py index ee2869c0..3984bef4 100644 --- a/oscarapi/serializers/admin/product.py +++ b/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 diff --git a/oscarapi/serializers/fields.py b/oscarapi/serializers/fields.py index 1e0599f8..dd9aa18a 100644 --- a/oscarapi/serializers/fields.py +++ b/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,30 +105,46 @@ 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 get_data_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 @@ -134,49 +152,9 @@ def to_internal_value(self, data): # noqa 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.get_data_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: diff --git a/oscarapi/serializers/product.py b/oscarapi/serializers/product.py index b480d6f3..cc71d01d 100644 --- a/oscarapi/serializers/product.py +++ b/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 diff --git a/oscarapi/tests/unit/testproduct.py b/oscarapi/tests/unit/testproduct.py index 8abedbf6..3a0771dd 100644 --- a/oscarapi/tests/unit/testproduct.py +++ b/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, diff --git a/oscarapi/utils/attributes.py b/oscarapi/utils/attributes.py new file mode 100644 index 00000000..fdef848c --- /dev/null +++ b/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)) diff --git a/oscarapi/utils/deprecations.py b/oscarapi/utils/deprecations.py new file mode 100644 index 00000000..88ccef88 --- /dev/null +++ b/oscarapi/utils/deprecations.py @@ -0,0 +1,2 @@ +class RemovedInOScarAPI4(PendingDeprecationWarning): + pass