Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#12247 closed (fixed)

update on empty queryset in which the update kwargs refer to inherited columns update all rows of base table

Reported by: jsmullyan Owned by: matiasb
Component: Database layer (models, ORM) Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A queryset that happens to return nothing if evaluated, when updated, may erroneously update all rows if the queryset refers to an inherited model and the update columns are in the base table. I'm testing with django 1.1.X on rev 11752. This is a bug with potentially very serious consequences. (Note: I believe this bug to be distinct, although similar in its effects, to #12142.)

Here is test code to produce the problem. One of the below tests will fail (test_empty_update_with_inheritance).

#models.py

from django.db import models

# Create your models here.

class A(models.Model):
    x=models.IntegerField(default=10)

class B(models.Model):
    a=models.ForeignKey(A)
    y=models.IntegerField(default=10)

class C(models.Model):
    y=models.IntegerField(default=10)

class D(C):
    a=models.ForeignKey(A)

# tests.py
from django.test import TestCase

from models import A, B, D

class SimpleTest(TestCase):
    def setUp(self):
        self.a1=A.objects.create()
        self.a2=A.objects.create()
        for x in range(20):
            B.objects.create(a=self.a1)
            D.objects.create(a=self.a1)

    def tearDown(self):
        B.objects.all().delete()
        A.objects.all().delete()
        
        
    def test_nonempty_update(self):
        """
        Test that update changes that right number of rows for a nonempty queryset
        """
        num_updated=self.a1.b_set.update(y=100)
        self.failUnlessEqual(num_updated, 20)
        cnt=B.objects.filter(y=100).count()
        self.failUnlessEqual(cnt, 20)

    def test_empty_update(self):
        """
        Test that update changes that right number of rows for an empty queryset
        """

        num_updated=self.a2.b_set.update(y=100)
        self.failUnlessEqual(num_updated, 0)
        cnt=B.objects.filter(y=100).count()
        self.failUnlessEqual(cnt, 0)

    def test_nonempty_update_with_inheritance(self):
        """
        Test that update changes that right number of rows for an empty queryset
        when the update affects only a base table
        """

        num_updated=self.a1.d_set.update(y=100)
        self.failUnlessEqual(num_updated, 20)
        cnt=D.objects.filter(y=100).count()
        self.failUnlessEqual(cnt, 20)


    def test_empty_update_with_inheritance(self):
        """
        Test that update changes that right number of rows for an empty queryset
        when the update affects only a base table
        """

        num_updated=self.a2.d_set.update(y=100)
        self.failUnlessEqual(num_updated, 0)
        cnt=D.objects.filter(y=100).count()
        self.failUnlessEqual(cnt, 0)

Attachments (2)

12247-1.diff (3.2 KB) - added by matiasb 4 years ago.
Fix for related updates query construction
12247-jkocherhans.diff (3.2 KB) - added by jkocherhans 4 years ago.
Moved the models and tests from matiasb's patch into the existing update test and changed the formatting to follow pep8.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by nessita

  • Owner changed from nobody to nessita
  • Status changed from new to assigned

comment:3 Changed 4 years ago by nessita

  • Owner nessita deleted
  • Status changed from assigned to new

comment:4 Changed 4 years ago by matiasb

  • Owner set to matiasb
  • Status changed from new to assigned

Changed 4 years ago by matiasb

Fix for related updates query construction

comment:5 Changed 4 years ago by matiasb

After long debugging to find the issue, added patch to manage empty related_ids when building related_updates queries.
Thanks to nessita for the previous debugging info and jsmullyan for the provided tests.

comment:6 Changed 4 years ago by matiasb

  • Has patch set

comment:7 Changed 4 years ago by jkocherhans

The patch looks pretty good, and seems to fix the problem, but I wonder if the bug isn't another level down. This seems to be triggered when self.related_ids is [], so we're essentially calling query.add_filter(('pk__in', [])). It seems like pk__in=[] should essentially be a no-op, but it sounds like it's actually selecting everything.

Changed 4 years ago by jkocherhans

Moved the models and tests from matiasb's patch into the existing update test and changed the formatting to follow pep8.

comment:8 Changed 4 years ago by kmtracey

#13228 reported this again.

comment:9 Changed 4 years ago by russellm

@jkocherhans - add_filter(pkin=[]) *is* a no-op (or rather, it raises EmptyResultSet). The issue here is that the "if self.related_ids" condition means that you *aren't* adding the [] filter, which means that you are left with a default "update everything" filter. Changing the condition to "if self.related_ids is None" means that the [] filter will be added, and thereby shortcut the update into a no-op.

comment:10 Changed 4 years ago by russellm

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

(In [12910]) Fixed #12247 -- Corrected the way update queries are processed when the update only refers to attributes on a base class. Thanks to jsmullyan for the report, and matiasb for the fix.

comment:11 Changed 4 years ago by russellm

(In [12911]) [1.1.X] Fixed #12247 -- Corrected the way update queries are processed when the update only refers to attributes on a base class. Thanks to jsmullyan for the report, and matiasb for the fix.

Backport of r12910 from trunk.

comment:12 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.