search

rpkilby / jsonfield

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

Support polymorphic objects #193

Closed ubaumann closed 4 years ago

ubaumann commented 7 years ago

Fix polymorphic object issue: https://github.com/dmkoch/django-jsonfield/issues/101

With polymorphic objects I get a AttributeError on obj.pk. obj.id works fine. pk and id has in the tests always the same value.

nemesifier commented 7 years ago

@ubaumann could you add a test for your fix and ensure it fails if you revert your patch?

youssefm commented 7 years ago

I can confirm this appears to fix https://github.com/dmkoch/django-jsonfield/issues/101. Without this change, JSON fields on parents don't get deserialized properly and are returned as strings instead.

The problem with this change is that id is only the default primary key if none is specified, and this likely breaks deserialization when a primary key is explicitly specified. (see: https://docs.djangoproject.com/en/1.11/topics/db/models/#automatic-primary-key-fields)

ubaumann commented 7 years ago

@youssefm Thank you. I hadn't considered that.

@nemesisdesign I can add some test but it would add the dependency django-polymorphic.

ubaumann commented 7 years ago

@nemesisdesign Should I add some tests? The fix is working well in our production environment.

nemesifier commented 7 years ago

I'm not a maintainer of this package, but unless django-polymorphic is too heavy, I would add it only as a dependency for the build. This kind of dependency is usually added in a file called requirements-test.txt.

Alternatively it can be added directly here: https://github.com/dmkoch/django-jsonfield/blob/master/.travis.yml#L36

ngaranko commented 6 years ago

Hi All, my project depends on this PR, is there anything I can help with to get it merged?

youssefm commented 6 years ago

Just my two cents here - the current fix may address the specific issue with django-polymorphic, but I don't believe it fixes the more general problem that JSONField has, namely that a JSONField on a parent class gets deserialized as a string when an instance of the child class gets deserialized.

It seems to me that it would make more sense to fix the broader issue here rather than trying to detect instances of a particular library in order to treat them differently.

rpkilby commented 4 years ago

Hi all. I've added tests that ensure that jsonfield is compatible with MTI inheritance. If you have any further issues, please let me know. Thanks!