Annotate counts deleted objects

[ihorm5]

Both Property and Room are SafeDelete models with corresponding querysets and managers.

Property.objects.annotate(bc=Count('rooms', distinct=True))
generates the following code.

SELECT "property_property"."id",
       COUNT(DISTINCT "property_room"."id") AS "bc"
FROM "property_property"
         LEFT OUTER JOIN "property_room" ON ("property_psproperty"."id" = "property_room"."property_id")
WHERE "property_property"."deleted" IS NULL
GROUP BY "property_property"."id"

It is easy to see that this code will count deleted rooms too, which is probably not an expected behavior?

It would be nice if possible if it would also generate:
AND "property_room"."deleted" is NULL

[Gagaro]

I'm not sure if this is possible to easily do. In the meantime, you can filter on the aggregate manually:

Property.objects.annotate(bc=Count('rooms', distinct=True, filter=Q(deleted__isnull=True))
```

[ihorm5]

@Gagaro Thanks, I am also not sure if there is an easy solution. For now, I have changed annotation to filter on deleted, would be cool if it worked this way out of the box. This way it will prevent accidental bugs when someone forgot about this.

[xdekasx]

Same problem here.

A first approach to override the default annotate() the better option I've achieved is the following:

from django.db.models import Q

class SafeDeleteQueryset(query.QuerySet):
    
    ...

    def annotate(self, *args, **kwargs):
        """        """

        self._not_support_combined_queries('annotate') 
        
        # <<<<<<<< Start of the patch
        if self.query._safedelete_visibility == DELETED_INVISIBLE:
            for key, value in kwargs.items():
                filter = None
                for expression in kwargs[key].source_expressions:
                    if not kwargs[key].filter:
                        filter = Q(**{'{}__deleted__isnull'.format(expression.name): True})
                    else:
                        filter = kwargs[key].filter & Q(**{'{}__deleted__isnull'.format(expression.name): True})

                kwargs[key].filter = filter

        print("kwargs: ", kwargs)  # Only for debugging and testing, will show the updated kwargs after the patch 
        # <<<<<<<< End of patch

        return self._annotate(args, kwargs, select=True)

Example of uses:

queryset = ModelA.objects.annotate(model_b_count=Count('model_b')

Without patch => {'model_b_count': Count(F(model_b))}
Patched kwargs => {'model_b_count': Count(F(model_b), filter=(AND: ('model_b__deleted__isnull', True)))}

At the moment I can't test it as much as I would like but in my scenario and project it seems that it solves the trouble.

I hope it helps.

Any comments or ways to improve it will be welcome 👍

[Gagaro]

Thansk for the contribution. I'm not sure there's a better way to do that, but I'm afraid of all the implication it could have on existing code as well. I don't think all expression have a filter attribute for example (Value() ?).

[xdekasx]

You are right, there are expressions that don't have the filter attribute.

I have been testing. The expressions Max, Min, Sum, Avg and Count has filter attribute and works properly.

I have edited the first patch by adding a check if the filter attribute is present on the expression to avoid crashes ( I would prefer another way but is the simplest I found for the current projects we are using the package. I understand that if the expressions does not have the filter attribute it is because they cannot be filtered or it does not make sense for them to be filtered 🤔 ). If there are another objection with the code or with another expression, please let me know it and I will update it.

I let the final patch in case someone is interested in using it or someone wants to test it. ( Remember to create your own SafeDeleteQueryset with this patch and inherit your Queryset classes to the created one, instead of the default one in the package to test it. ) .

Also, the new annotate() adds force_deleted param to disallow this new behavior similar as all(), delete(), etc. methods already in the module.

...
from django.core.exceptions import FieldDoesNotExist
from django.db.models.constants import LOOKUP_SEP

# To override the default SafeDeleteQueryset.
from safedelete.queryset import SafeDeleteQueryset as BaseSafeDeleteQueryset
...
  
class SafeDeleteQueryset(BaseSafeDeleteQueryset):
    def __init__(self, model=None, query=None, using=None, hints=None):
        super(SafeDeleteQueryset, self).__init__(model=model, query=query, using=using, hints=hints)

    def _is_valid_lookup_for_annotate(self, lookup: str) -> bool:
        """
        For the given model, check if the lookup is valid.
        The last lookup must be a relation to allow adding `__deleted__isnull`

        @param lookup:
        @return:
        """
        model = self.model

        for counter, part in enumerate(lookup.split(LOOKUP_SEP)):
            try:
                field = model._meta.get_field(part)
            except FieldDoesNotExist:
                break

            if not field.is_relation:
                break

            # Not final lookup, continue with next one
            if counter < len(lookup.split(LOOKUP_SEP)) - 1:
                model = field.related_model
                continue

            return True

        return False

    def annotate(self, force_deleted: bool = True, *args, **kwargs):
        """
        Return a query set in which the returned objects have been annotated
        with extra data or aggregations to filter the deleted ones.

        @param force_deleted: Force if deleted objects will be taken into account.

        """

        if not force_deleted or not self.query._safedelete_visibility == DELETED_INVISIBLE:
            return super().annotate(*args, **kwargs)

        for key, value in kwargs.items():
            print("====> key: ", key, "value: ", value)

            if not hasattr(kwargs[key], 'filter'):
                continue

            for expression in kwargs[key].get_source_expressions():
                print("expression: ", expression)

                if not self._is_valid_lookup_for_annotate(expression.name):
                    print("NOT VALID")
                    continue
                print("VALID: ")

                if not kwargs[key].filter:
                    extra_filter = Q(**{'{}__deleted__isnull'.format(expression.name): True})
                else:
                    extra_filter = kwargs[key].filter & Q(**{'{}__deleted__isnull'.format(expression.name): True})

                kwargs[key].filter = extra_filter

            print("kwargs[key].filter: ", kwargs[key].filter)

        return super().annotate(*args, **kwargs)

Tested with Django 3.2.7