Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#34792 closed Bug (duplicate)

Creating and saving a model using a custom primary key field can yield a bad "id" value on the instance

Reported by: mike w Owned by: nobody
Component: Database layer (models, ORM) Version: 4.2
Severity: Normal Keywords: field, custom fields
Cc: Chris M Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by mike w)

Summary: In some situations, creating and saving a model which uses a custom primary key can cause a wrong ".id" value to be set on the instance in memory.

Detail: I am using a custom primary key Field type which inherits from AutoField, django-spicy-id. Essentially what this Field does is translate the underlying integer data in the database, to and from a string (with a static prefix) at the Django level.

It does this by implementing from_db_value() and friends in the Field implementation.

This might not be to everyone's taste, but so far I think it is completely within the acceptable functionality & contract of custom Fields.

I noticed that in some environments, particularly in Django pre-4.0, save() on a new instance would cause the instance's .id field to be the underlying number - and not the expected string value, which the custom Field implements. Example:

class MyModel(models.Model):
  id = SpicyBigAutoField("ex", primary_key=True, randomize=True)
>>> o = MyModel()
>>> o.save()
>>> o.id
3735928559  # yikes! we should never get a number here.
>>> o.refresh_from_db()
>>> o.id
'ex_deadbeef`

I believe the culprit is this block in django.db.models.sql.compiler, specifically the last condition where self.connection.ops.last_insert_id is called and returned without modification.

In turn, the parent caller django.db.models.base.Model::_save_table() will blindly setattr this value on the instance.

It seems like a possible fix could be to flow the "last_insert_id" case through the Field's from_db_value(); in most cases this is surely a no-op, but.. not in this case.

(I'd happy to prepare a patch and/or test if offered a little guidance.) Thanks!

Change History (4)

comment:1 by mike w, 15 months ago

Description: modified (diff)
Type: UncategorizedBug

comment:2 by Chris M, 15 months ago

Cc: Chris M added

I can reproduce this in version 3.2, but not in 4.0+.

It looks like this is a duplicate of #32442 and was resolved there. I'm just starting out triaging tickets, but I believe this should be resolved as a duplicate. I'm not sure that it should be kept open to patch 3.2 as LTS since it seems like a backwards incompatible change. Can someone with more experience please confirm?

comment:3 by Mariusz Felisiak, 15 months ago

Resolution: duplicate
Status: newclosed

Chris, thanks! Yes it's a duplicate of #32442.

I'm not sure that it should be kept open to patch 3.2 as LTS since it seems like a backwards incompatible change.

No, Django 3.2 doesn't receive bugfixed anymore (except security patches).

comment:4 by mike w, 15 months ago

I'm super impressed with your triage, and the fact that this issue has already been expertly dissected and fixed elsewhere. All makes sense. Thanks Chris and Mariusz!

Note: See TracTickets for help on using tickets.
Back to Top