Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem when iterating if default value is '""' on postgresql-9.3 #70

Closed
wants to merge 1 commit into from
Closed

Problem when iterating if default value is '""' on postgresql-9.3 #70

wants to merge 1 commit into from

Conversation

toopy
Copy link

@toopy toopy commented Mar 19, 2014

In [1]: from my_app.models import MyModel

In [2]: [fo for fo in MyModel.objects.all()]

ValidationError Traceback (most recent call last)
in ()
----> 1 [fo for fo in MyModel.objects.all()]

/home/florent/.virtualenvs//local/lib/python2.7/site-packages/django/db/models/query.pyc in iter(self)
94 - Responsible for turning the rows into model objects.
95 """
---> 96 self._fetch_all()
97 return iter(self._result_cache)
98

/home/florent/.virtualenvs//local/lib/python2.7/site-packages/django/db/models/query.pyc in _fetch_all(self)
852 def _fetch_all(self):
853 if self._result_cache is None:
--> 854 self._result_cache = list(self.iterator())
855 if self._prefetch_related_lookups and not self._prefetch_done:
856 self._prefetch_related_objects()

/home/florent/.virtualenvs//local/lib/python2.7/site-packages/django/db/models/query.pyc in iterator(self)
228 obj = model_cls(*_dict(zip(init_list, row_data)))
229 else:
--> 230 obj = model(_row_data)
231
232 # Store the source database of the object

/home/florent/.virtualenvs//local/lib/python2.7/site-packages/django/db/models/base.pyc in init(self, _args, *_kwargs)
345 # without changing the logic.
346 for val, field in zip(args, fields_iter):
--> 347 setattr(self, field.attname, val)
348 else:
349 # Slower, kwargs-ready version.

/home/florent/Work//django-jsonfield/jsonfield/subclassing.pyc in set(self, obj, value)
39 # we can definitively tell if a value has already been deserialized
40 # More: #33
---> 41 obj.dict[self.field.name] = self.field.pre_init(value, obj)
42
43

/home/florent/Work//django-jsonfield/jsonfield/fields.pyc in pre_init(self, value, obj)
75 return json.loads(value, **self.load_kwargs)
76 except ValueError:
---> 77 raise ValidationError(_("Enter valid JSON"))
78
79 return value

ValidationError: [u'Enter valid JSON']

@toopy
Copy link
Author

toopy commented Mar 19, 2014

postgres@florent:/home/florent/$ psql my_db
psql (9.3.3)
Saisissez « help » pour l'aide.

my_db=# update my_app_mymodel set field_data = '""'::json;
UPDATE 37
my_db=#

@toopy
Copy link
Author

toopy commented Mar 19, 2014

We should admit empty strings can be stored by default in the DB.

@chris-maestro
Copy link

There is definitely an issue with postgresql-9.3 when there is a blank=True empty value.

@bradjasper
Copy link
Collaborator

Thanks for the pull request @toopy — I think this might be related to #66 but am not entirely sure (I can't reproduce either yet).

Do you have a test case that shows this problem you can help me out with?

@ellisonleao
Copy link

I'm having the same issue here, and i'm using MySQL. I have a JSONField with a default value of '{}' and when i generate a new migration to add this field, the '{}' is not recorded on the database. And when i try to fetch the data, the empty values throws those errors.

@toopy
Copy link
Author

toopy commented Apr 9, 2014

Hello, in my case here is the real issue:
https://github.com/bradjasper/django-jsonfield/blob/master/jsonfield/fields.py#L140

I think you should only use default column type or text column for json field. Otherwise you will encounter real problems. In PG, json column are binary, offer a lot of interesting features at a raw level, but django can't handle them for now, some raw sql generated queries are cleary incompatible with json column. For example, first DISTINCT statement will raise an ugly error and you will need to rollback your transaction manually:/.

Examples of queries on json column:
http://michael.otacoo.com/postgresql-2/postgres-9-3-feature-highlight-json-operators/

If you want I can send you a new PR about it.

@bradjasper
Copy link
Collaborator

@toopy I think improved JSON support for PG is being worked on: https://www.kickstarter.com/projects/mjtamlyn/improved-postgresql-support-in-django

If this can wait until 1.8 or 1.9 we can see if that improves things before doing the backwards incompatible change (json column -> text column). Thoughts?

@toopy
Copy link
Author

toopy commented Apr 9, 2014

Yes, the kickstarter project is a good thing. but imo it's bad to wait for
that support.

at the opposite, we enlight the update to do. I mean, remove the pg-9.3
test to create a json column, until django support it like a charm. Then
the test should be: if pg >= 9.3 and django >= 1.8 (or even higher) use the
json column type.

Otherwise, it means we need to fork the django-jsonfield or handle our json
content our way to move from pg-9.1 to pg-9.3 on production :/
Le 9 avr. 2014 22:33, "Brad Jasper" [email protected] a écrit :

@toopy https://github.com/toopy I think improved JSON support for PG is
being worked on:
https://www.kickstarter.com/projects/mjtamlyn/improved-postgresql-support-in-django

If this can wait until 1.8 or 1.9 we can see if that improves things
before doing the backwards incompatible change (json column -> text
column). Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com//pull/70#issuecomment-40012892
.

@bradjasper
Copy link
Collaborator

@toopy I agree it's not a great solution, but the alternative is to make a pretty huge backwards incompatible change. To do that we'd need a pretty good plan on how to migrate your data.

Since you'd be doing this anyway, do you want to investigate this? What exactly are the issues with switching back from JSON column type to TEXT?

If we can outline the main issues, I'd be ok making this change in the next major update.

@toopy
Copy link
Author

toopy commented Apr 10, 2014

@bradjasper Finding work around and wait better pg support is not an option. On another hand ensure a good support here, why not, but it s not an easy task.

Here is a simple project we can iterate on to check everything works properly.

https://github.com/toopy/mysite-jsonfield

The dummy.py illustrate some errors, It looks easy not to do theses queries I agree. But when you a big app, a lot of tables, relations, entries .. etc. even if you have a good coverage, it looks very risky to move from 9.1 (our case) to 9.3 on that basis.

At the opposite move from limited json column to 'basic' text column should change nothing (I you don't already wrote raw json sql queries;).

(note for me: adds unittest to replace the dummy module).

@bradjasper
Copy link
Collaborator

I've got a forked version that makes this change: https://github.com/bradjasper/django-jsonfield/tree/postgresql

This will be kept up-to-date with master and merged in on the next major release (likely a few weeks ago).

Suggestions on how to minimize pain are welcome—feel free to reopen issue or create a new one.

@bradjasper bradjasper closed this May 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants