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

Support polymorphic objects #193

Closed
wants to merge 2 commits into from
Closed

Conversation

ubaumann
Copy link

@ubaumann ubaumann commented Jun 8, 2017

Fix polymorphic object issue: #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.

Fix polymorphic object issue: rpkilby#101
@nemesifier
Copy link

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

@youssefm
Copy link

youssefm commented Jun 10, 2017

I can confirm this appears to fix #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
Copy link
Author

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

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

@ubaumann
Copy link
Author

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

@nemesifier
Copy link

nemesifier commented Aug 18, 2017

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
Copy link

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

@youssefm
Copy link

youssefm commented Jan 4, 2018

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
Copy link
Owner

rpkilby commented Feb 22, 2020

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!

@rpkilby rpkilby mentioned this pull request Feb 22, 2020
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.

None yet

5 participants