rpkilby / jsonfield

A reusable Django model field for storing ad-hoc JSON data
MIT License
1.16k stars 271 forks source link

version 3.0.0 + Django 2 breaks the field deserialization from db #242

Closed tramora closed 4 years ago

tramora commented 4 years ago

Hi, In my environment (Django 2.2.1), it seems that the last version 3.0.0 of jsonfield (2020/02/15) breaks the field deserialization from db.

The automated tests against a sqlitedb raised the errors

I freezed the version to 2.1.1

rpkilby commented 4 years ago

Hi @tramora. Could you provide more information or an example test case? I tried the following, and had no issues.

instance = JSONNotRequiredModel.objects.create(json={'data': 'data'})
instance = JSONNotRequiredModel.objects.get()
self.assertEqual(instance.json, {'data': 'data'})
tramora commented 4 years ago

you are right @rpkilby my initial issue was not precise enough to work with. Sorry for that.

In my case, a developer trusted too much the jsonfield 2.x and passed sometimes strings instead of a dict.

The following code works with the permissive 2.1.1 version but breaks in 3.0.0

# django model
class Tenant(models.Model):
    tenantid = models.TextField(primary_key=True, editable=False)
    json_path = JSONField(default=[])

...
# test case
class JsonFieldTestCase(TestCase):

    tenantid = '00dc43d92ce34dba85a45401033590b1'

    def setUp(self):
        tenant = Tenant(tenantid=self.tenantid)
        # offending instruction : we should save directly a dict not a str
        tenant.json_path = json.dumps({'id': 'domain_id', 'name': 'domain_name', 'description': '', 'children': [{'id': '1', 'name': 'region1', 'description': '', 'children': [{}]}]})
        tenant.save()

    def test_get_jsonfield(self):
        tenant = Tenant.objects.get(tenantid=self.tenantid)
        assert tenant.json_path.__class__.__name__ == 'dict'

I'll have a look to the whole codebase but do you confirm that the old behaviour was not desired ?

rpkilby commented 4 years ago

I'm not entirely sure, but my guess is that they might have been working around an old bug.

That said, it's unnecessary to dump the data before saving. Just pass the dict directly. Also, you should probably type check the value with assert isinstance(tenant.json_path, dict)