[Django] #29147: Postgres JSONField missing to_python
Django
--------------------------------------------+------------------------
Reporter: Javier Buzzi | Owner: (none)
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
--------------------------------------------+------------------------
Every other field implements a to_python except for JSONField:
https://github.com/django/django/blob/master/django/contrib/postgres/fields/hstore.py#L38
https://github.com/django/django/blob/master/django/contrib/postgres/fields/array.py#L102
https://github.com/django/django/blob/master/django/contrib/postgres/fields/ranges.py#L47
JSONField defaults to the value, meaning if you pass `'{"object":
"value"}'` you get back `'{"object": "value"}'`. The same example in
HStoreField, you pass in `'{"object": "value"}'` you get back `{"object":
"value"}` -- makes the world of difference.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Williams Mendez):
* owner: (none) => Williams Mendez
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:1>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Component: contrib.postgres | Version: 1.9
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => invalid
Comment:
I don't think that change is correct because
[https://github.com/django/django/blob/16436f3751e9eec67a7b80b25c8e7d29a34be67e/tests/postgres_tests/test_json.py#L46-L50
a string is valid JSON] -- to treat it differently would break backwards
compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:2>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Williams Mendez):
At first I thought the same, but when I try to store a String into a
postgres 'jsonb' field it crashes. Same when I try to fetch a string as a
json:
{{{
will_test=# select 'hello'::json ;
ERROR: invalid input syntax for type json
LINE 1: select 'hello'::json ;
^
DETAIL: Token "hello" is invalid.
CONTEXT: JSON data, line 1: hello
will_test=# select '{}'::json ;
json
------
{}
(1 row)
}}}
I'll do some research on that topic.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:3>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Williams Mendez):
Yeah, it's totally right:
{{{
will_test=# select '"hello"'::json ;
json
---------
"hello"
(1 row)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:4>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
As i understood it was that `to_python()` is the representation of the
data in python, postgres could save it and do with it what it wants; store
it binary/urlencode it/ encrypt it for all it wants, but from a python
perspective that data to me, needs to be represented as a dict/simple type
(if it is "hello") -- i still don't understand why this is invalid.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:5>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham):
Can you be more clear about what the unexpected behavior is? (i.e. give a
failing test for `tests/postgres_tests/test_json.py`)
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:6>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
I added this (copy and paste from hstore.py)
{{{
--- a/django/contrib/postgres/fields/jsonb.py
+++ b/django/contrib/postgres/fields/jsonb.py
@@ -41,6 +41,14 @@ class JSONField(CheckFieldDefaultMixin, Field):
self.encoder = encoder
super().__init__(verbose_name, name, **kwargs)
+ def to_python(self, value):
+ if isinstance(value, str):
+ value = json.loads(value)
+ return value
+
+ def value_to_string(self, obj):
+ return json.dumps(self.value_from_object(obj))
+
def db_type(self, connection):
return 'jsonb'
}}}
ran the tests:
{{{
$ /django/tests/runtests.py postgres_tests.test_json --failfast
--settings=mysettings
Testing against Django installed in '/usr/local/lib/python3.6/site-
packages/Django-2.1.dev20180313024248-py3.6.egg/django' with up to 4
processes
Creating test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
System check identified no issues (0 silenced).
.......................................................
----------------------------------------------------------------------
Ran 55 tests in 0.118s
OK
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
}}}
Here are all the steps on how i got it to work:
{{{
$ docker run --rm -it python:3.6.4 bash
apt-get update
git clone https://github.com/django/django.git
cd django
sed -i "s/^exit 101$/exit 0/" /usr/sbin/policy-rc.d
apt-get install -y libmemcached-dev postgresql vim postgresql-contrib
python setup.py install
pip install -rtests/requirements/py3.txt -rtests/requirements/postgres.txt
echo "SECRET_KEY = 'hellooooo'
DATABASES = {
'default': {
'ENGINE': 'django.db.backends.postgresql_psycopg2',
'NAME': 'django',
'USER': 'django',
'PASSWORD': 'django',
'HOST': 'localhost',
'PORT': 5432,
}
}" > tests/mysettings.py
echo "local all all trust" >>
/etc/postgresql/9.4/main/pg_hba.conf
echo "diff --git a/django/contrib/postgres/fields/jsonb.py
b/django/contrib/postgres/fields/jsonb.py
index 3c27607..49b65ef 100644
--- a/django/contrib/postgres/fields/jsonb.py
+++ b/django/contrib/postgres/fields/jsonb.py
@@ -41,6 +41,14 @@ class JSONField(CheckFieldDefaultMixin, Field):
self.encoder = encoder
super().__init__(verbose_name, name, **kwargs)
+ def to_python(self, value):
+ if isinstance(value, str):
+ value = json.loads(value)
+ return value
+
+ def value_to_string(self, obj):
+ return json.dumps(self.value_from_object(obj))
+
def db_type(self, connection):
return 'jsonb'
" > mychanges.diff
patch django/contrib/postgres/fields/jsonb.py mychanges.diff
service postgresql start
echo "set the password to 'django'"
su postgres -c 'createuser --superuser django -P'
/django/tests/runtests.py postgres_tests.test_json --failfast
--settings=mysettings
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:7>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham):
What behavior changes as a result of the patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:8>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
@Tim, nothing that i can see.
We need this for Aloe, we have a string table to load data for our tests
and we rely heavily on the field's ability to turn a raw/string value to a
python value.
Example:
{{{
from django.db.models import BooleanField, CharField, NumberField
from django.contrib.postgres import fields
CharField.to_python(None, 'hello')
'hello'
IntegerField.to_python(None, '123')
123
BooleanField.to_python(None, 'True')
True
fields.HStoreField.to_python(None, '{"hello": "world"}')
{'hello': 'world'}
}}}
The only thing that breaks that pattern in the entire lib, is `JSONField`:
{{{
fields.JSONField.to_python(None, '{"hello": "world"}')
'{"hello": "world"}'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:9>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham):
I believe your proposal is backwards incompatible. Assuming that a string
should be parsed as a JSON dict rather than treating a string as a string
could be undesired. Your patch results in a test failure of
`postgres_tests.test_json.TestSerialization.test_dumping`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:10>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
Please explain, i don't see that on my side. (using the same setup from
above)
{{{
# /django/tests/runtests.py
postgres_tests.test_json.TestSerialization.test_dumping --failfast
--settings=mysettings
Testing against Django installed in '/usr/local/lib/python3.6/site-
packages/Django-2.1.dev20180320161010-py3.6.egg/django' with up to 4
processes
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.
----------------------------------------------------------------------
Ran 1 test in 0.004s
OK
Destroying test database for alias 'default'...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:11>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham):
In your patch, I think you missed the `JSONField.value_to_string()` that's
already defined. It overrides the one you added since it's declared below
yours. The test failure happens if you remove the existing
`value_to_string()` method in favor of yours. Anyway, if you provide a
pull request that passes all tests and includes a test for the new
behavior, I'll take a closer look to see if this is suitable.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:12>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
Thanks Tim.
Here it is: https://github.com/django/django/pull/9801
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:13>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Javier Buzzi):
@Tim, can you look at the PR https://github.com/django/django/pull/9801
Any comments, concerns, and/or changes, will be appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:14>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Javier Buzzi):
* has_patch: 0 => 1
Old description:
> Every other field implements a to_python except for JSONField:
>
> https://github.com/django/django/blob/master/django/contrib/postgres/fields/hstore.py#L38
> https://github.com/django/django/blob/master/django/contrib/postgres/fields/array.py#L102
> https://github.com/django/django/blob/master/django/contrib/postgres/fields/ranges.py#L47
>
> JSONField defaults to the value, meaning if you pass `'{"object":
> "value"}'` you get back `'{"object": "value"}'`. The same example in
> HStoreField, you pass in `'{"object": "value"}'` you get back `{"object":
> "value"}` -- makes the world of difference.
New description:
Every other field implements a to_python except for JSONField:
https://github.com/django/django/blob/master/django/contrib/postgres/fields/hstore.py#L38
https://github.com/django/django/blob/master/django/contrib/postgres/fields/array.py#L102
https://github.com/django/django/blob/master/django/contrib/postgres/fields/ranges.py#L47
JSONField defaults to the value, meaning if you pass `'{"object":
"value"}'` you get back `'{"object": "value"}'`. The same example in
HStoreField, you pass in `'{"object": "value"}'` you get back `{"object":
"value"}` -- makes the world of difference.
PR. https://github.com/django/django/pull/9801
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:15>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"623139b5d1bd006eac78b375bcaf5948e695c3c6" 623139b]:
{{{
#!CommitTicketReference repository=""
revision="623139b5d1bd006eac78b375bcaf5948e695c3c6"
Refs #29147 --- Added JSONField serialization tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:16>
Django
Reporter: Javier Buzzi | Owner: Williams
| Mendez
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham):
The tests committed above demonstrate why this proposal is backwards
incompatible. I think if your application makes the assumption you've
articulated about your data, you'll have to add your own parsing.
--
Ticket URL: <https://code.djangoproject.com/ticket/29147#comment:17>