Opened 9 years ago
Closed 9 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 by , 9 years ago
| Has patch: | set |
|---|
comment:2 by , 9 years ago
comment:3 by , 9 years ago
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 by , 9 years ago
| Patch needs improvement: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:5 by , 9 years ago
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
comment:6 by , 9 years ago
| Patch needs improvement: | unset |
|---|
comment:7 by , 9 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:8 by , 9 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:9 by , 9 years ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:10 by , 9 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
| Triage Stage: | Accepted → Ready 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 by , 9 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|---|
| Version: | 1.10 → master |
Please don't mark your own patch as RFC. See our contributing guidelines for more details.
PR.
comment:12 by , 9 years ago
| Cc: | added |
|---|---|
| Triage Stage: | Accepted → Ready 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:15 by , 9 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Fixed in pull request: https://github.com/django/django/pull/7012