Drop jsonfield package for django's native JSONField #2824

patrick-emmanuel · 2018-09-11T16:38:18Z

I want to merge this change because...

resolve #2802
I could not completely remove the package from requirements.txt because django-prices depends on it.

Screenshots

Pull Request Checklist

Privileged views and APIs are guarded by proper permission checks.
All visible strings are translated with proper context.
All data-formatting is locale-aware (dates, numbers, and so on).
Database queries are optimized and the number of queries is constant.
The changes are tested.
The code is documented (docstrings, project documentation).

patrys · 2018-09-11T16:41:21Z

I don't think you planned to include the package-lock.json change with a purely backend PR.

Edit: Thank you for picking that up!

codecov · 2018-09-11T16:51:45Z

Codecov Report

Merging #2824 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2824   +/-   ##
=======================================
  Coverage   89.41%   89.41%           
=======================================
  Files         207      207           
  Lines       10486    10486           
  Branches     1011     1011           
=======================================
  Hits         9376     9376           
  Misses        796      796           
  Partials      314      314

Impacted Files	Coverage Δ
saleor/checkout/models.py	`98.76% <100%> (ø)`	⬆️
saleor/order/models.py	`95.76% <100%> (ø)`	⬆️
saleor/menu/models.py	`83.6% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e526f24...5a7c819. Read the comment docs.

Pacu2 · 2018-09-11T18:01:16Z

Looks great, just one thing, we've just merged #2811, which introduced another JSONFieldon OrderEvents model, do you mind rebasing this PR against latest master and changing it in there too?

Also, I think that jsonfield should be dropped from the requirements as well.

patrick-emmanuel · 2018-09-11T19:06:33Z

Sure, not a problem.
As regards dropping it from the requirements, django-prices-vatlayer has a dependency on it. It'll break the code.

Pacu2 · 2018-09-11T19:11:03Z

I think we might just drop it from requirements.in then, as it’s not required directly by Saleor

patrick-emmanuel · 2018-09-11T19:17:06Z

Alright, on it.

Pacu2 · 2018-09-11T20:30:31Z

saleor/order/models.py

-        blank=True, default={},
-        dump_kwargs={'cls': CustomJsonEncoder, 'separators': (',', ':')})
+        blank=True, default=dict,
+        encoder=CustomJsonEncoder)


I think it will all fit on line 331.

Also, I suppose we need to generate a new migration to reflect the encoder change on the parameters field, don't we?

patrick-emmanuel · 2018-09-11T20:48:04Z

Review has been factored in. Please take a look again.

Pacu2 · 2018-09-11T20:56:08Z

Looks good to me, thanks for contributing to Saleor!

Will merge the PR tomorrow.

patrick-emmanuel · 2018-09-11T21:32:24Z

Happy to help!

Pacu2 · 2018-09-12T07:14:40Z

There's a problem when migrating from the old instance with JSON data created (eg. checkout from the master, generate data via populatedb, pull this branch, run new migrations)

In [3]: Menu.objects.first().json_content
Out[3]: '"\\"[{\\\\\\"url\\\\\\": \\\\\\"/en/products/category/apparel-1/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Apparel\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": []}, {\\\\\\"url\\\\\\": \\\\\\"/en/products/category/accessories-2/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Accessories\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": []}, {\\\\\\"url\\\\\\": \\\\\\"/en/products/category/groceries-3/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Groceries\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": [{\\\\\\"url\\\\\\": \\\\\\"/en/products/category/coffees-4/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Coffees\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": []}, {\\\\\\"url\\\\\\": \\\\\\"/en/products/category/candies-5/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Candies\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": []}]}, {\\\\\\"url\\\\\\": \\\\\\"/en/products/category/books-6/\\\\\\", \\\\\\"name\\\\\\": \\\\\\"Books\\\\\\", \\\\\\"translations\\\\\\": {}, \\\\\\"child_items\\\\\\": []}]\\""'

The ugly quick fix I've tried

In [13]: json.loads(json.loads(json.loads(Menu.objects.first().json_content)))
Out[13]:
[{'child_items': [],
  'name': 'Apparel',
  'translations': {},
  'url': '/en/products/category/apparel-1/'},
 {'child_items': [],
  'name': 'Accessories',
  'translations': {},
  'url': '/en/products/category/accessories-2/'},
 {'child_items': [{'child_items': [],
    'name': 'Coffees',
    'translations': {},
    'url': '/en/products/category/coffees-4/'},
   {'child_items': [],
    'name': 'Candies',
    'translations': {},
    'url': '/en/products/category/candies-5/'}],
  'name': 'Groceries',
  'translations': {},
  'url': '/en/products/category/groceries-3/'},
 {'child_items': [],
  'name': 'Books',
  'translations': {},
  'url': '/en/products/category/books-6/'}]

But have not investigated what are the differences between jsonfield's JSONField and the one that Django offers.

patrick-emmanuel · 2018-09-12T08:55:21Z

Alright. I will look into this.

patrick-emmanuel · 2018-09-12T15:05:19Z

I used django's interactive shell and tried the following code:

from saleor.menu.models import Menu
val = Menu.objects.first().json_content
print(val)

This is the result I got running populatedb after applying the new migrations.

[{"url": "/en/products/category/apparel-1/", "name": "Apparel", "translations": {}, "child_items": []}, {"url": "/en/products/category/accessories-2/", "name": "Accessories", "translations": {}, "child_items": []}, {"url": "/en/products/category/groceries-3/", "name": "Groceries", "translations": {}, "child_items": [{"url": "/en/products/category/coffees-4/", "name": "Coffees", "translations": {}, "child_items": []}, {"url": "/en/products/category/candies-5/", "name": "Candies", "translations": {}, "child_items": []}]}, {"url": "/en/products/category/books-6/", "name": "Books", "translations": {}, "child_items": []}]

Running populatedb before applying migrations I got this:

"[{\"url\": \"/en/products/category/apparel-1/\", \"name\": \"Apparel\", \"translations\": {}, \"child_items\": []}, {\"url\": \"/en/products/category/accessories-2/\", \"name\": \"Accessories\", \"translations\": {}, \"child_items\": []}, {\"url\": \"/en/products/category/groceries-3/\", \"name\": \"Groceries\", \"translations\": {}, \"child_items\": [{\"url\": \"/en/products/category/coffees-4/\", \"name\": \"Coffees\", \"translations\": {}, \"child_items\": []}, {\"url\": \"/en/products/category/candies-5/\", \"name\": \"Candies\", \"translations\": {}, \"child_items\": []}]}, {\"url\": \"/en/products/category/books-6/\", \"name\": \"Books\", \"translations\": {}, \"child_items\": []}]"

Still unsure, still investigating.

Edit: please can you share with me where the populatedb command is configured?

Pacu2 · 2018-09-12T15:11:10Z

Steps to reproduce for me:

Checkout to latest master branch
Fill some JSONField with data (eg. create Menus using populatedb command)
Download this branch, apply its migration
Open shell and check json_content of the Menu
It should not be a valid JSON (at least in my case)

Is that how you've reproduced it?

Pacu2 · 2018-09-12T16:06:49Z

@the-bionic
Populatedb command lives in
https://github.com/mirumee/saleor/blob/master/saleor/core/management/commands/populatedb.py

patrick-emmanuel · 2018-09-12T16:33:17Z

I have noticed some differences in how they both work:

jsonfield's JSONField saves the value as text in the database while django's native JSONField saves its own as jsonb, which is native to postgres too. The difference is what is causing those inconsistencies.

Here is a snippet from jsonfield's JSONField to buttress my point:

class JSONField(JSONFieldBase, models.TextField):
    """JSONField is a generic textfield that serializes/deserializes JSON objects"""
    form_class = JSONFormField

    def dumps_for_display(self, value):
        kwargs = {"indent": 2}
        kwargs.update(self.dump_kwargs)
        return json.dumps(value, ensure_ascii=False, **kwargs)

patrick-emmanuel · 2018-09-12T17:48:44Z

@Pacu2
I think the data should not be populated as strings anymore, but as dictionaries.
It will make things like this possible:

Dog.objects.create(name='Rufus', data={
     'breed': 'labrador',
     'owner': {
         'name': 'Bob',
         'other_pets': [{
             'name': 'Fishy',
         }],
     },
 })
Dog.objects.create(name='Meg', data={'breed': 'collie', 'owner': None})

Dog.objects.filter(data__breed='collie')
<QuerySet [<Dog: Meg>]>

This part of the codebase will be touched to return a dictionary:
https://github.com/mirumee/saleor/blob/34bc56a89caa3d65fc2dd852a5d1dc4bb7cec101/saleor/dashboard/menu/utils.py#L51-L64

I also don't know how many more lines like there are.
What do you think?

Pacu2 · 2018-09-13T06:32:36Z

@the-bionic I agree, we could rework this to work with dictionaries instead.
Searching for json.dumps would help us localize all the places (as far as I've briefly checked that's the only one)
Apart from that, we could add empty migrations to convert all string-based JSONFields to dictionaries via python scripts.

patrick-emmanuel · 2018-09-13T09:48:05Z

I am worried about something though, for the JSONFields to be changed from text (the one jsonfield provides) to jsonb (the one django provides), the migrations need to be done on a fresh database, because no migration files was generated when we changed the fields to django's native version.

We should take a new approach to it, like the one in this stackoverflow answer, then we migrate the data from string-base to dictionaries.

patrick-emmanuel · 2018-09-14T10:35:45Z

Updated PR. Should work as expected now.

Pacu2 · 2018-09-17T14:52:40Z

saleor/menu/migrations/0008_menu_json_content_new.py

+    def populate_data(apps, schema_editor):
+        Menu = apps.get_model('menu', 'Menu')
+        for menu in Menu.objects.all():
+            if isinstance(menu.json_content, str):


@the-bionic I'm still having an issue with the Menu being a str.
I wonder if we can just change from if to while in here, wouldn't that solve the problem?

Is it possible that we've stored a json_content as a str, inside of text-based JSONField? Wouldn't that make it the double nested string?

@Pacu2 Yes that did it. I did not realize it was a deeply nested string. The logic for resolving the nested string, though, is a little bit more than changing the if condition to a while loop, since we transferring the content of a text-based JSONField to a jsonb-based JSONField, before dropping the text-based one.

PR updated again. Thanks.

Pacu2 · 2018-09-18T14:44:14Z

Works like a charm. Thank you very much for taking care of this issue. The scope was bigger than planned, but you've handled it like a boss.

Congrats on your first contribution to Saleor!

patrick-emmanuel · 2018-09-18T14:56:08Z

I'm humbled. Thanks for being patient with me. :)

patrick-emmanuel force-pushed the fix2802 branch 2 times, most recently from f07c428 to 9d4ecda Compare September 11, 2018 17:07

patrick-emmanuel closed this Sep 11, 2018

patrick-emmanuel reopened this Sep 11, 2018

patrick-emmanuel force-pushed the fix2802 branch from 9d4ecda to e774de4 Compare September 11, 2018 19:44

Pacu2 approved these changes Sep 11, 2018

View reviewed changes

Pacu2 reviewed Sep 11, 2018

View reviewed changes

patrick-emmanuel force-pushed the fix2802 branch from e774de4 to 5a7c819 Compare September 11, 2018 20:46

patrick-emmanuel force-pushed the fix2802 branch 3 times, most recently from 07d1436 to 24d4bdf Compare September 13, 2018 15:33

maarcingebala assigned Pacu2 Sep 17, 2018

Pacu2 reviewed Sep 17, 2018

View reviewed changes

Drop jsonfield package for django's native JSONField

b486975

patrick-emmanuel force-pushed the fix2802 branch from 24d4bdf to b486975 Compare September 18, 2018 10:36

Pacu2 merged commit 4b43932 into saleor:master Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop jsonfield package for django's native JSONField #2824

Drop jsonfield package for django's native JSONField #2824

patrick-emmanuel commented Sep 11, 2018 •

edited by Pacu2

patrys commented Sep 11, 2018 •

edited

codecov bot commented Sep 11, 2018 •

edited

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018 •

edited

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018

Pacu2 Sep 11, 2018 •

edited

patrick-emmanuel commented Sep 11, 2018

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018

Pacu2 commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018 •

edited

Pacu2 commented Sep 12, 2018

Pacu2 commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018 •

edited

patrick-emmanuel commented Sep 12, 2018 •

edited

Pacu2 commented Sep 13, 2018

patrick-emmanuel commented Sep 13, 2018 •

edited

patrick-emmanuel commented Sep 14, 2018

Pacu2 Sep 17, 2018

patrick-emmanuel Sep 18, 2018 •

edited

Pacu2 commented Sep 18, 2018

patrick-emmanuel commented Sep 18, 2018

Drop jsonfield package for django's native JSONField #2824

Drop jsonfield package for django's native JSONField #2824

Conversation

patrick-emmanuel commented Sep 11, 2018 • edited by Pacu2

Screenshots

Pull Request Checklist

patrys commented Sep 11, 2018 • edited

codecov bot commented Sep 11, 2018 • edited

Codecov Report

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018 • edited

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018

Pacu2 Sep 11, 2018 • edited

Choose a reason for hiding this comment

patrick-emmanuel commented Sep 11, 2018

Pacu2 commented Sep 11, 2018

patrick-emmanuel commented Sep 11, 2018

Pacu2 commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018 • edited

Pacu2 commented Sep 12, 2018

Pacu2 commented Sep 12, 2018

patrick-emmanuel commented Sep 12, 2018 • edited

patrick-emmanuel commented Sep 12, 2018 • edited

Pacu2 commented Sep 13, 2018

patrick-emmanuel commented Sep 13, 2018 • edited

patrick-emmanuel commented Sep 14, 2018

Pacu2 Sep 17, 2018

Choose a reason for hiding this comment

patrick-emmanuel Sep 18, 2018 • edited

Choose a reason for hiding this comment

Pacu2 commented Sep 18, 2018

patrick-emmanuel commented Sep 18, 2018

patrick-emmanuel commented Sep 11, 2018 •

edited by Pacu2

patrys commented Sep 11, 2018 •

edited

codecov bot commented Sep 11, 2018 •

edited

patrick-emmanuel commented Sep 11, 2018 •

edited

Pacu2 Sep 11, 2018 •

edited

patrick-emmanuel commented Sep 12, 2018 •

edited

patrick-emmanuel commented Sep 12, 2018 •

edited

patrick-emmanuel commented Sep 12, 2018 •

edited

patrick-emmanuel commented Sep 13, 2018 •

edited

patrick-emmanuel Sep 18, 2018 •

edited