[Django] #36073: Custom field without get_db_prep_value() errors on updates if part of composite primary key

[Django]

#36073: Custom field without get_db_prep_value() errors on updates if part of
composite primary key
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
A model with a custom field only implementing `get_db_prep_save()` but not
`get_db_prep_value()` fails on `save()` when used in a composite primary
key, but only for updates. Inserts work fine.

This fell out of #36062 where the test model I adjusted happened to have a
[https://github.com/django/django/blob/a9c79b462923ce366101db1b5a3fff3d1dad870c/tests/serializers/models/base.py#L128-L149
custom field with these characteristics.]

I didn't get as far as verifying this to be relevant, but I noticed while
stepping through the code that `SQLInsertCompiler` has
[https://github.com/django/django/blob/a9c79b462923ce366101db1b5a3fff3d1dad870c/django/db/models/sql/compiler.py#L1814
logic to add `pk` to the list of fields], so I wonder if it's not being
prepared the same way in `SQLUpdateCompiler`?

{{{#!diff
diff --git a/tests/custom_pk/fields.py b/tests/custom_pk/fields.py
index 2d70c6b6dc..135b315ffe 100644
--- a/tests/custom_pk/fields.py
+++ b/tests/custom_pk/fields.py
@@ -59,6 +59,27 @@ class MyWrapperField(models.CharField):
return value


+class MyWrapperFieldWithoutGetDbPrepValue(models.CharField):
+ def to_python(self, value):
+ if not value:
+ return
+ if not isinstance(value, MyWrapper):
+ value = MyWrapper(value)
+ return value
+
+ def from_db_value(self, value, expression, connection):
+ if not value:
+ return
+ return MyWrapper(value)
+
+ def get_db_prep_save(self, value, connection):
+ if not value:
+ return
+ if isinstance(value, MyWrapper):
+ return str(value)
+ return value
+
+
class MyAutoField(models.BigAutoField):
def from_db_value(self, value, expression, connection):
if value is None:
diff --git a/tests/custom_pk/models.py b/tests/custom_pk/models.py
index 000ea7a29d..4ea5190d5f 100644
--- a/tests/custom_pk/models.py
+++ b/tests/custom_pk/models.py
@@ -7,7 +7,7 @@ this behavior by explicitly adding ``primary_key=True`` to
a field.

from django.db import models

-from .fields import MyAutoField, MyWrapperField
+from .fields import MyAutoField, MyWrapperField,
MyWrapperFieldWithoutGetDbPrepValue


class Employee(models.Model):
@@ -40,3 +40,9 @@ class Foo(models.Model):

class CustomAutoFieldModel(models.Model):
id = MyAutoField(primary_key=True)
+
+
+class CompositePKWithoutGetDbPrepValue(models.Model):
+ pk = models.CompositePrimaryKey("id", "tenant")
+ id = MyWrapperFieldWithoutGetDbPrepValue()
+ tenant = models.SmallIntegerField()
diff --git a/tests/custom_pk/tests.py b/tests/custom_pk/tests.py
index 5f865b680a..fa371942e6 100644
--- a/tests/custom_pk/tests.py
+++ b/tests/custom_pk/tests.py
@@ -2,7 +2,7 @@ from django.db import IntegrityError, transaction
from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature

from .fields import MyWrapper
-from .models import Bar, Business, CustomAutoFieldModel, Employee, Foo
+from .models import Bar, Business, CompositePKWithoutGetDbPrepValue,
CustomAutoFieldModel, Employee, Foo


class BasicCustomPKTests(TestCase):
@@ -164,6 +164,14 @@ class CustomPKTests(TestCase):
Business.objects.create(name="Bears")
Business.objects.create(pk="Tears")

+ def test_custom_composite_pk_save_force_update(self):
+ """A custom primary key field that implements get_db_prep_save()
+ rather than get_db_prep_value() still saves."""
+ obj = CompositePKWithoutGetDbPrepValue()
+ obj.id = MyWrapper("1")
+ obj.tenant = 1
+ obj.save() # passes with force_insert=True
+
def test_unicode_pk(self):
# Primary key may be Unicode string.
Business.objects.create(name="jaźń")
}}}
----
{{{
ERROR: test_custom_composite_pk_save_force_update
(custom_pk.tests.CustomPKTests.test_custom_composite_pk_save_force_update)
A custom primary key field that implements get_db_prep_save()
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/.../django/django/db/backends/utils.py", line 105, in
_execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../django/django/db/backends/sqlite3/base.py", line 360,
in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.ProgrammingError: Error binding parameter 3: type 'MyWrapper' is
not supported

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/Users/.../django/tests/custom_pk/tests.py", line 173, in
test_custom_composite_pk_save_force_update
bar.save() # passes with force_insert=True
^^^^^^^^^^
...

File "/Users/.../django/django/db/backends/sqlite3/base.py", line 360,
in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.ProgrammingError: Error binding parameter 3: type
'MyWrapper' is not supported

----------------------------------------------------------------------
Ran 16 tests in 0.011s

FAILED (errors=1, skipped=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36073>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36073: Custom field without get_db_prep_value() errors on updates if part of
composite primary key
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Csirmaz Bendegúz (added)
* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36073#comment:1>

[Django]

#36073: Custom field without get_db_prep_value() errors on updates if part of
composite primary key
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Csirmaz Bendegúz):

I don't think this is related to composite primary keys, it errors if it's
a regular primary key as well
--
Ticket URL: <https://code.djangoproject.com/ticket/36073#comment:2>

[Django]

#36073: Custom field without get_db_prep_value() errors on updates if part of
composite primary key
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Release blocker | Resolution: invalid

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* resolution: => invalid
* status: new => closed

Comment:

Yes, it's the same for regular primary keys which makes me think whether
this is not valid
It's possible that the test code from serializers that inspired this is
not fully defined.
Jacob if you can add more information we can look at this again

(for reference, same thing for regular primary keys)

{{{#!diff
diff --git a/tests/custom_pk/fields.py b/tests/custom_pk/fields.py

index 2d70c6b6dc..3908b426c9 100644
--- a/tests/custom_pk/fields.py
+++ b/tests/custom_pk/fields.py
@@ -20,7 +20,7 @@ class MyWrapper:
return self.value == other


-class MyWrapperField(models.CharField):
+class MyWrapperFieldWithoutGetDbPrepValue(models.CharField):
def __init__(self, *args, **kwargs):
kwargs["max_length"] = 10
super().__init__(*args, **kwargs)
@@ -51,6 +51,7 @@ class MyWrapperField(models.CharField):
return str(value)
return value

+class MyWrapperField(models.CharField):
def get_db_prep_value(self, value, connection, prepared=False):
if not value:
return
diff --git a/tests/custom_pk/models.py b/tests/custom_pk/models.py
index 000ea7a29d..75b1031e10 100644

--- a/tests/custom_pk/models.py
+++ b/tests/custom_pk/models.py
@@ -7,7 +7,7 @@ this behavior by explicitly adding ``primary_key=True`` to
a field.

from django.db import models

-from .fields import MyAutoField, MyWrapperField
+from .fields import MyAutoField, MyWrapperField,
MyWrapperFieldWithoutGetDbPrepValue


class Employee(models.Model):

@@ -40,3 +40,7 @@ class Foo(models.Model):

class CustomAutoFieldModel(models.Model):
id = MyAutoField(primary_key=True)
+
+

+class PKWithoutGetDbPrepValue(models.Model):
+ id = MyWrapperFieldWithoutGetDbPrepValue(primary_key=True)
diff --git a/tests/custom_pk/tests.py b/tests/custom_pk/tests.py
index 5f865b680a..3bfd32c29c 100644

--- a/tests/custom_pk/tests.py
+++ b/tests/custom_pk/tests.py
@@ -2,7 +2,7 @@ from django.db import IntegrityError, transaction
from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature

from .fields import MyWrapper
-from .models import Bar, Business, CustomAutoFieldModel, Employee, Foo

+from .models import Bar, Business, CustomAutoFieldModel, Employee, Foo,
PKWithoutGetDbPrepValue


class BasicCustomPKTests(TestCase):
@@ -164,6 +164,11 @@ class CustomPKTests(TestCase):

Business.objects.create(name="Bears")
Business.objects.create(pk="Tears")

+ def test_custom_composite_pk_save_force_update(self):

+ obj = PKWithoutGetDbPrepValue()

+ obj.id = MyWrapper("1")

+ obj.save() # passes with force_insert=True
+
def test_unicode_pk(self):
# Primary key may be Unicode string.
Business.objects.create(name="jaźń")
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/36073#comment:3>