Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#25532 closed Bug (fixed)

JSONField form field double encodes invalid values

Reported by: David Szotten Owned by: Claude Paroz <claude@…>
Component: contrib.postgres Version: 1.9
Severity: Normal Keywords:
Cc: me@…, tbeadle@…, xblitz@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

the JSONField form field double-encodes invalid json strings,

e.g. entering the string {"foo"} and submitting the form, it comes back with an error, but the form field populated with the string "{\"foo\"}"

not sure how to fix, since prepare_value doesn't know if we have valid json (and we, like postgres, but unlike the json spec consider bare strings as valid json)

Attachments (1)

0001-Fix-issue-in-JSONField.patch (496 bytes) - added by Tommy Beadle 8 years ago.
This is how I fixed the issue when I ran across it.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

What about not calling dumps on string values?

comment:2 in reply to:  1 Changed 8 years ago by David Szotten

Replying to claudep:

What about not calling dumps on string values?

because (as i mentioned) bare strings are allowed as valid json (e.g. "foo") this would instead cause problems for such values, which would get stripped of their quotes

Version 0, edited 8 years ago by David Szotten (next)

comment:3 Changed 8 years ago by Claude Paroz

I made a tentative patch here: https://github.com/django/django/compare/master...claudep:25532

If we would choose this way, we would have to provide a deprecation path for custom fields without kwargs for prepare_value, even if it seems currently undocumented.

comment:4 in reply to:  3 Changed 8 years ago by David S

Replying to claudep:

Looks like it fixes the problem. Can't speak to whether this is the best solution; what's the next step here?

comment:5 Changed 8 years ago by Gabe Anzelini

Cc: me@… added

comment:6 Changed 8 years ago by Tim Graham

Version: 1.9a11.9

comment:7 Changed 8 years ago by Tommy Beadle

Cc: tbeadle@… added

comment:8 Changed 8 years ago by Eric

Just got burned by this too

comment:9 Changed 8 years ago by Eric

Cc: xblitz@… added

Changed 8 years ago by Tommy Beadle

This is how I fixed the issue when I ran across it.

comment:10 Changed 8 years ago by Claude Paroz

@tbeadle On invalid input, json.loads() will produce a ValueError, how would that solve the issue?

comment:11 Changed 8 years ago by Claude Paroz

I see now, your patch is a fix when the form is redisplayed because of another invalid value, but not when the JSON input itself is wrong, right?

comment:12 Changed 8 years ago by Claude Paroz

Has patch: set

PR. Thanks Tommy, you gave me a good track!

comment:13 Changed 8 years ago by Tim Graham

Patch needs improvement: set

Left a few comments for improvement.

comment:14 Changed 8 years ago by Tim Graham

Patch needs improvement: unset
Summary: JSONField form field double encodesJSONField form field double encodes invalid values
Triage Stage: AcceptedReady for checkin

comment:15 Changed 8 years ago by Claude Paroz <claude@…>

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In db196195:

Fixed #25532 -- Properly redisplayed JSONField form input values

Thanks David Szotten for the report and Tommy Beadle for code inspiration.
Thanks Tim Graham for the review.

comment:16 Changed 8 years ago by Claude Paroz <claude@…>

In 6a8ba2e:

[1.9.x] Fixed #25532 -- Properly redisplayed JSONField form input values

Thanks David Szotten for the report and Tommy Beadle for code inspiration.
Thanks Tim Graham for the review.
Partial backport of db19619545 from master.

comment:17 Changed 8 years ago by Tim Graham

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

The new test_formfield_disabled test is failing on the 1.9 branch with TypeError: the JSON object must be str, not 'list'.

comment:18 Changed 8 years ago by Claude Paroz

I knew that test was failing on 1.9 and it was not supposed to be committed for that reason. Sorry for the mess.

comment:19 Changed 8 years ago by Claude Paroz <claude@…>

In 97ccab12:

[1.9.x] Refs #25532 -- Removed a failing test on Django 1.9

That test is failing on Django 1.9, that issue has been fixed on master
only as it touches a part of form validation that is too important to
touch for a stable release.

comment:20 Changed 8 years ago by Claude Paroz

Resolution: fixed
Status: newclosed

comment:21 Changed 7 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:22 Changed 7 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

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