Opened 8 years ago
Closed 8 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 , 8 years ago
Has patch: | set |
---|
comment:2 by , 8 years ago
comment:3 by , 8 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 , 8 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 8 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 , 8 years ago
Patch needs improvement: | unset |
---|
comment:7 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:9 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:10 by , 8 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 , 8 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 , 8 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 , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Fixed in pull request: https://github.com/django/django/pull/7012