W1ldPo1nter/django-queryable-properties

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

Closed this issue · 10 comments

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

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:

  • your Django version
  • a more complete excerpt of your models (i.e. the three model classes with the relevant fields (at least amount and the relation fields) and queryable properties)
  • the code you're trying to execute that actually raises the error

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

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)

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:

  • the line return = TenantForeignKey(...) isn't valid because return is a keyword
  • your implementation of total_refunded_amount seems to be missing parts as there's a & followed by nothing and a closing parenthesis is missing

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'))

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.

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

@W1ldPo1nter That would be nice to have in the docs with examples :) That is really powerful feature

Thanks

@Anton-Shutik What exactly is missing in the docs from your point of view? The most vital part of the fix here was to use a SubqueryFieldProperty for the inner property, which itself is documented along with an example. The docs regarding implementation of annotaters also state in a note at the end that

The returned annotation object may reference the names of other annotatable queryable properties on the same model, which will be resolved accordingly.

This note should obviously be improved as annotations can also reference other annotatable queryable properties on related models, which is the second part of the solution here. But once this note is improved, both parts of the solution here are part of the docs, so is there anything else you'd like to see documented?

I mean usage of annotated properties inside of other annotated properties, smth like that:

class Product(models.Model):

    @queryable_property
    def is_composite(self):
        return self.kit_items.count() > 0

    @is_composite.annotater
    def is_composite(self):
        return Exists(KitItem.objects.filter(kit=OuterRef("pk")))
    
    @queryable_property
    def is_eligible_for_sell(self):
        if self.display_status == DISPLAY_STATUS.UNPUBLISHED:
            return False
        return not self.is_composite

    @is_eligible_for_sell.annotater
    def is_eligible_for_sell(self):
        return Case(
            When(display_status=DISPLAY_STATUS.UNPUBLISHED, then=Value(False)),
            ##########
            ########## This uses another annotated property
            default= ~ F("is_composite"), 
            ##########
            output_field=models.BooleanField(),
        )

Probably it's not related to the issue itself, but I'd found it helpful, since issue's title describes what it does.

Anyway, thanks for the app :)

That's pretty much what the note mentioned above says, but I can look into providing an explicit example in the docs.

By the way, your is_composite property could just be a one-liner using a SubqueryExistenceCheckProperty:

from queryable_properties.properties import SubqueryExistenceCheckProperty


class Product(models.Model):
    is_composite = SubqueryExistenceCheckProperty(lambda: KitItem.objects.filter(kit=OuterRef("pk")))

Phew, this way even better :)

Didn't know that, thank you!