current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote -1 down vote favorite

I have the following models:

class Donation(models.Model):
    donation_amount = models.DecimalField(max_digits=9, decimal_places=2)
    donor_name = models.CharField(max_length=200)


class Charge(models.Model):
    donation = models.ForeignKey(Donation)
    error = models.TextField(null=True, blank=True)

And I have a complex query that should do one of the following:

Get all donations or all donations which has at least one failed charge.

I came up with the following solution:

# get all donations with charge prefetched
all_donations = Donation.objects.all().prefetch_related('charge_set')

# sometimes filter for failed ones
if need_failed_charges:
  all_donations = all_donations.annotate(count=Count('charge')) \
            .filter(charge__error__isnull=False, count__gt=0)

Here I use the count to track the number of charges I have per donation, I filter them afterward to check if I have at least one.

I think there is a better way to express this, but as I'm still new to the django world I cannot see it.

What do you think?

share|improve this question
up vote 3 down vote accepted

  1. It's easier to answer this kind of question if you give us more code. I would have preferred the question to contain (possibly cut-down) code for your models, not just a summary description of the models.

  2. The comment for the error field says "can be null/blank" but your code only tests for NULL, not for blank. If the comment is correct, then your code is buggy; conversely, if the code is correct, then the comment is wrong.

  3. The question says, "I filter them afterward to check if I have more than one" but the code says, count__gt=0, so it is actually filtering for more than zero failed charges. If the text of question is right then the code is buggy.

  4. There's no need for the count annotation — if you add a filter on a one-to-many relation then you only get results where there is at least one record that matches the filter. (In SQL terms, Django's filters on one-to-many relations are implemented as inner joins.)

    So all that's needed is:

    # Donations with at least one failed charge.
Donation.objects.filter(charge__error__isnull=False)

    You can check this by looking at the SQL that is generated by Django's ORM:

    >>> print(Donation.objects.filter(charge__error__isnull=False).query)
SELECT "myapp_donation"."id" FROM "myapp_donation"
  INNER JOIN "myapp_charge"
  ON ( "myapp_donation"."id" = "myapp_charge"."donation_id" )
  WHERE "myapp_charge"."error" IS NOT NULL

    (The count annotation would, however, be necessary if the requirement were to select donations with more than one failed charge, as stated in the question.)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.