Repository: django-oscar/django-oscar

Issue:
Writing to products attributes with Product.attr can lead to dataloss
### Issue Summary

Writing to products attributes with `product.attr` can lead to data loss

### Steps to Reproduce

Create a productclass with 3 attributes, the second one you create must be optional.
Create a product and do not fill the second optional attribute. Now write to the non-optional attributes with code like this:

        p = Product.objects.get(pk=4)
        p.attr.subtitle = "lama"
        p.attr.available = False
        p.attr.save()

One of the 2 attributes will not be updated because the attr object will not be intialized. During save it will encounter the second attribute which is not on the object and trigger initialization. Initialization will overwrite one of the 2 values written and reset it to whatever is in the database.

### Technical details

This problem occurs with all python versions and all oscar versions.


Discussion:
As a workaround, before writing, just call
```
p.attr.initiate_attributes()
```
@specialunderwear could you please supply a failing test for this? I tried the below but it passes, which suggests I'm not correctly replicating the situation you describe:

```python
def test_attributes_initialised_before_write(self):
    product_class = factories.ProductClassFactory()
    product_class.attributes.create(name='a1', code='a1', required=True)
    product_class.attributes.create(name='a2', code='a2', required=False)
    product_class.attributes.create(name='a3', code='a3', required=True)
    product = factories.ProductFactory(product_class=product_class)
    product.attr.a1 = "v1"
    product.attr.a3 = "v3"
    product.attr.save()

    product.refresh_from_db()
    product.attr.a1 = "v2"
    product.attr.a3 = "v6"
    product.attr.save()

    product.refresh_from_db()
    assert product.attr.a1 == "v2"
    assert product.attr.a3 == "v6"
```
Unfortunately refresh_from_db is not enough to reset the attributes.
try:
```
product.attr.__dict__ = {}
```

Fix patch:
diff --git a/src/oscar/apps/catalogue/abstract_models.py b/src/oscar/apps/catalogue/abstract_models.py
--- a/src/oscar/apps/catalogue/abstract_models.py
+++ b/src/oscar/apps/catalogue/abstract_models.py
@@ -15,7 +15,7 @@
 from django.db.models.fields import Field
 from django.db.models.lookups import StartsWith
 from django.urls import reverse
-from django.utils.functional import cached_property
+from django.utils.functional import SimpleLazyObject, cached_property
 from django.utils.html import strip_tags
 from django.utils.safestring import mark_safe
 from django.utils.translation import get_language
@@ -435,7 +435,7 @@ class Meta:
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self.attr = ProductAttributesContainer(product=self)
+        self.attr = SimpleLazyObject(lambda: ProductAttributesContainer(product=self))
 
     def __str__(self):
         if self.title:
diff --git a/src/oscar/apps/catalogue/product_attributes.py b/src/oscar/apps/catalogue/product_attributes.py
--- a/src/oscar/apps/catalogue/product_attributes.py
+++ b/src/oscar/apps/catalogue/product_attributes.py
@@ -2,7 +2,7 @@
 from django.utils.translation import gettext_lazy as _
 
 
-class ProductAttributesContainer(object):
+class ProductAttributesContainer:
     """
     Stolen liberally from django-eav, but simplified to be product-specific
 
@@ -13,25 +13,16 @@ class ProductAttributesContainer(object):
 
     def __setstate__(self, state):
         self.__dict__ = state
-        self.initialised = False
 
     def __init__(self, product):
         self.product = product
-        self.initialised = False
-
-    def initiate_attributes(self):
         values = self.get_values().select_related('attribute')
         for v in values:
             setattr(self, v.attribute.code, v.value)
-        self.initialised = True
 
     def __getattr__(self, name):
-        if not name.startswith('_') and not self.initialised:
-            self.initiate_attributes()
-            return getattr(self, name)
         raise AttributeError(
-            _("%(obj)s has no attribute named '%(attr)s'") % {
-                'obj': self.product.get_product_class(), 'attr': name})
+            _("%(obj)s has no attribute named '%(attr)s'") % {'obj': self.product.get_product_class(), 'attr': name})
 
     def validate_attributes(self):
         for attribute in self.get_all_attributes():
diff --git a/src/oscar/apps/dashboard/catalogue/forms.py b/src/oscar/apps/dashboard/catalogue/forms.py
--- a/src/oscar/apps/dashboard/catalogue/forms.py
+++ b/src/oscar/apps/dashboard/catalogue/forms.py
@@ -286,7 +286,6 @@ def _post_clean(self):
         Set attributes before ModelForm calls the product's clean method
         (which it does in _post_clean), which in turn validates attributes.
         """
-        self.instance.attr.initiate_attributes()
         for attribute in self.instance.attr.get_all_attributes():
             field_name = 'attr_%s' % attribute.code
             # An empty text field won't show up in cleaned_data.


Test patch:
diff --git a/tests/integration/catalogue/test_attributes.py b/tests/integration/catalogue/test_attributes.py
--- a/tests/integration/catalogue/test_attributes.py
+++ b/tests/integration/catalogue/test_attributes.py
@@ -4,9 +4,33 @@
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase
 
+from oscar.apps.catalogue.models import Product
 from oscar.test import factories
 
 
+class TestContainer(TestCase):
+
+    def test_attributes_initialised_before_write(self):
+        # Regression test for https://github.com/django-oscar/django-oscar/issues/3258
+        product_class = factories.ProductClassFactory()
+        product_class.attributes.create(name='a1', code='a1', required=True)
+        product_class.attributes.create(name='a2', code='a2', required=False)
+        product_class.attributes.create(name='a3', code='a3', required=True)
+        product = factories.ProductFactory(product_class=product_class)
+        product.attr.a1 = "v1"
+        product.attr.a3 = "v3"
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        product.attr.a1 = "v2"
+        product.attr.a3 = "v6"
+        product.attr.save()
+
+        product = Product.objects.get(pk=product.pk)
+        assert product.attr.a1 == "v2"
+        assert product.attr.a3 == "v6"
+
+
 class TestBooleanAttributes(TestCase):
 
     def setUp(self):
@@ -52,7 +76,7 @@ def test_save_multi_option_value(self):
         product = factories.ProductFactory()
         # We'll save two out of the three available options
         self.attr.save_value(product, [self.options[0], self.options[2]])
-        product.refresh_from_db()
+        product = Product.objects.get(pk=product.pk)
         self.assertEqual(list(product.attr.sizes), [self.options[0], self.options[2]])
 
     def test_delete_multi_option_value(self):
@@ -60,7 +84,7 @@ def test_delete_multi_option_value(self):
         self.attr.save_value(product, [self.options[0], self.options[1]])
         # Now delete these values
         self.attr.save_value(product, None)
-        product.refresh_from_db()
+        product = Product.objects.get(pk=product.pk)
         self.assertFalse(hasattr(product.attr, 'sizes'))
 
     def test_multi_option_value_as_text(self):