Safe delete does not work as expected with OneToOneField

[lunika]

On a model, if we use a OneToOneField relation, we expect to not access to the related model record once deleted. But in fact we can still continue to access it.

I wrote test to prove it and understand what is expected on this commit : lunika@0ca7359

Thanks for your help

[Gagaro]

That's a complex one. The only way to currently fix that would be to overwrite the base manager on SafeDeleteModel, but that would cause a lot of other issues (for example Collector would not be able to find deleted instances during undelete). Base manager should always return all rows anyway.

Next thing we could try is overwrite the descriptors but I think this would mean needing to have a custom OneToOneField.

I also think we have the same issue on ForeignKey.

In your test, if we want to be logic, both author and book should be deleted as the on_delete is CASCADE. But this actually is another issue we have, ideally we would use the on_delete policies and it would solve a lot of problem (such as having to have a SOFT_DELETE_CASCADE policy), but it would requires quite a lot of refactoring / testing (and time...).

TL;DR: I'm not sure there is an easy fix for now.

[ColemanDunn]

I also came across this problem and made my own solution by dynamically creating unique constraints based on the 'deleted' field in the metaclass. Inspired by this post here: https://stackoverflow.com/questions/45687127/altering-unique-fields-of-inherited-django-model.

I create my own version of OneToOneField, currently called CustomOneToOneField, so I will be able to check if a given field is an instance of CustomOneToOneField and then add the appropriate unique constraints against the 'deleted' field (should probably by DELETED_BY_CASCADE_FIELD_NAME but I'm not using that and don't indent do so 'deleted' will work for now). I also had to slightly change the related_accessor_class and forward_related_accessor_class to work with actually many objects with the related key (but only one not set as deleted). There are probably some things I may be missing but I haven't ran into anything yet. The one caveat is that undeleting will break, but I am ok with that and currently do not undelete anywhere in my app. One solution would to just delete the current object and then undelete the the object you wish to undelete.

Might be cool to have a field like this in this built into library at some point.

here is my code:

class CustomReverseOneToOneDescriptor(ReverseOneToOneDescriptor):
    def __get__(self, instance, cls=None):
        """
        Get the related instance through the reverse relation.

        With the example above, when getting ``place.restaurant``:

        - ``self`` is the descriptor managing the ``restaurant`` attribute
        - ``instance`` is the ``place`` instance
        - ``cls`` is the ``Place`` class (unused)

        Keep in mind that ``Restaurant`` holds the foreign key to ``Place``.
        """
        if instance is None:
            return self

        # The related instance is loaded from the database and then cached
        # by the field on the model instance state. It can also be pre-cached
        # by the forward accessor (ForwardManyToOneDescriptor).
        try:
            rel_obj = self.related.get_cached_value(instance)
        except KeyError:
            related_pk = instance.pk
            if related_pk is None:
                rel_obj = None
            else:
                filter_args = self.related.field.get_forward_related_filter(instance)
                try:
                    rel_obj = self.get_queryset(instance=instance).get(**filter_args, deleted=None)
                except self.related.related_model.DoesNotExist:
                    rel_obj = None
                else:
                    # Set the forward accessor cache on the related object to
                    # the current instance to avoid an extra SQL query if it's
                    # accessed later on.
                    self.related.field.set_cached_value(rel_obj, instance)
            self.related.set_cached_value(instance, rel_obj)

        if rel_obj is None:
            raise self.RelatedObjectDoesNotExist(
                f"{instance.__class__.__name__} has no {self.related.get_accessor_name()}."
            )
        else:
            return rel_obj


class CustomForwardOneToOneDescriptor(ForwardOneToOneDescriptor):
    def get_object(self, instance):
        if self.field.remote_field.parent_link:
            deferred = instance.get_deferred_fields()
            # Because it's a parent link, all the data is available in the
            # instance, so populate the parent model with this data.
            rel_model = self.field.remote_field.model
            fields = [field.attname for field in rel_model._meta.concrete_fields]

            # If any of the related model's fields are deferred, fallback to
            # fetching all fields from the related model. This avoids a query
            # on the related model for every deferred field.
            if not any(field in fields for field in deferred):
                kwargs = {field: getattr(instance, field) for field in fields}
                obj = rel_model(**kwargs)
                obj._state.adding = instance._state.adding
                obj._state.db = instance._state.db
                return obj
        qs = self.get_queryset(instance=instance)
        # Assuming the database enforces foreign keys, this won't fail.
        return qs.get(self.field.get_reverse_related_filter(instance) & Q(deleted=None))

class CustomOneToOneField(models.OneToOneField):
    related_accessor_class = CustomReverseOneToOneDescriptor
    forward_related_accessor_class = CustomForwardOneToOneDescriptor

    description = "Custom one-to-one relationship"

    def __init__(self, to, on_delete, to_field=None, **kwargs):
        from django.db.models import OneToOneField

        super(OneToOneField, self).__init__(to, on_delete, to_field=to_field, **kwargs)


class ModelBaseMeta(ModelBase):
    def __new__(cls, name, bases, attrs):
        super_new = super().__new__

        if meta := attrs.get("Meta", None):
            if hasattr(meta, "abstract") and meta.abstract:
                return super_new(cls, name, bases, attrs)

            constraints = getattr(meta, "constraints", [])
            new_constraints = []
            for constraint in constraints:
                if isinstance(constraint, models.UniqueConstraint) and "deleted" not in constraint.fields:
                    new_constraint = models.UniqueConstraint(
                        fields=constraint.fields,
                        condition=Q(deleted=None),
                        name=f"{constraint.name}_deleted_is_null_existing",
                    )
                    new_constraints.append(new_constraint)
                    constraint.fields += ("deleted",)

            meta.constraints = constraints + new_constraints

        else:

            class Meta:
                constraints = []

            meta = Meta

        for field_name, field in attrs.items():
            if isinstance(field, models.OneToOneField) and not isinstance(field, CustomOneToOneField):
                raise TypeError(
                    "OneToOneField is not supported when inheriting from BaseModel. Use CustomOneToOneField instead"
                )
            if isinstance(field, CustomOneToOneField):
                meta.constraints.append(
                    models.UniqueConstraint(fields=[field_name, "deleted"], name=f"{name}_{field_name}_deleted")
                )
                meta.constraints.append(
                    models.UniqueConstraint(
                        fields=[field_name], condition=Q(deleted=None), name=f"{name}_{field_name}_deleted_is_null"
                    )
                )

        if meta.constraints:
            attrs["Meta"] = meta

        return super_new(cls, name, bases, attrs)


class BaseModel(SafeDeleteModel, metaclass=ModelBaseMeta):
    _safedelete_policy = SOFT_DELETE_CASCADE
    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

    class Meta:
        abstract = True