[Django] #34539: `get_prep_value` no longer called for JSONField
Django
-------------------------------------+-------------------------------------
Reporter: Julie | Owner: nobody
Rymer |
Type: Bug | Status: new
Component: Database | Version: 4.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hello, I just upgraded from django 4.1 to 4.2 and I have a custom
`JSONField` with a `get_prep_value()` override that stopped working.
After searching a bit, I saw that was because `JSONField.get_prep_value()`
is no longer called in 4.2.
I think this issue need a resolution either:
- `JSONField` should call `get_prep_value()` like all other fields type,
because this is the method that the
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field
documentation] tell us to override in custom fields.
Otherwise we need to override `get_db_prep_value()` which is heavier and
does not have the same purpose. I think simply replacing
`connection.ops.adapt_json_value(value, self.encoder)` with
`connection.ops.adapt_json_value(self.get_prep_value(value),
self.encoder)` in `JSONField.get_db_prep_value()` would fix this
- If there is a good reason to no longer call `get_prep_value()`, this
exception for `JSONField` should be clearly documented in
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field.get_prep_value
custom get_prep_value() doc]. It should also be added to
[https://docs.djangoproject.com/en/4.2/releases/4.2/#backwards-
incompatible-changes-in-4-2 Backwards incompatible changes in 4.2 release
note] because I got stuck with this issue with no warning when migrating.
PS: #34397 seems to be related but in fact is about Django 3.2 so it is
not the current issue
--
Ticket URL: <https://code.djangoproject.com/ticket/34539>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Hello, I just upgraded from django 4.1 to 4.2 and I have a custom
> `JSONField` with a `get_prep_value()` override that stopped working.
> After searching a bit, I saw that was because
> `JSONField.get_prep_value()` is no longer called in 4.2.
>
> I think this issue need a resolution either:
>
> - `JSONField` should call `get_prep_value()` like all other fields type,
> because this is the method that the
> [https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field
> documentation] tell us to override in custom fields.
> Otherwise we need to override `get_db_prep_value()` which is heavier
> and does not have the same purpose. I think simply replacing
> `connection.ops.adapt_json_value(value, self.encoder)` with
> `connection.ops.adapt_json_value(self.get_prep_value(value),
> self.encoder)` in `JSONField.get_db_prep_value()` would fix this
>
> - If there is a good reason to no longer call `get_prep_value()`, this
> exception for `JSONField` should be clearly documented in
> [https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field.get_prep_value
> custom get_prep_value() doc]. It should also be added to
> [https://docs.djangoproject.com/en/4.2/releases/4.2/#backwards-
> incompatible-changes-in-4-2 Backwards incompatible changes in 4.2 release
> note] because I got stuck with this issue with no warning when migrating.
>
> PS: #34397 seems to be related but in fact is about Django 3.2 so it is
> not the current issue
New description:
Hello, I just upgraded from django 4.1 to 4.2 and I have a custom
`JSONField` with a `get_prep_value()` override that stopped working.
After searching a bit, I saw that was because `JSONField.get_prep_value()`
is no longer called in 4.2
([https://github.com/django/django/commit/5c23d9f0c32f166c81ecb6f3f01d5077a6084318
5c23d9f0c32f166c81ecb6f3f01d5077a6084318]).
I think this issue need a resolution either:
- `JSONField` should call `get_prep_value()` like all other fields type,
because this is the method that the
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field
documentation] tell us to override in custom fields.
Otherwise we need to override `get_db_prep_value()` which is heavier and
does not have the same purpose. I think simply replacing
`connection.ops.adapt_json_value(value, self.encoder)` with
`connection.ops.adapt_json_value(self.get_prep_value(value),
self.encoder)` in `JSONField.get_db_prep_value()` would fix this
- If there is a good reason to no longer call `get_prep_value()`, this
exception for `JSONField` should be clearly documented in
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field.get_prep_value
custom get_prep_value() doc]. It should also be added to
[https://docs.djangoproject.com/en/4.2/releases/4.2/#backwards-
incompatible-changes-in-4-2 Backwards incompatible changes in 4.2 release
note] because I got stuck with this issue with no warning when migrating.
PS: #34397 seems to be related but in fact is about Django 3.2 so it is
not the current issue
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Accepting this since, as the reporter says, the docs suggest this method
will be called for most fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
Thanks for report. The way how built-in fields are implemented is an
implementation detail and can change without the deprecation path.
Replying to [ticket:34539 Julie Rymer]:
> Hello, I just upgraded from django 4.1 to 4.2 and I have a custom
`JSONField` with a `get_prep_value()` override that stopped working.
After searching a bit, I saw that was because `JSONField.get_prep_value()`
is no longer called in 4.2
([https://github.com/django/django/commit/5c23d9f0c32f166c81ecb6f3f01d5077a6084318
5c23d9f0c32f166c81ecb6f3f01d5077a6084318]).
>
> I think this issue need a resolution either:
>
> - `JSONField` should call `get_prep_value()` like all other fields type,
because this is the method that the
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field
documentation] tell us to override in custom fields.
Both methods `get_prep_value()` and `get_db_prep_value()` are documented
there. I don't see any preferences for `get_prep_value()` 🤔.
> Otherwise we need to override `get_db_prep_value()` which is heavier and
does not have the same purpose. I think simply replacing
`connection.ops.adapt_json_value(value, self.encoder)` with
`connection.ops.adapt_json_value(self.get_prep_value(value),
self.encoder)` in `JSONField.get_db_prep_value()` would fix this
>
> - If there is a good reason to no longer call `get_prep_value()`, this
exception for `JSONField` should be clearly documented in
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field.get_prep_value
custom get_prep_value() doc]. It should also be added to
[https://docs.djangoproject.com/en/4.2/releases/4.2/#backwards-
incompatible-changes-in-4-2 Backwards incompatible changes in 4.2 release
note] because I got stuck with this issue with no warning when migrating.
This is not really an exception, there are many built-in fields that don't
call `get_prep_value()` in `get_db_prep_value()`, e.g. `ArrayField` or
`DurationField`.
You can check how, for example, Wagtail solved
[https://github.com/wagtail/wagtail/issues/9692 the same issue] on their
side.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------
* cc: Simon Charette, Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
Also, the docs that you mentioned is `Field API reference` not `How to
create a custom field` or `How to override build-in fields`, so it only
describes methods available in the `Field` API.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Julie Rymer):
Replying to [comment:3 Mariusz Felisiak]:
> Thanks for report. The way how built-in fields are implemented is an
implementation detail and can change without the deprecation path.
>
> ...
>
> Both methods `get_prep_value()` and `get_db_prep_value()` are documented
there. I don't see any preferences for `get_prep_value()` 🤔.
It doesn't seems like an implementation detail when those function are
clearly documented for customisation.
As for the preference, the first paragraph of the
[https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field
Field API doc] is
> Field is an abstract class that represents a database table column.
Django uses fields to create the database table (db_type()), to map Python
types to database (get_prep_value()) and vice-versa (from_db_value()).
`get_prep_value` is the documented method for mapping Python types to
database, which is what I was trying to do, `get_db_prep_value` is only
for backend specific need so not the most common case (hence why only
`get_prep_value` is put foremost in the intro I suppose).
Replying to [comment:4 Mariusz Felisiak]:
> Also, the docs that you mentioned is Field API reference not How to
create a custom field or How to override build-in fields, so it only
describes methods available in the Field API.
You are right, however the [https://docs.djangoproject.com/en/4.2/howto
/custom-model-fields/#converting-python-objects-to-query-values
customisation doc] still does not mention that `get_prep_value()` is not
always available for override.
Replying to [comment:3 Mariusz Felisiak]:
> This is not really an exception, there are many built-in fields that
don't call get_prep_value() in get_db_prep_value(), e.g. ArrayField or
DurationField.
Then it should be changed, or documented, for those fields also :)
Like wagtail, I also ended up overriding `get_db_prep_value()` instead of
`get_prep_value()`, but this seems like too much work when I don't care
about the backend and simply want to serialise my custom object.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Natalia Bidart):
Julie, could you tell us a little bit more about how you are using
`get_prep_value`? That could help us understand a bit better the use case
to make further decisions. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
> It doesn't seems like an implementation detail when those function are
clearly documented for customisation.
`Field.get_prep_value()` is documented for the customization, not
`JSONField.get_prep_value()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Julie Rymer):
My usecase is the following:
I have a db json field that I use to keep data in the following format
`[{id: str, label: str, length: Decimal, height: Decimal, width: Decimal,
weight: Decimal}]`
For easier manipulation and validation, I have a dataclass containing
those properties. I created a custom JSONField to convert to/from a list
of that dataclass to a simple python dict to be inserted in db.
From the [https://docs.djangoproject.com/en/4.2/howto/custom-model-fields
/#converting-values-to-python-objects doc]
> If your custom Field class deals with data structures that are more
complex than strings, dates, integers, or floats, then you may need to
override from_db_value() and to_python().
And so I overrode `from_db_value()` and `to_python()`
Further along the [https://docs.djangoproject.com/en/4.2/howto/custom-
model-fields/#converting-python-objects-to-query-values doc]
> Since using a database requires conversion in both ways, if you override
from_db_value() you also have to override get_prep_value() to convert
Python objects back to query values.
And so I overrode `get_prep_value()`. But `get_prep_value()` is never
called, alas. This behaviour is not consistent from what is expected.
I know that this doc is about `Field.get_prep_value()` and not
`JSONField.get_prep_value()`, but unless I missed something, there is no
specific documentation for `JSONField.get_prep_value()`, nor forewarning
in `Field.get_prep_value()`, so the obvious guess is that this doc apply
to `JSONField.get_prep_value()` as well as any other Field class.
I think the question to solve is
- do we want to add a warning that the documented rules for customisation
does not apply to some Field classes and to document the missing rules, or
- should we harmonize instead all Field classes that don't follow the
rules yet and make their behaviour consistent with current documentation.
I am more in favour of the second option since it does seems to be less of
a change effort and more practical for the end-user.
You can find my custom Field implementation here:
{{{
from __future__ import annotations
import decimal
import json
from dataclasses import dataclass, fields
from decimal import Decimal
from django.core import exceptions
from django.db import models
from django.utils.translation import gettext_lazy as _
@dataclass(repr=True, eq=True, unsafe_hash=True)
class ProductPackage:
id: str
length: Decimal # in cm
height: Decimal # in cm
width: Decimal # in cm
weight: Decimal # in kg
label: str = ""
@staticmethod
def from_dict(package_json: dict) -> ProductPackage:
return ProductPackage(
id=package_json.get("id"),
length=Decimal(package_json["length"]),
height=Decimal(package_json["height"]),
width=Decimal(package_json["width"]),
weight=Decimal(package_json["weight"]),
label=package_json.get("label", ""),
)
def to_dict(self) -> dict:
return dict((field.name, getattr(self, field.name)) for field in
fields(self))
@classmethod
def from_list(cls, packages_json: list[dict]) -> list:
return list(
cls.from_dict(package_json) for package_json in (packages_json
or [])
)
def __copy__(self):
return self.copy()
def copy(self):
return self.__class__(**self.to_dict())
class PackageJSONField(models.JSONField):
"""
ProductPackage class will make package dimension more practical to use
and this class also handle Decimal so they can be
serialized/deserialized from JSON
"""
description = "A JSON field to store a list of ProductPackage"
default_error_messages = {
**models.JSONField.default_error_messages,
"invalid_packages": _("Value must be valid list of packages."),
"invalid_decimal": _("Package dimensions must be valid decimal
strings."),
}
def from_db_value(self, value, expression, connection) ->
list[ProductPackage]:
packages_json = super().from_db_value(value, expression,
connection)
return ProductPackage.from_list(packages_json)
def to_python(self, value):
if value is None:
return value
if isinstance(value, ProductPackage):
return value
try:
if isinstance(value, list):
return ProductPackage.from_list(value)
return json.loads(value, cls=self.decoder)
except json.JSONDecodeError:
raise exceptions.ValidationError(
self.error_messages["invalid"],
code="invalid",
params={"value": value},
)
except TypeError:
raise exceptions.ValidationError(
self.error_messages["invalid_packages"],
code="invalid_packages",
params={"value": value},
)
except decimal.InvalidOperation:
raise exceptions.ValidationError(
self.error_messages["invalid_decimal"],
code="invalid_decimal",
params={"value": value},
)
def get_prep_value(self, value: list[ProductPackage | dict]):
if value is None:
return value
if any(isinstance(package, dict) for package in value):
value = ProductPackage.from_list(value)
value = list(
{
"id": package.id,
"label": package.label,
"length": str(package.length),
"height": str(package.height),
"width": str(package.width),
"weight": str(package.weight),
}
for package in value
)
return super().get_prep_value(value)
def get_db_prep_value(self, value, connection, prepared=False):
# the override I ended up doing for the current issue
if isinstance(value, models.expressions.Value) and isinstance(
value.output_field, models.JSONField
):
value = value.value
elif hasattr(value, "as_sql"):
return value
return connection.ops.adapt_json_value(self.get_prep_value(value),
self.encoder)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
Replying to [comment:9 Julie Rymer]:
> I think the question to solve is
> - do we want to add a warning that the documented rules for
customisation does not apply to some Field classes and to document the
missing rules, or
> - should we harmonize instead all Field classes that don't follow the
rules yet and make their behaviour consistent with current documentation.
> I am more in favour of the second option since it does seems to be less
of a change effort and more practical for the end-user.
This will required adding LOC (probably many) that are not used by Django
itself and aren't tested (unless we will add many tests with custom
subclasses of built-in fields). This can also cause a lot of backward
incompatible changes in the behavior. I'm strongly against it as it'll set
a dangerous precedent and increase the maintenance burden.
I'll wait for other opinions.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Julie Rymer):
Replying to [comment:10 Mariusz Felisiak]:
> This will required adding LOC (probably many) that are not used by
Django itself and aren't tested (unless we will add many tests with custom
subclasses of built-in fields). This can also cause a lot of backward
incompatible changes in the behavior. I'm strongly against it as it'll set
a dangerous precedent and increase the maintenance burden.
I'll try to address Mariusz' concerns.
From what I understand, the change would only be a matter of adding
`value=self.get_prep_value(value)` is the concerned Field classes'
`get_db_prep_value` (`JSONField`, `ArrayField`, `DurationField`, other
?). The default `Field.get_prep_value` definition is the following:
{{{
def get_prep_value(self, value):
"""Perform preliminary non-db specific value checks and
conversions."""
if isinstance(value, Promise):
value = value._proxy____cast()
return value
}}}
So it doesn't seems to do much so I don't see what backward incompatible
change that would cause.
But if there is concern about it, `get_prep_value` could be overridden in
the concerned Field classes to directly return the value instead of doing
any operation, hence guaranteed no behaviour change.
This addition to the code **will be tested**, because `get_db_prep_value`
is already tested (right?) and we are not adding any behaviour that should
involve any explicit test. We are just adding an easier entrypoint for
customisation, the customisation itself is not for django to test so I
don't see what additional maintenance burden it would add.
The only potential burden I see is a new rule for future changes done on
`get_db_prep_value` of any standard field class to take care to call
`get_prep_value` even if it does nothing.
Perhaps I missed something, do tell me if that is the case.
On the contrary, updating the documentation to keep track of any Field
class not following the rule and explaining how to do customisation for
each of them seems like a lot more involved effort.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
Replying to [comment:11 Julie Rymer]:
> {{{
> def get_prep_value(self, value):
> """Perform preliminary non-db specific value checks and
conversions."""
> if isinstance(value, Promise):
> value = value._proxy____cast()
> return value
> }}}
>
> So it doesn't seems to do much so I don't see what backward incompatible
change that would cause.
The number of LOC is not important. We are talking not only about
`JSONField`. If we will agree that all built-in fields must implement the
entire `Field` API and fulfill everything that is in the `Field` API
reference, we will be force to make many other changes.
> This addition to the code **will be tested**, because
`get_db_prep_value` is already tested (right?)
This is not true, removing `get_prep_value()` will not cause any tests to
fail, so it won't be tested.
> On the contrary, updating the documentation to keep track of any Field
class not following the rule and explaining how to do customisation for
each of them seems like a lot more involved effort.
Personally, I'd close this ticket as invalid. As far as I'm aware, neither
docs nor code changes are required.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Julie Rymer):
Replying to [comment:12 Mariusz Felisiak]:
> This is not true, removing `get_prep_value()` will not cause any tests
to fail, so it won't be tested.
You're right, but nothing a spy+`assert_called_once` can't solve :p
Also, I though I'd mention that I'd be willing to make the necessary
changes in case this scenario is selected (I'd not have that self-
confidence for a doc update however ^^)
Finally as a last input, I'll mention that the
[https://docs.djangoproject.com/en/4.2/howto/custom-model-fields/ custom
field doc] actually explicitly elaborate on some standard Field type
gotcha when they deviate from the Field API contract: `AutoField` and
`FileField`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Mariusz Felisiak):
> You're right, but nothing a spy+`assert_called_once` can't solve :p
This is something we use as a last resort, not something that is a good
API testing.
It's pretty obvious that we're not going to reach a consensus, so let's
wait for other opinions.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------
* cc: Tim Graham (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Natalia Bidart):
As a Django user, I agree that the current documentation does imply that
`get_prep_value()` would be called for any `Field` instance. I always
assumed that everything documented for `Field` as a toplevel class applies
to all children (except explicitly stated otherwise), so I can personally
relate to the ticket description.
Having said the above, if modifying the Django code could generate extra
maintenance burden and/or introduce patterns that are not desired (such as
including code that is not used by Django itself), I would at least
suggest that the documentation is extended to explicitly say that
`get_prep_value()` may not be called in every Django-provided field.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:16>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------
* owner: nobody => Jatin-tec
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:17>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Jatin-tec => (none)
* status: assigned => new
* stage: Accepted => Unreviewed
Comment:
Hi Jatin-tec, sorry for the confusion but this ticket is not yet ready to
be worked on, we still haven't decided on the best course of action. It
could be that we change code, or docs, or nothing.
Thanks for understanding! I'll be removing you from the assignee field for
now.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I don't hold a strong opinion against restoring the call to
`get_prep_value` given how common using `JSONField` to persist Python
datastructure is.
I never assumed that we guaranteed that `get_prep_lookup` would be called
by `get_db_prep_lookup` but I can see how it can be interpreted this way
from the docs.
Given this was an unintended side-effect of
5c23d9f0c32f166c81ecb6f3f01d5077a6084318 I'd be +1 to having
`get_db_prep_value` start by calling `get_prep_value` when
`prepared=False`.
As for the other fields that don't respect this contract that Mariusz
identified I think that if we wanted to be coherent with the docs we
should likely adapt them as well in a follow up ticket.
For as much as this API is awkward and open to interpretation it has been
around for years so trying to stick to it as much as possible seems like a
worthy approach?
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Replying to [comment:19 Simon Charette]:
> I never assumed that we guaranteed that `get_prep_lookup` would be
called by `get_db_prep_lookup` but I can see how it can be interpreted
this way from the docs.
>
> Given this was an unintended side-effect of
5c23d9f0c32f166c81ecb6f3f01d5077a6084318 I'd be +1 to having
`get_db_prep_value` start by calling `get_prep_value` when
`prepared=False`.
OK, persuaded, let's restore it for `JSONField`.
Julie, please prepare a patch (via GitHub PR) with a regression test and
release notes (`4.2.2.txt`.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(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 Julie Rymer):
Replying to [comment:20 Mariusz Felisiak]:
> Replying to [comment:19 Simon Charette]:
> > I never assumed that we guaranteed that `get_prep_lookup` would be
called by `get_db_prep_lookup` but I can see how it can be interpreted
this way from the docs.
> >
> > Given this was an unintended side-effect of
5c23d9f0c32f166c81ecb6f3f01d5077a6084318 I'd be +1 to having
`get_db_prep_value` start by calling `get_prep_value` when
`prepared=False`.
>
> OK, persuaded, let's restore it for `JSONField`.
>
> Julie, please prepare a patch (via GitHub PR) with a regression test and
release notes (`4.2.2.txt`.)
Cool, I'll try to do this this w-e :)
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:21>
Django
-------------------------------------+-------------------------------------
| Rymer
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------
* owner: (none) => Julie Rymer
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/16858
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0ec60661e61b153e6bcec64649b1b7f524eb3e18" 0ec6066]:
{{{
#!CommitTicketReference repository=""
revision="0ec60661e61b153e6bcec64649b1b7f524eb3e18"
Fixed #34539 -- Restored get_prep_value() call when adapting JSONFields.
Regression in 5c23d9f0c32f166c81ecb6f3f01d5077a6084318.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9c301814b0d342bfc5f10c1513594ce5fbb6c6a9" 9c301814]:
{{{
#!CommitTicketReference repository=""
revision="9c301814b0d342bfc5f10c1513594ce5fbb6c6a9"
[4.2.x] Fixed #34539 -- Restored get_prep_value() call when adapting
JSONFields.
Regression in 5c23d9f0c32f166c81ecb6f3f01d5077a6084318.
Backport of 0ec60661e61b153e6bcec64649b1b7f524eb3e18 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "stack.png" added.
stack frame
--
Ticket URL: <https://code.djangoproject.com/ticket/34539>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Shaheed Haque):
* status: closed => new
* resolution: fixed =>
Comment:
This has caused a behaviour change for me which I **think** may be a
regression. I'm not especially knowledgeable about the issues debated
above, but here is what I see. First, I am using an add-on app called
social_django which sets a JSONField and then saves the value here:
https://github.com/python-social-auth/social-app-
django/blob/8d0a2052d1b22a899454571c62237d23aa25af97/social_django/storage.py#L24
Second, on the stack, I have the frames show in the attachment "stack
frame", and the important point is that when I enter the two new lines of
code added, value is a dict, but the value is flattened into a string by
the new code.
The result is that the database JSONField has the saved string. **I
believe that to be incorrect**. Previously, the dict was saved as
expected.
(Perhaps the intent here was to only have this change apply to subclasses
of JSONField?)
Advice appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Julie Rymer | Owner: Julie
| Rymer
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
Please don't reopen closed tickets. If you believe there is an issue in
Django, then please create a new ticket in Trac and follow our
[https://docs.djangoproject.com/en/dev/internals/contributing/bugs-and-
features/#reporting-bugs bug reporting guidelines].
--
Ticket URL: <https://code.djangoproject.com/ticket/34539#comment:29>