Two foreign-key fields should not be nullable

[byshen]

Issue Summary

The foreign-key field should not be nullable in the following cases. Because when the associated attributes or reviews are deleted with CASCADE effect, thus the field will not be null in normal operations. If the foreign key is indeed null, it may reflect that errors already happen before.

I found two cases.

• The product_class field in class AbstractProductAttribute.
• The review's product of the AbstractProductReview.

In src/oscar/apps/catalogue/abstract_models.py,

class AbstractProductAttribute(models.Model):
    """
    Defines an attribute for a product class. (For example, number_of_pages for
    a 'book' class)
    """
    product_class = models.ForeignKey(
        'catalogue.ProductClass',
        blank=True,
        on_delete=models.CASCADE,
        related_name='attributes',
        null=True,
        verbose_name=_("Product type"))

In src/oscar/apps/catalogue/reviews/abstract_models.py

class AbstractProductReview(models.Model):
    """
    A review of a product

    Reviews can belong to a user or be anonymous.
    """

    product = models.ForeignKey(
        'catalogue.Product', related_name='reviews', null=True,
        on_delete=models.CASCADE)

The product attributes can only be added/edited after we create a product type. If the product is deleted, the product attributes will also be deleted due to the CASCADE deletion. Therefore, the product_class should not be nullable at anytime for a product attribute.

Also, when the product is deleted, all the reviews of the product will be deleted with the CASCADE method #2333. The product for this review should not be nullable at any time. If the product is indeed a null value, some errors may have occurred. I would suggest removing null=True in this product field.

If this makes sense to you, I can create a PR to remove null=True for these field. Thanks!

[byshen]

@solarissmoke Could you take a look when you are available? Thanks a lot :D

[solarissmoke]

See my comment here which I think applies to these fields as well - this is a deliberate decision to aid migration of data.

[byshen]

Hi @solarissmoke, thanks for your reply.

I think these two fields are a bit different as they are foreignkey fields. Most of time there will not be NULL values, and they would be deleted with the CASCADE effect. So the cost of migration would be small.

The benefit of add the not NULL constraint would be that we can simplify the code logics by checking the data integrity at DB level. For example, some commits are specifically added to walk around the exceptions.

Your feedbacks are appreciated. Thank you in advance.

[sergey-gru]

Hello dear developers. I am getting an error when trying to save an object.

In [27]: b=Product(title="Product3")

In [28]: b.save()

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Input In [28], in <cell line: 1>()
----> 1 b.save()

File ~/opt/anaconda3/lib/python3.9/site-packages/oscar/apps/catalogue/abstract_models.py:540, in AbstractProduct.save(self, *args, **kwargs)
    538     self.slug = slugify(self.get_title())
    539 super().save(*args, **kwargs)
--> 540 self.attr.save()

File ~/opt/anaconda3/lib/python3.9/site-packages/oscar/apps/catalogue/product_attributes.py:111, in ProductAttributesContainer.save(self)
    110 def save(self):
--> 111     for attribute in self.get_all_attributes():
    112         if hasattr(self, attribute.code):
    113             value = getattr(self, attribute.code)

File ~/opt/anaconda3/lib/python3.9/site-packages/oscar/apps/catalogue/product_attributes.py:102, in ProductAttributesContainer.get_all_attributes(self)
    101 def get_all_attributes(self):
--> 102     return self.product.get_product_class().attributes.all()

AttributeError: 'NoneType' object has no attribute 'attributes'

Indeed, I did not set the product_class_id parameter when creating the product. I just didn't know about it. And I didn't have any classes created.

Note this behavior is different than if I set the wrong product_class_id

In [93]: c=Product(product_class_id=25)

In [94]: c.save()
---------------------------------------------------------------------------
IntegrityError                            Traceback (most recent call last)
...
IntegrityError: FOREIGN KEY constraint failed

I suggest replacing this code

#oscar.apps.catalogue.product_attributes.py
    def get_all_attributes(self):
        return self.product.get_product_class().attributes.all()

with this snippet

    def get_all_attributes(self):
        pc = self.product.get_product_class()
        if pc is None:
            return type(self.product).objects.none()
        else:
            return pc.attributes.all()

In this case, this function will not fail

    def save(self):
        for attribute in self.get_all_attributes():
            if hasattr(self, attribute.code):
                value = getattr(self, attribute.code)
                if attribute.code not in self._dirty:
                    # Make sure that if a value comes from a parent product, it is not
                    # copied to the child, we do this by checking if a value has been
                    # changed, which would not be the case if the value comes from the
                    # parent.
                    # for attributes are are set explicitly (_dirty), this check is not
                    # needed and should always be saved.
                    try:
                        attribute_value_current = self.get_value_by_attribute(attribute)
                        if 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)

however, I'm not sure about the correctness of this code in this case

    def get_attribute_by_code(self, code):
        return self.get_all_attributes().get(code=code)