Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25882 closed Bug (fixed)

Deletion on ForeignKey raises TypeError

Reported by: Markus Gerards Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.9
Severity: Release blocker Keywords: mysql
Cc: raphael.merx@… 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

Consider following model constellation:

class Catalog(models.Model):
	name = models.CharField(max_length=255)
	...

class Reader(models.Model):
	name = models.CharField(max_length=255)
	catalog = models.ForeignKey(Catalog)
	...

class ReaderHasMedia(models.Model):
	reader = models.ForeignKey(Reader)
	...

Now in some cases I need to perform following command

ReaderHasMedia.objects.filter(catalog=Catalog.objects.get(pk=123)).delete()

With this command, I get following TypeError:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/django/db/models/query.py", line 600, in delete
    deleted, _rows_count = collector.delete()
  File "/usr/local/lib/python2.7/site-packages/django/db/models/deletion.py", line 293, in delete
    deleted_counter[qs.model._meta.label] += count
TypeError: unsupported operand type(s) for +=: 'int' and 'NoneType'

With Django 1.8.7 there wasn't a count variable in deletion.py and so I believe, this is a fresh bug with Django 1.9...

Change History (13)

comment:1 Changed 4 years ago by Simon Charette

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

It looks like the fix for #16891 didn't account for the fact django.db.models.sql.subqueries.DeleteQuery.delete_qs can return None.

Not sure if this should be fixed at the Collector or DeleteQuery level.

comment:2 Changed 4 years ago by Simon Charette

Giving it more thought I strongly believe this should be fixed in DeleteQuery.delete_qs by simply returning 0 instead of None.

comment:3 Changed 4 years ago by Raphael Merx

In your example, ReaderHasMedia does not have a FK to Catalog, so ReaderHasMedia.objects.filter(catalog=Catalog.objects.get(pk=123)).delete() isn't valid.

comment:4 Changed 4 years ago by Raphael Merx

Cc: raphael.merx@… added

comment:5 in reply to:  3 Changed 4 years ago by Markus Gerards

Replying to raphaelmerx:

In your example, ReaderHasMedia does not have a FK to Catalog, so ReaderHasMedia.objects.filter(catalog=Catalog.objects.get(pk=123)).delete() isn't valid.

Ah! Sorry - I meant: ReaderHasMedia.objects.filter(reader__catalog=Catalog.objects.get(pk=123)).delete()

comment:6 Changed 4 years ago by Raphael Merx

I can't capture the bug in a test, neither on master nor on 1.9. Am I understanding something wrong?

  • tests/delete_regress/models.py

    diff --git a/tests/delete_regress/models.py b/tests/delete_regress/models.py
    index f0145de..6d77c31 100644
    a b class OrderedPerson(models.Model): 
    139139
    140140    class Meta:
    141141        ordering = ['name']
     142
     143
     144class Catalog(models.Model):
     145    name = models.CharField(max_length=255)
     146
     147class Reader(models.Model):
     148    name = models.CharField(max_length=255)
     149    catalog = models.ForeignKey(Catalog, models.CASCADE)
     150
     151class ReaderHasMedia(models.Model):
     152    reader = models.ForeignKey(Reader, models.CASCADE)
  • tests/delete_regress/tests.py

    diff --git a/tests/delete_regress/tests.py b/tests/delete_regress/tests.py
    index 2128733..8b36509 100644
    a b class OrderedDeleteTests(TestCase): 
    345345        OrderedPerson.objects.create(name='Bob', lives_in=h)
    346346        OrderedPerson.objects.filter(lives_in__address='Foo').delete()
    347347        self.assertEqual(OrderedPerson.objects.count(), 0)
     348
     349class DoubleForeignKeyTests(TestCase):
     350    def test_double_foreign_key(self):
     351        from .models import Catalog, Reader, ReaderHasMedia
     352        c = Catalog.objects.create(name='aaa')
     353        r = Reader.objects.create(name='bbb', catalog=c)
     354        ReaderHasMedia.objects.create(reader=r)
     355        ReaderHasMedia.objects.filter(reader__catalog=Catalog.objects.get(pk=1)).delete()

comment:7 Changed 4 years ago by Simon Charette

From looking at the stacktrace it looks like you need to run your tests against a backend that doesn't have the update_can_self_select feature.

The only core backend that has this feature turned off is MySQL.

comment:8 Changed 4 years ago by Simon Charette

Keywords: mysql added

comment:9 Changed 4 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:10 Changed 4 years ago by Simon Charette

Created a PR.

@raphaelmerx thanks for the investigation. I looks like the easiest way to reproduce the issue on MySQL is the following.

class A(models.Model):
    text = models.TextField()

class B(models.Model):
    a = models.ForeignKey(A, models.CASCADE)

B.objects.delete(a__text='missing')

I suppose the reported test case didn't work because you created a Reader object. The following should do:

Reader.objects.filter(catalog__name='missing')

In order to trigger the bug you have to generate a delete query that issues a join but doesn't match any row on MySQL.

comment:11 Changed 4 years ago by Tim Graham

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:12 Changed 4 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 8035cee9:

Fixed #25882 -- Prevented fast deletes matching no rows from crashing on MySQL.

Thanks to Trac aliases gerricom for the report, raphaelmerx for the
attempts to reproduce and Sergey Fedoseev and Tim for the review.

Refs #16891

comment:13 Changed 4 years ago by Simon Charette <charette.s@…>

In c402db2e:

[1.9.x] Fixed #25882 -- Prevented fast deletes matching no rows from crashing on MySQL.

Thanks to Trac aliases gerricom for the report, raphaelmerx for the
attempts to reproduce and Sergey Fedoseev and Tim for the review.

Refs #16891

Backport of 8035cee92293f3319919c8248c7787ba43c05917 from master

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