Opened 6 years ago

Closed 6 years ago

#27003 closed Bug (fixed)

ArrayField and JSONField form fields fail on already converted values

Reported by: Brandon Chinn Owned by:
Component: Forms Version: dev
Severity: Normal Keywords: postgres, arrayfield
Cc: emad.m.habib@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

If I make a model Foo with the PostgreSQL ArrayField,

class Foo(models.Model):
    bar = ArrayField(models.CharField(...))

And create a ModelForm:

FooForm = modelform_factory(Foo, excludes=[])

The field will fail with already-converted values, like

foo = Foo.objects.create(bar=['a', 'b'])
data = model_to_dict(foo) # {'bar': ['a', 'b']}
form = FooForm(instance=foo, data=data)
form.full_clean() # errors at django/contrib/postgres/forms/array.py:39

Shouldn't already converted values be checked in the to_python method of ArrayField? (Same with JSONField)

Change History (15)

comment:1 Changed 6 years ago by Brandon Chinn

Has patch: set

comment:2 Changed 6 years ago by Brandon Chinn

comment:3 Changed 6 years ago by Claude Paroz

I think the solution you proposed in your patch is good for ArrayField, but it might be more tricky for JSONField as a string can be both a converted and an unconverted value, so the isinstance is not covering all cases (see #25532).

comment:4 Changed 6 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:5 Changed 6 years ago by Brandon Chinn

For JSONField, what about making a custom six.text_type for converted strings, like #25532:

class JsonStr(six.text_type):
    pass

def to_python(self, value):
    ...
    if isinstance(value, JsonStr):
        return value
    val = json.loads(value)
    if isinstance(val, str):
        return JsonStr(val)
    else:
        return val
Last edited 6 years ago by Brandon Chinn (previous) (diff)

comment:6 Changed 6 years ago by Tim Graham

Patch needs improvement: unset

comment:7 Changed 6 years ago by MaartenPI

Owner: changed from nobody to MaartenPI
Status: newassigned

comment:8 Changed 6 years ago by MaartenPI

Owner: MaartenPI deleted
Status: assignednew

comment:9 Changed 6 years ago by Josh Harwood

Owner: set to Josh Harwood
Status: newassigned

comment:10 Changed 6 years ago by Josh Harwood

Owner: Josh Harwood deleted
Status: assignednew
Triage Stage: AcceptedReady for checkin

Work is all done, just needs a minor change to the name of the type used, however I can't edit this PR, only the merged code so I've removed myself from this ticket.

comment:11 Changed 6 years ago by Simon Charette

Triage Stage: Ready for checkinAccepted
Version: 1.10master

Please don't mark your own patch as RFC. See our contributing guidelines for more details.

PR.

comment:12 Changed 6 years ago by Emad Mokhtar

Cc: emad.m.habib@… added
Triage Stage: AcceptedReady for checkin

I ran the tests with changes nothing failed and ran the same tests without changes and it failed.

Note: I ran tests using PostgreSQL 9.3

comment:13 Changed 6 years ago by Tim Graham <timograham@…>

In 6573274:

Refs #27003 -- Fixed SimpleArrayField crash on converted values.

comment:14 Changed 6 years ago by Tim Graham <timograham@…>

In eed61500:

Refs #27003 -- Fixed JSONField crash on converted values.

comment:15 Changed 6 years ago by Tim Graham

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top