Opened 4 years ago

Closed 2 years ago

#17424 closed Bug (fixed)

Using exclude on a queryset with an annotate field gives attribute error.

Reported by: joseph.helfer@… Owned by: lrekucki
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: asendecka@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Generating a MySQL query with an annotate followed by an exclude which follows two foreignkey relations results in:

AttributeError: 'NoneType' object has no attribute 'startswith'

To reproduce this, you can do the following:
Create a new project, set the backend to mysql (and make a database), and create an app foo with this models.py:

from django.db import models

class Model1(models.Model):
    pass

class Model2(models.Model):
    model1 = models.ForeignKey(Model1)

class Model3(models.Model):
    model2 = models.ForeignKey(Model2)

Put foo in INSTALLED_APPS, run syncdb, and then run this in the django shell

from foo.models import *
from django.db.models import Count

print Model3.objects.annotate(bar=Count('id')).exclude(model2__model1__id=4).query

I'm running django 1.3.1 and python 2.6.7 on Mac OSX 10.7.1

This seems to be very similar to
https://code.djangoproject.com/ticket/12687
but, although that ticket is closed, the bug persists.

Attachments (3)

ticket17424.diff (640 bytes) - added by ethlinn 4 years ago.
Patch without tests (done mostly by lrekucki, I was just helping)
ticket17424_with_tests.diff (1.9 KB) - added by lrekucki 4 years ago.
Previous patch with regression test added.
ticket17424.2.diff (1.2 KB) - added by lrekucki 2 years ago.
Testcase rebased to master.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by glen.nelson.1@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I have hit a very similar case. I cannot upload and attach files due to IT restrcitions where I am. However I can give you the following source:

managers.py

from django.db import models
from django.db.models import Count

class TestRunAggregator(models.Manager):
    def get_query_set(self):
        return super(TestRunAggregator, self).get_query_set().annotate(tests_run=Count("testcaserun"))

models.py

from django.db import models
from managers import TestRunAggregator


class Location(models.Model):
    """ Represents the various locations a system
        can be found.
    """
    name = models.CharField(max_length=200, unique=True)
    description = models.TextField(null=True, blank=True)

    def __unicode__(self):
        return self.name

class System(models.Model):
    name = models.CharField(max_length=100, unique=True)
    description = models.TextField(null=True, blank=True)
    ipaddress = models.IPAddressField('IP address')
    owners = models.ManyToManyField(SystemOwner)
    location = models.ForeignKey(Location)
    objects = TestRunAggregator()

    def __unicode__(self):
        return self.name

    def all_owners(self):
        return ', '.join([owner.email for owner in self.owners.all()])

    @models.permalink
    def get_absolute_url(self):
        return ('system_view', [str(self.id)])

    def latest_version(self):
        if self.testcase_runs():
            return self.testcaserun_set.latest('end_date').build
        else:
            return 'unknown'

    def testcase_runs(self):
        return self.testcaserun_set.count()

class TestCaseRun(models.Model):
    start_date = models.DateTimeField('date started')
    end_date = models.DateTimeField('date finished')
    status = models.ForeignKey(TestCaseStatus)
    exception = models.TextField('exception message', null=True, blank=True)
    traceback = models.TextField(null=True, blank=True)
    cycles = models.IntegerField('number of cycles')
    build = models.TextField(null=True, blank=True)

    class Meta:
        unique_together = ('testcase', 'system', 'start_date', 'end_date')
        ordering = ['-start_date']

    def __unicode__(self):
        return self.testcase.name

    @classmethod
    def latest(cls, count=10):
        return cls.objects.all()[:count]

    @models.permalink
    def get_absolute_url(self):
        return ('testrun_view', [str(self.id)])

Example in the shell:

>>> import models as m

>>> m.System.objects.all()
[<System System1>, <System System2>, <System System3>]
>>> m.System.objects.all().exclude(location__name="City1")
[]
>>> # Not Shown here, but if you include connection, the exclude above never ran any SQL

>>> m.System.objects.all().filter(location__name="City1")
[<System System1>, <System System2>]
>>> m.SystemObjects.all().exclude(location__id=1)
[<System System3>]

comment:2 Changed 4 years ago by lrekucki

  • Triage Stage changed from Unreviewed to Accepted

Changed 4 years ago by ethlinn

Patch without tests (done mostly by lrekucki, I was just helping)

comment:3 Changed 4 years ago by ethlinn

  • Cc asendecka@… added

Changed 4 years ago by lrekucki

Previous patch with regression test added.

comment:4 Changed 4 years ago by lrekucki

  • Has patch set

comment:5 Changed 4 years ago by lrekucki

  • Owner changed from nobody to lrekucki

comment:6 Changed 4 years ago by glen.nelson.1@…

Tested this patch on my django project - it seems to have worked.

comment:7 follow-up: Changed 2 years ago by community@…

This is still an issue with 1.4.5 (not tested 1.5.x).

The patch provided fixes this exact problem, but is there any side effect I have to expect? Anybody verified this using tests?

And a last one: Will this patch be committed (or even backported to 1.4.x)?

Regards,
Marc

comment:8 in reply to: ↑ 7 Changed 2 years ago by lrekucki

Replying to community@…:

The patch provided fixes this exact problem, but is there any side effect I have to expect? Anybody verified this using tests?

I'm pretty sure it has no side effects. At the time of writing it, the whole Django test suite passed.

And a last one: Will this patch be committed (or even backported to 1.4.x)?

I hope so. What this issue needs is someone to test it with master (patch, run the test suite) and if everything is OK, mark it as "Ready for check-in". Sadly, only regressions and security fixes go to maintenance branches, so this has a chance to be included in 1.6.

comment:9 Changed 2 years ago by timo

  • Patch needs improvement set

Patch no longer applies cleanly.

Changed 2 years ago by lrekucki

Testcase rebased to master.

comment:10 Changed 2 years ago by lrekucki

The issue seems to be fixed by refactoring in changeset:01b9c3d5193fe61b82ae8b26242a13fdec22f211. On master the condition in promote_joins() still only checks if join_type is not LOUTER instead of explicitly checking for INNER (which could result in promoting non-join to join). I would recommend adding an explicit assertion there or at least a comment why the author assumes that join_type is not None.

comment:11 Changed 2 years ago by timo

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Test looks good. Marking as RFC for @akaariai to take a look.

comment:12 Changed 2 years ago by akaariai

I am going to investigate the join_type issue. Maybe there are other cases where None could be promoted accidentally, like combining two querysets with |.

comment:13 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c7739e30b20f55c2b055b12a628bfb5c2228ba4e:

Fixed #17424 -- annotate() + exclude() bug

The bug was already fixed by 01b9c3d5193fe61b82ae8b26242a13fdec22f211,
so only tests added.

At the same time promote_joins()'s uncoditional flag is gone, it isn't
needed for anything any more.

Note: See TracTickets for help on using tickets.
Back to Top