W1ldPo1nter / django-queryable-properties

Write Django model properties that can be used in database queries.
BSD 3-Clause "New" or "Revised" License
71 stars 1 forks source link

Is it possible to create an annotation-based property based on another annotation based property #12

Closed mnetkin closed 8 months ago

mnetkin commented 9 months ago

Let say I have 3 models Payment, Return and Report. Payment has a Many to One relationship to Return and Return has a Many to One relationship Report

I have a property defined as follows:

@queryable_property(annotation_based=True)
@classmethod
def total_refunded_amount(cls):
    annotator=Sum(
        'payment_set__amount',
        filter=(
                Q(payment_set__amount__gt=0) &
        default=0.00
    )
    return annotator

I tried to create the following property for the Report model

@queryable_property(annotation_based=True)
@classmethod
def amount_refunded(cls):
    return Sum('return__total_refunded_amount')

But i get the error django.core.exceptions.FieldError: Cannot resolve keyword 'payment_set' into field. It looks like it is trying to get payment_set in the context of the Report model instead of the Returns Model

W1ldPo1nter commented 9 months ago

It is possible to use queryable properties inside of other queryable properties in general.

I'd like to take a deeper look at your case if you could provide the following information:

With this information, I can try to reproduce your error and check whether it's a bug.

mnetkin commented 9 months ago

Django Version 4.1.10 Payment Model:

class Payment(model.Model):
 amount = models.DecimalField(decimal_places=2, max_digits=10, default=0.0)
 return = TenantForeignKey(WalmartReturn, on_delete=models.SET_NULL, null=True, related_name='payment_set')

Retun Model:

class Return(model.Model):
 report = TenantForeignKey(Report, on_delete=models.SET_NULL, null=True)

@queryable_property(annotation_based=True)
@classmethod
def total_refunded_amount(cls):
    annotator=Sum(
        'payment_set__amount',
        filter=(
                Q(payment_set__amount__gt=0) &
        default=0.00
    )
    return annotator

Report Model:

class Report(model.Model):
...

@queryable_property(annotation_based=True)
@classmethod
def amount_refunded(cls):
    return Sum('return__total_refunded_amount')

The code I am trying to run is Report.objects.first().amount_refunded orReport.objects.filter(amount_refunded__gt=0)

W1ldPo1nter commented 9 months ago

Okay, I've found the issue. The problem isn't that the "inner" property itself is resolved incorrectly, but that the filter part of the inner part isn't resolved correctly. Django seems to apply aggregate filters like this differently than regular filters, which is why django-queryable-properties can't inject the outer relation path. I'll check if this can be fixed on django-queryable-properties' side.

In the meantime, you can work around this by using a subquery with the aggregate for the inner property. I had to modify your example a bit as some parts of your code aren't valid:

For the following example, I've renamed the return field to ret and used regular ForeignKeys as I don't know where TenantForeignKey is from. I've also used AggregateProperty to simplify the code. Feel free to adapt.

from queryable_properties.properties import AggregateProperty, SubqueryFieldProperty

class Payment(models.Model):
    amount = models.DecimalField(decimal_places=2, max_digits=10, default=0)
    ret = models.ForeignKey('Return', on_delete=models.SET_NULL, null=True, related_name='payment_set')

class Return(models.Model):
    report = models.ForeignKey('Report', on_delete=models.SET_NULL, null=True)

    total_refunded_amount = SubqueryFieldProperty(
        lambda: (Payment.objects.filter(ret=models.OuterRef('pk'), amount__gt=0)
                                .annotate(total=models.Sum('amount', default=0))),
        field_name='total',
    )

class Report(models.Model):
    amount_refunded = AggregateProperty(models.Sum('return__total_refunded_amount'))
W1ldPo1nter commented 8 months ago

I've implemented a fix in a dedicated branch, which allows filter clauses of aggregates to be resolved correctly even when used via relations. I plan on merging this soon and then releasing a new version that contains the fix.

However, this will not mean that your original code is going to work. Django itself doesn't allow nested aggregates (i.e. it's not an issue with django-queryable-properties), so you'll probably run into this exception. I'd recommend to implement one of the properties using a subquery like in the example shown in my last comment here.

W1ldPo1nter commented 8 months ago

The fix that allows filter clauses of aggregates to be resolved correctly is part of version 1.9.1, which was released just now.