When I want to define my business logic, I'm struggling finding the right way to do this, because I often both need a property AND a custom queryset to get the same info. In the end, the logic is duplicated.
First, after defining my class, I naturally start writing a simple property for data I need:
class PickupTimeSlot(models.Model):
@property
def nb_bookings(self) -> int:
""" How many times this time slot is booked? """
return self.order_set.validated().count()
Then, I quickly realise that calling this property while dealing with many objects in a queryset will lead to duplicated queries and will kill performance (even if I use prefetching, because filtering is called again). So I solve the problem writing a custom queryset with annotation:
class PickupTimeSlotQuerySet(query.QuerySet):
def add_nb_bookings_data(self):
return self.annotate(db_nb_bookings=Count('order', filter=Q(order__status=Order.VALIDATED)))
And then, I end up with 2 problems:
nb_bookings for both the property and the annotation don't work. This forces me, when using my object, to think about how the data is generated, to call the right attribute name (let's say pickup_slot.nb_bookings (property) or pickup_slot.db_nb_bookings (annotation) )This seems poorly designed to me, and I'm pretty sure there is a way to do better. I'd need a way to always write pickup_slot.nb_bookings and having a performant answer, always using the same business logic.
I was thinking of completely removing the property and keeking custom queryset only. Then, for single objects, wrapping them in querysets just to be able to call add annotation data on it. Something like:
pickup_slot = PickupTimeSlot.objects.add_nb_bookings_data().get(pk=pickup_slot.pk)
Seems pretty hacky and unnatural to me. What do you think?
To avoid any duplication, one option could be:
class PickupTimeSlotManager(models.Manager):
def get_queryset(self):
return super().get_queryset().annotate(
db_nb_bookings=Count(
'order', filter=Q(order__status=Order.VALIDATED)
)
)
from django.db import models
from .managers import PickupTimeSlotManager
class PickupTimeSlot(models.Model):
...
# Add custom manager
objects = PickupTimeSlotManager()
advantage: the calculated properties is transparently added to any queryset; no further action is required to use it
disadvantage: the computational overhead occurs even when the calculated property is not used
Let this be the alternative way to archive what you want:
Since I usually add the prefetch_related every time I write a queryset. So when I face this problem, I will use Python to solve this problem.
I'm going to use Python to loop and count the data for me instead of doing it in SQL way.
class PickupTimeSlot(models.Model):
@property
def nb_bookings(self) -> int:
""" How many times this time slot is booked? """
orders = self.order_set.all() # this won't hit the database if you already did the prefetch_related
validated_orders = filter(lambda x: x.status == Order.VALIDATED, orders)
return len(validated_orders)
And most important thing, prefetch_related:
time_slots = PickupTimeSlot.objects.prefetch_related('order_set').all()
You may have a question that why I didn't prefetch_related with filtered queryset so Python doesn't need to filter again like:
time_slots = PickupTimeSlot.objects.prefetch_related(
Prefetch('order_set', queryset=Order.objects.filter(status=Order.VALIDATED))
).all()
The answer is there are sometimes that we also need the other information from orders as well. Doing the first way will not cost anything more if we're going to prefetch it anyway.
Hope this more or less helps you. Have a nice day!
I don't think there is a silver bullet here. But I use this pattern in my projects for such cases.
class PickupTimeSlotAnnotatedManager(models.Manager):
def with_nb_bookings(self):
return self.annotate(
_nb_bookings=Count(
'order', filter=Q(order__status=Order.VALIDATED)
)
)
class PickupTimeSlot(models.Model):
...
annotated = PickupTimeSlotAnnotatedManager()
@property
def nb_bookings(self) -> int:
""" How many times this time slot is booked? """
if hasattr(self, '_nb_bookings'):
return self._nb_bookings
return self.order_set.validated().count()
In code
qs = PickupTimeSlot.annotated.with_nb_bookings()
for item in qs:
print(item.nb_bookings)
This way I can always use property, if it is part of annotated queryset it will use annotated value if not it will calculate it. This approach guaranties that I will have full control of when to make queryset "heavier" by annotating it with required values. If I don't need this I just use regular PickupTimeSlot.objects. ...
Also if there are many such properties you could write decorator that will wrap property and simplify code. It will work as cached_property decorator, but instead it will use annotated value if it is present.
Based on your different good answers, I decided to stick with annotations and properties. I created a cache mechanism to make it transparent about the naming. The main advantage is to keep the business logic in one place only. The only drawback I see is that an object could be called from database a second time to be annotated. Performance impact stays minor IMO.
Here is a full example with 3 different attributes I need in my model. Feel free to comment to improve this.
class PickupTimeSlotQuerySet(query.QuerySet):
def add_booking_data(self):
return self \
.prefetch_related('order_set') \
.annotate(_nb_bookings=Count('order', filter=Q(order__status=Order.VALIDATED))) \
.annotate(_nb_available_bookings=F('nb_max_bookings') - F('_nb_bookings')) \
.annotate(_is_bookable=Case(When(_nb_bookings__lt=F('nb_max_bookings'),
then=Value(True)),
default=Value(False),
output_field=BooleanField())
) \
.order_by('start')
class PickupTimeSlot(models.Model):
objects = SafeDeleteManager.from_queryset(PickupTimeSlotQuerySet)()
nb_max_bookings = models.PositiveSmallIntegerField()
@annotate_to_property('add_booking_data', 'nb_bookings')
def nb_bookings(self):
pass
@annotate_to_property('add_booking_data', 'nb_available_bookings')
def nb_available_bookings(self):
pass
@annotate_to_property('add_booking_data', 'is_bookable')
def is_bookable(self):
pass
def annotate_to_property(queryset_method_name, key_name):
"""
allow an annotated attribute to be used as property.
"""
from django.apps import apps
def decorator(func):
def inner(self):
attr = "_" + key_name
if not hasattr(self, attr):
klass = apps.get_model(self._meta.app_label,
self._meta.object_name)
to_eval = f"klass.objects.{queryset_method_name}().get(pk={self.pk}).{attr}"
value = eval(to_eval, {'klass': klass})
setattr(self, attr, value)
return getattr(self, attr)
return property(inner)
return decorator