Can't use value of 0 for primary key

[fgutierrez AT aureal.com.pe]

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.

[fgutierrez]

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.

[Russell Keith-Magee]

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.

[mir@…]

Looks like a bug to me.

[Russell Keith-Magee]

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

[Russell Keith-Magee]

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

[Russell Keith-Magee]

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.

[Chris Beaven]

with updated docs

[Russell Keith-Magee]

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.

[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?

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

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

[Malcolm Tredinnick]

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

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