Opened 8 years ago

Closed 8 years ago

Last modified 8 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 by Claude Paroz, 8 years ago

Triage Stage: UnreviewedAccepted

What about not calling dumps on string values?

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

Replying to claudep:

What about not calling dumps on string values?

because 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

Last edited 8 years ago by David Szotten (previous) (diff)

comment:3 by Claude Paroz, 8 years ago

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.

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

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 by Gabe Anzelini, 8 years ago

Cc: me@… added

comment:6 by Tim Graham, 8 years ago

Version: 1.9a11.9

comment:7 by Tommy Beadle, 8 years ago

Cc: tbeadle@… added

comment:8 by Eric, 8 years ago

Just got burned by this too

comment:9 by Eric, 8 years ago

Cc: xblitz@… added

by Tommy Beadle, 8 years ago

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

comment:10 by Claude Paroz, 8 years ago

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

comment:11 by Claude Paroz, 8 years ago

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 by Claude Paroz, 8 years ago

Has patch: set

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

comment:13 by Tim Graham, 8 years ago

Patch needs improvement: set

Left a few comments for improvement.

comment:14 by Tim Graham, 8 years ago

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

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

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 by Claude Paroz <claude@…>, 8 years ago

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 by Tim Graham, 8 years ago

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 by Claude Paroz, 8 years ago

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 by Claude Paroz <claude@…>, 8 years ago

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 by Claude Paroz, 8 years ago

Resolution: fixed
Status: newclosed

comment:21 by Tim Graham <timograham@…>, 8 years ago

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 by Tim Graham <timograham@…>, 8 years ago

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