[Django] #35946: Postgres ArrayField should convert values when given a list

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Type: Bug
Status: new | Component:
| contrib.postgres
Version: | Severity: Normal
Keywords: ArrayField | Triage Stage:
to_python | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Currently ArrayField.to_python only calls base_field.to_python when given
string. In my opinion it should call it always. Currently using this field
is inconsistent. Simple example:
{{{
f = MyModel._meta.get_field("int_array")
f.clean('[1, "2", 3]', None) # ok
f.clean([1, "2", 3], None) # error
}}}

Proposed ArrayField.to_python, witch always calls
base_field.to_python(val):
{{{
def to_python(self, value):
if isinstance(value, str):
value = json.loads(value)
return [self.base_field.to_python(val) for val in value]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35946>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------

Reporter: Zerq | Owner: Tanish
| Yelgoe
Type: Bug | Status: assigned
Component: contrib.postgres | Version:
Severity: Normal | Resolution:

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

Changes (by Tanish Yelgoe):

* owner: (none) => Tanish Yelgoe
* status: new => assigned

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

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Owner: Tanish
| Yelgoe

Type: Bug | Status: closed
Component: contrib.postgres | Version:
Severity: Normal | Resolution: invalid

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

Changes (by Sarah Boyce):

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

Comment:

Hi Zerq, thank you for the ticket!

The string case is commented with `# Assume we're deserializing` and
that's why the `to_python` is called (hence the inconsistency, there's
different contexts)
Your suggested change is making us accept more values that we would
previously reject (which could be a regression if folks rely on this
behavior)
In short, I believe this by design and not a bug
--
Ticket URL: <https://code.djangoproject.com/ticket/35946#comment:2>

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Owner: Tanish
| Yelgoe
Type: Bug | Status: closed
Component: contrib.postgres | Version:
Severity: Normal | Resolution: invalid
Keywords: ArrayField | Triage Stage:
to_python | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Zerq):

Replying to [comment:2 Sarah Boyce]:

> Hi Zerq, thank you for the ticket!
>
> The string case is commented with `# Assume we're deserializing` and
that's why the `to_python` is called (hence the inconsistency, there's
different contexts)
> Your suggested change is making us accept more values that we would
previously reject (which could be a regression if folks rely on this
behavior)
> In short, I believe this by design and not a bug

Thanks for your replay.

I agree that it would be breaking change. But currently it is inconsistent
with how fields operate and that makes it a trap. This is why I think it
should be patched in new release as breaking change or at least warning
added to documentation. `to_python` is not only called during
deserializing, but as a normal cleaning process
(https://docs.djangoproject.com/en/5.1/howto/custom-model-fields
/#converting-values-to-python-objects). Users can expect that values will
be coerced to proper type when assigned to model and cleaned. Custom
fields can also preprocess values in this method (example:
https://github.com/django/django-
localflavor/blob/085fb2be02cae1c95594c2a358b8df8d62fa5577/localflavor/es/models.py#L43).
This field will work fine alone, but not in `ArrayField`.

Example with different `base_field`:
{{{
class MyModel(models.Model):
boolean = models.BooleanField()
bool_array = ArrayField(models.BooleanField())

def test_my_model():
m = MyModel()
value = "false" # value read from network
m.boolean = value # ok
m.bool_array = [value] # error
m.full_clean()
}}}

I see no design reason why `ArrayField` should decide what values are
acceptable or skip preprocess on its own.

I do not try to force this change, but ask to give it second thought. For
me it is a bug, breaking how fields work within `ArrayField`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35946#comment:3>

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Owner: Tanish
| Yelgoe

Type: Bug | Status: new

Component: contrib.postgres | Version:
Severity: Normal | Resolution:

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

Changes (by Zerq):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/35946#comment:4>

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Owner: Tanish
| Yelgoe

Type: Bug | Status: closed
Component: contrib.postgres | Version:
Severity: Normal | Resolution: wontfix

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

Changes (by Sarah Boyce):

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

Comment:

Looking at the linked docs: (https://docs.djangoproject.com/en/5.1/howto
/custom-model-fields/#converting-values-to-python-objects

>to_python() is called by deserialization and during the clean() method
used from forms.
>As a general rule, to_python() should deal gracefully with any of the
following arguments:
>
>* An instance of the correct type (e.g., Hand in our ongoing example).
>
>* A string

Does it handle an instance of the correct type?
Yes, `[1, 2, 3]` would be correct in terms of a ArrayField with an
IntegerField base_field, and `[1, "2", 3]` is not a valid array field with
integer base

Does it handle a deserialization (the string case) correctly?
Yes, when we serialize an ArrayField with `[1, 2, 3]` we get `'["1", "2",
"3"]'` which is handled to be valid.

To me, the odd case is that `'[1, "2", 3]'` valid. That's because we get
`'[1, "2", 3]'` apply `json.load` and get `[1, "2", 3]` and then the
`to_python` of the base fields handle both strings (as the deserializer
case) but also the case that it just received a value of the correct
instance. We _possibly_ could handle this. I am not sure the cost benefit.

So I am adjusting this to "wontfix". This means you can start a new
conversation on the [https://forum.djangoproject.com/c/internals/5 Django
Forum], where you'll reach a broader audience and receive additional
feedback, if there is consensus that we should change this. You can reopen
the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/35946#comment:5>
Django <https://code.djangoproject.com/>
The web framework for perfectionists with deadlines.

[Django]

#35946: Postgres ArrayField should convert values when given a list
-------------------------------------+-------------------------------------
Reporter: Zerq | Owner: Tanish
| Yelgoe
Type: Bug | Status: closed
Component: contrib.postgres | Version:
Severity: Normal | Resolution: wontfix
Keywords: ArrayField | Triage Stage:
to_python | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* cc: Mike Lissner (added)

Comment:

There are similar issues with `HStoreField.to_python()` and
`CompositePrimaryKey.to_python()`.

By contrast, `RangeField.to_python()` *does* run `to_python()` on
individual components when given a list.

More interestingly, various ''form'' fields, including for `ArrayField`
itself (`postgres.forms.SimpleArrayField`), run `to_python()` on
individual components when given a list.

I think this is ripe for a standardization. I'll try to get some findings
into a forum post to support reframing this one.

I ended up here during review of #36865--which is going to start
leveraging `to_python()` more during admin searches--and investigating the
behavior difference between model `to_python()` and form `to_python()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35946#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.