Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#11311 closed (fixed)

Deleting model instance with a string id and m2m relation fails

Reported by: ronny Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: delete string pk m2m
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Say a model has a CharField as its primary key and the model also has a many to many relation to some other "normal" model, deleting an instance of the model with string primary key like "abc" fails with an exception, but not with a string id that can be converted into an int, e.g. "1". Seems like the code assumes that primary keys are always int()?

Using django from svn trunk r10982.

The following is a very small test case (models.py) that demonstrate the issue.

from django.db import models

class Line(models.Model):
    name = models.CharField(max_length=100)

class Worksheet(models.Model):
    id = models.CharField(primary_key=True, max_length=100)
    lines = models.ManyToManyField(Line, blank=True, null=True)

After running syncdb (happens with sqlite3 and postgresql-8.3 btw), then the following is the output from manage.py shell:

In [1]: from x.y.models import *

In [2]: w = Worksheet(id='abc')

In [3]: w.save()

In [4]: w
Out[4]: <Worksheet: Worksheet object>

In [5]: w.delete()
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (20, 0))

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

/private/tmp/x/<ipython console> in <module>()

/Library/Python/2.5/site-packages/django/db/models/base.pyc in delete(self)
    567 
    568         # Actually delete the objects.
--> 569         delete_objects(seen_objs)
    570 
    571     delete.alters_data = True

/Library/Python/2.5/site-packages/django/db/models/query.pyc in delete_objects(seen_objs)
   1035             pk_list = [pk for pk,instance in items]
   1036             del_query = sql.DeleteQuery(cls, connection)
-> 1037             del_query.delete_batch_related(pk_list)
   1038 
   1039             update_query = sql.UpdateQuery(cls, connection)

/Library/Python/2.5/site-packages/django/db/models/sql/subqueries.pyc in delete_batch_related(self, pk_list)
     68                 where.add((Constraint(None, f.m2m_column_name(), f), 'in',
     69                         pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE]),
---> 70                         AND)
     71                 if w1:
     72                     where.add(w1, AND)

/Library/Python/2.5/site-packages/django/db/models/sql/where.pyc in add(self, data, connector)
     54         if hasattr(obj, "process"):
     55             try:
---> 56                 obj, params = obj.process(lookup_type, value)
     57             except (EmptyShortCircuit, EmptyResultSet):
     58                 # There are situations where we want to short-circuit any

/Library/Python/2.5/site-packages/django/db/models/sql/where.pyc in process(self, lookup_type, value)
    267         try:
    268             if self.field:
--> 269                 params = self.field.get_db_prep_lookup(lookup_type, value)
    270                 db_type = self.field.db_type()
    271             else:

/Library/Python/2.5/site-packages/django/db/models/fields/related.py in get_db_prep_lookup(self, lookup_type, value)
    160             return [pk_trace(value)]
    161         if lookup_type in ('range', 'in'):
--> 162             return [pk_trace(v) for v in value]
    163         elif lookup_type == 'isnull':
    164             return []

/Library/Python/2.5/site-packages/django/db/models/fields/related.py in pk_trace(value)
    137             if lookup_type in ('range', 'in'):
    138                 v = [v]
--> 139             v = field.get_db_prep_lookup(lookup_type, v)
    140             if isinstance(v, list):
    141                 v = v[0]

/Library/Python/2.5/site-packages/django/db/models/fields/__init__.pyc in get_db_prep_lookup(self, lookup_type, value)
    210             return [self.get_db_prep_value(value)]
    211         elif lookup_type in ('range', 'in'):
--> 212             return [self.get_db_prep_value(v) for v in value]
    213         elif lookup_type in ('contains', 'icontains'):
    214             return ["%%%s%%" % connection.ops.prep_for_like_query(value)]

/Library/Python/2.5/site-packages/django/db/models/fields/__init__.pyc in get_db_prep_value(self, value)
    359         if value is None:
    360             return None
--> 361         return int(value)
    362 
    363     def contribute_to_class(self, cls, name):

ValueError: invalid literal for int() with base 10: 'abc'

Attachments (0)

Change History (5)

comment:1 Changed 5 years ago by russellm

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

Ok... this one opens up a nest of vipers.

This is a regression caused by [10952], which closed #10785. That ticket in turn makes reference to #10243, and brokenness in get_db_prep_lookup(). Now that I'm digging deeper, I'm starting to see the magnitude of the problem that Malcolm was referring to.

For v1.1, we may need to simply revert [10952] and live with the edge case that won't work.

comment:2 Changed 5 years ago by russellm

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

(In [11007]) Fixed #11311 -- Reverted [10952], Refs #10785. Changeset [10952] caused problems with m2m relations between models that had non-integer primary keys. Thanks to Ronny for the report and test case.

comment:3 Changed 5 years ago by russellm

(In [11008]) [1.0.X] Fixed #11311 -- Reverted [10952], Refs #10785. Changeset [10952] caused problems with m2m relations between models that had non-integer primary keys. Thanks to Ronny for the report and test case.

comment:4 Changed 5 years ago by ronny

Thanks, Russell. It's working again now.

comment:5 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 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.