Opened 2 years ago

Closed 2 years ago

Last modified 8 days ago

#34397 closed Bug (invalid)

Subclasses of JSONField call `get_prep_value` with field as value

Reported by: pheki Owned by: nobody
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi!

Let's say I have a class called PhoneNumber:

@dataclass
class PhoneNumber:
    country_iso_code: str
    national_number: int

I create a field I use to use PhoneNumber in my database:

class PhoneNumberField(JSONField):
    def get_prep_value(
        self, value: Optional[PhoneNumber]
    ) -> Optional[JsonAdapter]:
        if isinstance(value, PhoneNumber):
            return JsonAdapter(dataclasses.asdict(value), encoder=CustomJSONEncoder)
        elif value is None and self.null:
            return None
        else:
            raise Exception("Fetching phone number with unexpected value")

    # Similar from_db_value ommited
    def from_db_value(...):
        ...

The problem is that when I do a KeyTextTransform to get a field, it will call get_prep_value with that field and AFAIK there's no way to know which field is it getting. For from_db_value the same happens, but it's fine as you can inspect the expression column. Example:

User.objects.annotate(
    national_number=KeyTextTransform("national_number", "phone_number")
).filter(
    national_number="12345678"
)[0]

Will call field.get_prep_value("12345678"), forcing the only way to make it work correctly to silently ignore unexpected types. This didn't happen on django 2.2 for django.contrib.postgres.fields.jsonb.JSONField.

Change History (3)

comment:1 by Mariusz Felisiak, 2 years ago

Resolution: invalid
Status: newclosed

If I understand correctly, you have a custom JSONField with your own implementation of get_prep_value(). JSONField no longer use get_prep_value() in Django 4.2+ (see 5c23d9f0c32f166c81ecb6f3f01d5077a6084318) so you check if it works for you, however, even if it doesn't, you can always implement your own expression and use it instead of KeyTextTransform.

comment:2 by Julie Rymer, 22 months ago

This issue was filed for django 3.2 so the recent change in JSONField behaviour has no impact.
However, from what I understand @pheki, wants two different behaviour depending on the field it is called on. For that the solution is to create two custom classes, each implementing the behaviour that you want and assign it to the corresponding field.

comment:3 by pheki, 8 days ago

Sorry for the very-very-late response, but I never took the time to revisit this before and it may be useful for future reference.

This issue was filed for django 3.2 so the recent change in JSONField behaviour has no impact.

Yes, that's true, this is was a difference in behavior between Django 2.2's Postgres JSONField and 3.2's JSONField, which I realized as I was working on a codebase update. At the same time it was useful information as, even though 5.0 re-introduced get_prep_value, 4.2 is a kind of necessary stepping stone.

However, from what I understand @pheki, wants two different behaviour depending on the field it is called on. For that the solution is to create two custom classes, each implementing the behaviour that you want and assign it to the corresponding field.

Not exactly, I explained pretty badly but here's a second try: I have a single field that inherits JSONField and I'm using get_prep_value and from_db_value to autoconvert the JSON to/from a dataclass. The issue was that, when using KeyTextTransform and a filter or just a field lookup, it sends the value to get_prep_value without any way to know its a field lookup, so the conversion fails. You may be receiving a value that's really internal in the JSON, not the whole dict.

My first solution was just to ignore errors on get_prep_value, but that opens a margin for users using the field incorrectly. I later changed it to use the encoder directly. In the end, I think that even though it makes it harder to use correctly, at least it's consistent with other field types. e.g. TextField lookups (e.g. __contains) also calls get_prep_db with the lookup value and no way to check if it's a "field value" or a "lookup value".

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