Django

Code

Ticket #2160 (closed: fixed)

Opened 2 years ago

Last modified 3 months ago

Can't use value of 0 for primary key

Reported by: fgutierrez AT aureal.com.pe Assigned to: adrian
Milestone: Component: Database layer (models, ORM)
Version: SVN Keywords:
Cc: Triage Stage: Design decision needed
Has patch: 1 Needs documentation: 0
Needs tests: 1 Patch needs improvement: 1

Description (Last modified by russellm)

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

0_pk.patch (0.5 kB) - added by SmileyChris on 02/25/07 19:55:26.
0_pk.2.patch (1.6 kB) - added by SmileyChris on 02/25/07 20:05:12.
with updated docs

Change History

06/14/06 18:17:11 changed by fgutierrez

  • owner changed from jacob to adrian.
  • component changed from Documentation to Database wrapper.
  • severity changed from major to critical.

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.

06/14/06 18:22:14 changed by anonymous

  • summary changed from Documentation How Django knows to Update vs Insert not correct to How Django knows to Update vs Insert not correct for PK values that evaluates to False.

12/17/06 08:33:11 changed by russellm

  • priority changed from high to normal.
  • summary changed from How Django knows to Update vs Insert not correct for PK values that evaluates to False to Can't use value of 0 for primary key.
  • description changed.
  • version set to SVN.
  • severity changed from critical to normal.

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.

01/30/07 16:06:35 changed by mir@noris.de

  • stage changed from Unreviewed to Accepted.

Looks like a bug to me.

02/03/07 18:57:18 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

(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@aureal.com.pe.

02/07/07 16:56:53 changed by russellm

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

02/07/07 16:58:48 changed by russellm

  • status changed from closed to reopened.
  • resolution deleted.

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.

02/25/07 19:55:26 changed by SmileyChris

  • attachment 0_pk.patch added.

02/25/07 20:05:12 changed by SmileyChris

  • attachment 0_pk.2.patch added.

with updated docs

02/25/07 20:05:53 changed by SmileyChris

  • has_patch set to 1.
  • stage changed from Accepted to Ready for checkin.

02/25/07 20:21:30 changed by russellm

  • needs_better_patch set to 1.
  • needs_tests set to 1.
  • stage changed from Ready for checkin to Design 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.

02/25/07 21:01:56 changed by SmileyChris

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?

02/25/07 21:27:41 changed by russellm

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).

08/18/07 22:16:04 changed by mtredinnick

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.

08/18/07 22:23:32 changed by mtredinnick

  • status changed from reopened to closed.
  • resolution set to fixed.

(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.

08/27/08 02:19:45 changed by gwilson

(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).

Add/Change #2160 (Can't use value of 0 for primary key)




Change Properties
Action