Opened 18 years ago

Closed 17 years ago

Last modified 16 years ago

#2160 closed defect (fixed)

Can't use value of 0 for primary key

Reported by: fgutierrez AT aureal.com.pe Owned by: Adrian Holovaty
Component: Database layer (models, ORM) Version: dev
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 17 years ago.
0_pk.2.patch (1.6 KB ) - added by Chris Beaven 17 years ago.
with updated docs

Download all attachments as: .zip

Change History (16)

comment:1 by fgutierrez, 18 years ago

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 by anonymous, 18 years ago

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 by Russell Keith-Magee, 17 years ago

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 by mir@…, 17 years ago

Triage Stage: UnreviewedAccepted

Looks like a bug to me.

comment:5 by Russell Keith-Magee, 17 years ago

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 by Russell Keith-Magee, 17 years ago

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

comment:7 by Russell Keith-Magee, 17 years ago

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.

by Chris Beaven, 17 years ago

Attachment: 0_pk.patch added

by Chris Beaven, 17 years ago

Attachment: 0_pk.2.patch added

with updated docs

comment:8 by Chris Beaven, 17 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:9 by Russell Keith-Magee, 17 years ago

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 by Chris Beaven, 17 years ago

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 by Russell Keith-Magee, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Gary Wilson, 16 years ago

(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