Skip to content

Fix issue #5258 #5259

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

Merged
merged 2 commits into from
Jul 10, 2017
Merged

Fix issue #5258 #5259

merged 2 commits into from
Jul 10, 2017

Conversation

elmccarthy
Copy link
Contributor

Description

Sanitize serializer.data to remove hidden fields before rendering template
JSON instance to raw data form.

refs #5258
closes #5258

Sanitize serializer.data to remove hidden fields before rendering template
JSON instance to raw data form.
@tomchristie tomchristie added this to the 3.6.4 Release milestone Jul 10, 2017
@tomchristie tomchristie merged commit b905197 into encode:master Jul 10, 2017
@tomchristie
Copy link
Member

Thanks @elmccarthy - seems pretty clear-cut. 😄

@seav
Copy link

seav commented Sep 21, 2017

I think this PR breaks serializers from the djangorestframework-gis add-on. This add-on serializes model objects into a GeoJSON-compatible dict with id, type, geometry, and properties fields. So this PR tries to look for these fields in the serializer class and will most probably result into a KeyError. This is obviously not DRF's fault but I think the removing of hidden fields should happen earlier, when the model object is being serialized, and not after the fact.

@carltongibson
Copy link
Collaborator

@seav First step here is a test showing the kind of call that is failing but shouldn't. From there we can think about adjustments.

@jklaiho
Copy link

jklaiho commented Oct 9, 2017

Seconding the djangorestframework-gis issue, ran into this today. Reported at openwisp/django-rest-framework-gis#145

@rpkilby
Copy link
Member

rpkilby commented Oct 9, 2017

Hi @jklaiho and @seav, I'm going to second @carltongibson here. It would be very helpful to have a minimal test case or a bit of sample code that demonstrates the issue.

@jklaiho
Copy link

jklaiho commented Oct 13, 2017

@carltongibson @rpkilby Here's a minimal Django project I cooked up to demonstrate this:

https://github.com/jklaiho/drf_gis_bug

rpkilby pushed a commit to rpkilby/django-rest-framework that referenced this pull request Oct 13, 2017
@rpkilby
Copy link
Member

rpkilby commented Oct 13, 2017

Thanks @jklaiho, that and #5498 clarified the problem.

To recap, django-rest-framework-gis, does not use HiddenFields. The problem is that the HiddenField filtering in the browsable API assumes that the keys in the serializer.data will correspond to fields on the serializer. This assumption is incorrect and breaks the browsable API in these cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HiddenField appears in Raw Data form initial content
7 participants