Code

Opened 8 years ago

Closed 7 years ago

Last modified 6 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
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: UI/UX:

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

0_pk.patch (539 bytes) - added by SmileyChris 7 years ago.
0_pk.2.patch (1.6 KB) - added by SmileyChris 7 years ago.
with updated docs

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by fgutierrez

  • Component changed from Documentation to Database wrapper
  • Owner changed from jacob to adrian
  • 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.

comment:2 Changed 8 years ago 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

comment:3 Changed 8 years ago by russellm

  • Description modified (diff)
  • priority changed from high to normal
  • Severity changed from critical 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
  • Version set to 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 7 years ago by mir@…

  • Triage Stage changed from Unreviewed to Accepted

Looks like a bug to me.

comment:5 Changed 7 years ago by russellm

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

(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 7 years ago by russellm

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

comment:7 Changed 7 years ago by russellm

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 7 years ago by SmileyChris

Changed 7 years ago by SmileyChris

with updated docs

comment:8 Changed 7 years ago by SmileyChris

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 7 years ago by russellm

  • Needs tests set
  • Patch needs improvement set
  • Triage 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.

comment:10 Changed 7 years ago 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?

comment:11 Changed 7 years ago 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).

comment:12 Changed 7 years ago 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.

comment:13 Changed 7 years ago by mtredinnick

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

(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 6 years ago 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 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.