Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#2160 closed defect (fixed)

Can't use value of 0 for primary key

Reported by: fgutierrez AT Owned by: Adrian Holovaty
Component: Database layer (models, ORM) Version: master
Severity: normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Russell Keith-Magee)

The DB API uses a 'bool(pk_val) == false' check to determine if a model instance is a new item needs to be INSERTed, or an existing entry that needs to be UPDATEd.

Most databases use PK's starting at 1, but if you try to manually use a pk value of 0, the object cannot be saved - it can only be recreated.

Attachments (2)

0_pk.patch (539 bytes) - added by Chris Beaven 12 years ago.
0_pk.2.patch (1.6 KB) - added by Chris Beaven 12 years ago.
with updated docs

Download all attachments as: .zip

Change History (16)

comment:1 Changed 12 years ago by fgutierrez

Component: DocumentationDatabase wrapper
Owner: changed from Jacob to Adrian Holovaty
Severity: majorcritical

I have realized also that if i have that kind of PK values i could never update the object, i can get the object, change its values but once I use save then the exact same problem raises and Django tries to do an Insert, so raising a Duplicate Key exception.

comment:2 Changed 12 years ago by anonymous

Summary: Documentation How Django knows to Update vs Insert not correctHow Django knows to Update vs Insert not correct for PK values that evaluates to False

comment:3 Changed 12 years ago by Russell Keith-Magee

Description: modified (diff)
priority: highnormal
Severity: criticalnormal
Summary: How Django knows to Update vs Insert not correct for PK values that evaluates to FalseCan't use value of 0 for primary key
Version: SVN

Original ticket description covered two problems. First problem was resubmitted as #3118, and has since been resolved. Ticket description has been revised to reflect the remaining problem.

comment:4 Changed 12 years ago by mir@…

Triage Stage: UnreviewedAccepted

Looks like a bug to me.

comment:5 Changed 12 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

(In [4459]) Fixed #2160 -- Modified object save logic to check for pk is None, rather than bool(pk) == False. This allows primary keys to take a value of 0. Thanks for the report, fgutierrez@….

comment:6 Changed 12 years ago by Russell Keith-Magee

(In [4463]) Fixes #3447, Refs #2160 -- Reverting change [4459] because it breaks admin. Apologies for the inconvenience, guys.

comment:7 Changed 12 years ago by Russell Keith-Magee

Resolution: fixed
Status: closedreopened

Needs a little bit more than just checking if the primary key is "None" - admin uses empty string as well as None to establish new objects.

Changed 12 years ago by Chris Beaven

Attachment: 0_pk.patch added

Changed 12 years ago by Chris Beaven

Attachment: 0_pk.2.patch added

with updated docs

comment:8 Changed 12 years ago by Chris Beaven

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:9 Changed 12 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinDesign decision needed

The approach of the 0_pk.2 patch isn't my ideal solution. The fact that the internal logic reads as 'bool(arg) or arg == 0' should be a warning flag; this doesn't make much sense as a logical construct, and doesn't scan well with standard python usage of truth values.

IMHO, the better fix for this problem would be to use the 'pk is None' logic from [4459], but modify the edit-inline code that breaks as a result of this approach. Keep in mind that edit-inlines will be cleaned up as a part of the newforms admin rewrite; if we can kill 2 birds with one stone, all the better.

comment:10 Changed 12 years ago by Chris Beaven

Fair enough, but there's probably cases (either in the wild or elsewhere in the framework) where the pk could resolve to an empty string. Perhaps pk is None or pk == '' would be enough?

comment:11 Changed 12 years ago by Russell Keith-Magee

pk is None or pk == is the right solution in the short term, but in the slightly-longer-than-short term (i.e., when newforms is rolled out as the default), I'm not so sure.

The larger design issue is 'should models autoconvert string arguments into value arguments'. The current behaviour uses to_python method on model fields to convert strings into field values, with the side effect being that in-code instance definitions can also use strings. This conversion is migrating into the newforms fields. To me, this means that accepting strings as model values (which the model then converts) is behaviour that shouldn't be encouraged, which is what the pk== case is protecting (i.e., empty string as form-compliant null).

comment:12 Changed 11 years ago by Malcolm Tredinnick

Whilst I completely agree with Russell's design philosophy here, reality gets in the way in practice. Oldforms exists and there are apps in the wild that create models using those idioms. So having pk_val being the empty string is going to be with us for a while, even beyond 1.0. I think we have to live with the slight ugliness. Ruling out certain valid primary key values to save a less-than-perfect test in the code feels like we are handicapping the wrong people.

comment:13 Changed 11 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

(In [5934]) Tweaked the unset primary key check from [5933] to accommodate models created in the oldforms style. It's a backwards-compatibility hack that we can remove when oldforms go away. See #2160 for discussion. Fixed #5204, #2160. Refs #5102.

comment:14 Changed 10 years ago by Gary Wilson

(In [8616]) Removed oldforms, validators, and related code:

  • Removed Manipulator, AutomaticManipulator, and related classes.
  • Removed oldforms specific bits from model fields:
    • Removed validator_list and core arguments from constructors.
    • Removed the methods:
      • get_manipulator_field_names
      • get_manipulator_field_objs
      • get_manipulator_fields
      • get_manipulator_new_data
      • prepare_field_objs_and_params
      • get_follow
    • Renamed flatten_data method to value_to_string for better alignment with its use by the serialization framework, which was the only remaining code using flatten_data.
  • Removed oldforms methods from django.db.models.Options class: get_followed_related_objects, get_data_holders, get_follow, and has_field_type.
  • Removed oldforms-admin specific options from django.db.models.fields.related classes: num_in_admin, min_num_in_admin, max_num_in_admin, num_extra_on_change, and edit_inline.
  • Serialization framework
    • Serializer.get_string_value now calls the model fields' renamed value_to_string methods.
    • Removed a special-casing of models.DateTimeField in core.serializers.base.Serializer.get_string_value that's handled by django.db.models.fields.DateTimeField.value_to_string.
  • Removed django.core.validators:
    • Moved ValidationError exception to django.core.exceptions.
    • For the couple places that were using validators, brought over the necessary code to maintain the same functionality.
  • Introduced a SlugField form field for validation and to compliment the SlugField model field (refs #8040).
  • Removed an oldforms-style model creation hack (refs #2160).
Note: See TracTickets for help on using tickets.
Back to Top