-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotate counts deleted objects #178
Comments
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))
``` |
@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. |
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 👍 |
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 ( |
You are right, there are expressions that don't have the I have been testing. The expressions Max, Min, Sum, Avg and Count has I have edited the first patch by adding a check if the 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 ...
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 |
Both Property and Room are SafeDelete models with corresponding querysets and managers.
Property.objects.annotate(bc=Count('rooms', distinct=True))
generates the following code.
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
The text was updated successfully, but these errors were encountered: