Opened 10 years ago

Closed 4 years ago

Last modified 4 years ago

#22089 closed Uncategorized (invalid)

Deleting object with custom primary_key causes the pk to be None

Reported by: anonymous Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Roman Levin Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am using PostgreSQL 9.3. I can reproduce this error every time. I have the following object model for my application:

class Location(models.Model):
    id = models.CharField(primary_key=True, max_length=20)
    ..  bunch of other non required fields ...

The location details view looks like this :

def location(request, location_id):
    # get the requested location
    location = Location.objects.get(pk=location_id)

    ... 

   elif request.POST.get('delete-button'):
        location.delete()
        messages.success(request, 'Location {0} was deleted.'.format(location_id))
        logger.info('User {0} has deleted location {1}.'.format(request.user, location_id))
        return redirect(reverse('locations_home'))

The redirect goes to a main locations page that runs a query set to get all locations

ls = Location.objects.all()

For each location a link is built using {% url 'locaion_page' location.id %}

The deleted object seems to be still pulled into the queryset as the delete operation didn't complete but the id field is now None or a blank character in the view. The application crashes with a NoReverse error for location_page with u

Not sure why this started happening, it wasn't ever a problem, but now deletes seem to take longer and the amount of locations hasn't increased. ~ 100.

When i did location.delete() in the shell and quickly ran location again, i noticed it was still there. I ran location.delete() again and I got an AssertionError which made sense.

I suspect this is a bug related to using a CharField primary key.

Change History (6)

comment:1 by anonymous, 10 years ago

I also found this error here, and they concluded that switching to the default id fixes it all:

https://github.com/django-mptt/django-mptt/issues/151

This is actually one of the things that was done by the previous programmer, he decided to make a CharField primary key. Bad idea .. but it identifies the location # uniquely using an integer. I will see if I can find a workaround. If having a CharField primary key causes the object to be partially deleted if a queryset to get all objects is ran right after then maybe CharFields should not be allowed to be pk.

I'm going to do more investigating in the meantime...

comment:2 by anonymous, 10 years ago

I didn't encounter the issue after adding blank=False to the model field & required no schema changes:

class Location(models.Model):

id = models.CharField(primary_key=True, max_length=20, blank=False)

in reply to:  2 comment:3 by Claude Paroz, 10 years ago

Resolution: needsinfo
Status: newclosed

Replying to anonymous:

I didn't encounter the issue after adding blank=False to the model field & required no schema changes:

That's weird, because blank=False is the default value anyway...

Could you maybe provide us a minimal project to be able to reproduce the issue?

comment:4 by Roman Levin, 4 years ago

Cc: Roman Levin added
Resolution: needsinfo
Status: closednew

This issue still seems to exist.

I created a small project to reproduce this issue here: https://github.com/romanlevin/django-custom-pk-bug

I ran into the issue because I used a custom primary key field in __str__ in an inlined model, so when one attempts to delete the inlined object, writing the parent model's change log fails, due to the deleted nested object's __str___ returning None.

comment:5 by Simon Charette, 4 years ago

Resolution: invalid
Status: newclosed

... writing the parent model's change log fails, due to the deleted nested object's str_ returning None.

That's the crux of the issue here and has little to do with Django models IMO; object.__str__ is simply not allowed to return None.

I'm afraid you'll have to return an empty string when the value is missing and that there's little to be fixed on Django's side. Model.delete has been setting .pk = None forever and changing it to accommodate a bogus __str__ implementation seems wrong.

FWIW this has little to do with the usage of custom primary key or model deletion. Given the following model

class Example(models.Model):
    def __str__(self):
        return self.pk

It would still crash on str(Example()) just like str(EmailAddress()) crash from your provided test project.

In summary Model.__str__ implementations should account for None attributes just like any object subclass has to.

Last edited 4 years ago by Simon Charette (previous) (diff)

in reply to:  5 comment:6 by Roman Levin, 4 years ago

I'm far from experienced with Django, so maybe I'm missing something here, but the issue isn't that a __str__ returns None; I just used that to illustrate that when being deleted, a (previously persisted) object's primary key gets set to None, which was what the original ticket is about and, as far as I can tell, either unintentional or undocumented.

I used this test case because that's how I ran into the issue today.

Replying to Simon Charette:

... writing the parent model's change log fails, due to the deleted nested object's str_ returning None.

That's the crux of the issue here and has little to do with Django models IMO; object.__str__ is simply not allowed to return None.

I'm afraid you'll have to return an empty string when the value is missing and that there's little to be fixed on Django's side. Model.delete has been setting .pk = None forever and changing it to accommodate a bogus __str__ implementation seems wrong.

FWIW this has little to do with the usage of custom primary key or model deletion. Given the following model

class Example(models.Model):
    def __str__(self):
        return self.pk

It would still crash on str(Example()) just like str(EmailAddress()) crash from your provided test project.

In summary Model.__str__ implementations should account for None attributes just like any object subclass has to.

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