[Django] #25532: JSONField form field double encodes
Django
----------------------------------+-------------------
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+-------------------
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)
--
Ticket URL: <https://code.djangoproject.com/ticket/25532>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
What about not calling `dumps` on string values?
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:1>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by davidszotten):
Replying to [comment:1 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
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:2>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:3>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by davids):
Replying to [comment:3 claudep]:
Looks like it fixes the problem. Can't speak to whether this is the best
solution; what's the next step here?
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:4>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9a1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gabeanzelini):
* cc: me@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:5>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.9a1 => 1.9
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:6>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tbeadle):
* cc: tbeadle@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:7>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by xblitz):
Just got burned by this too
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:8>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by xblitz):
* cc: xblitz@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:9>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tbeadle):
* Attachment "0001-Fix-issue-in-JSONField.patch" added.
This is how I fixed the issue when I ran across it.
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
@tbeadle On invalid input, `json.loads()` will produce a `ValueError`, how
would that solve the issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:10>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:11>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6342 PR]. Thanks Tommy, you gave me
a good track!
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:12>
Django
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left a few comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:14>
Django
-------------------------------------+-------------------------------------
| Paroz <claude@…>
Type: Bug | Status: closed
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* owner: => Claude Paroz <claude@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"db19619545dd99a1d2502c72974d79eca33acff7" db196195]:
{{{
#!CommitTicketReference repository=""
revision="db19619545dd99a1d2502c72974d79eca33acff7"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Type: Bug | Status: closed
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"6a8ba2eef45ccc3022c88510bffd29734d71c39e" 6a8ba2e]:
{{{
#!CommitTicketReference repository=""
revision="6a8ba2eef45ccc3022c88510bffd29734d71c39e"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
The new `test_formfield_disabled` test is failing on the 1.9 branch with
`TypeError: the JSON object must be str, not 'list'`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I knew that test was failing on 1.9 and it was not supposed to be
committed for that reason. Sorry for the mess.
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"97ccab126cb395f3ed5d31ebb37995dcbe27329f" 97ccab12]:
{{{
#!CommitTicketReference repository=""
revision="97ccab126cb395f3ed5d31ebb37995dcbe27329f"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Type: Bug | Status: closed
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a5f85d891b51d7ceb4f9e422e3e4f5c741062288" a5f85d89]:
{{{
#!CommitTicketReference repository=""
revision="a5f85d891b51d7ceb4f9e422e3e4f5c741062288"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: davidszotten | Owner: Claude
| Paroz <claude@…>
Type: Bug | Status: closed
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3744fc1666c41aeb4ed44f31605bafdb413b42bc" 3744fc1]:
{{{
#!CommitTicketReference repository=""
revision="3744fc1666c41aeb4ed44f31605bafdb413b42bc"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25532#comment:22>