Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#26949 closed Bug (fixed)

Disabled forms.JSONField crashes with TypeError

Reported by: Tim Graham Owned by: Olexander Yermakov
Component: contrib.postgres Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The fix for #25532 is partially reverted to fix the regression in #26917 but this means the following test fails with TypeError: the JSON object must be str, not 'list'.:

def test_formfield_disabled(self):
    class JsonForm(Form):
        name = CharField()
        jfield = forms.JSONField(disabled=True)

    form = JsonForm({'name': 'xyz', 'jfield': '["bar"]'}, initial={'jfield': ['foo']})
    self.assertIn('[&quot;foo&quot;]</textarea>', form.as_p())

Attachments (1)

0001-Fix-disabled-forms.JSONField-crash-with-TypeError.patch (810 bytes) - added by Olexander Yermakov 3 years ago.
As I see the problem with previous fix was because assumption that initial values are already cleaned is wrong. I added these lines to omit json.loads() for disable field. But I think that's the obvious fix, means there are downsides here if it's not been done before, so I'm not sure about it. Could you, please, advice what could be wrong with this approach? Thanks!

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by Olexander Yermakov

Owner: set to Olexander Yermakov
Status: newassigned

Changed 3 years ago by Olexander Yermakov

As I see the problem with previous fix was because assumption that initial values are already cleaned is wrong. I added these lines to omit json.loads() for disable field. But I think that's the obvious fix, means there are downsides here if it's not been done before, so I'm not sure about it. Could you, please, advice what could be wrong with this approach? Thanks!

comment:2 Changed 3 years ago by Tim Graham

Has patch: set

Looks okay to me. I added that fix to my PR for the other ticket. Thanks!

comment:3 Changed 3 years ago by Olexander Yermakov

Should I create PR for the 1.9 branch or it's not required?

comment:4 Changed 3 years ago by Tim Graham

Not needed since the committer will backport any commits as needed. I think I'll just backport to 1.10 only since 1.9.x will be in security fix only mode after the 1.10 release next week.

comment:5 Changed 3 years ago by Olexander Yermakov

Ok. Thanks for the fast and clear response!

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

In a5f85d89:

Fixed #26917 -- Fixed crash in disabled ModelChoiceFields.

Partially reverted refs #25532 to fix a regression in Django 1.10.
This reintroduces a crash for disabled forms.JSONField (refs #26949),
however, that issue is also present on Django 1.9.

Thanks Ryan Schave for the test.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 68de48c:

Fixed #26949 -- Fixed crash of disabled forms.JSONField.

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

In 3744fc1:

[1.10.x] Fixed #26917 -- Fixed crash in disabled ModelChoiceFields.

Partially reverted refs #25532 to fix a regression in Django 1.10.
This reintroduces a crash for disabled forms.JSONField (refs #26949),
however, that issue is also present on Django 1.9.

Thanks Ryan Schave for the test.

Backport of a5f85d891b51d7ceb4f9e422e3e4f5c741062288 from master

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 714e287d:

[1.10.x] Fixed #26949 -- Fixed crash of disabled forms.JSONField.

Backport of 68de48c96328e13d5dbdb1f3006e4a1ca74f3c34 from master

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