Skip to content

Fix related resource on inherited polymorphic model #767

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

Conversation

4nickel
Copy link
Contributor

@4nickel 4nickel commented Feb 6, 2020

Fixes #621

Description of the Change

When adding a ResourceRelatedField to a serializer that is included in the polymorphic_serializers
list of a polymorphic serializer, the parent_model is not correctly resolved in get_related_resource_type: instead of resolving to the inherited type, the base type is resolved. This can cause an AttributeError if the field in question is not present on the base model.

We ran into this, and then found the closed and seemingly abandoned issue referenced above. This PR implements the fix suggested by the original author, who deserves all the credit. The example code has been expanded to trigger the error when running the test-suite.

Thanks in advance for your time and please let us know if you have any suggestions for improvement.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added test-suite failure is introduced, but no unit-test added
  • documentation updated unnecessary
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

When adding a related resource to an inherited polymorphic model, that
is absent on the base model, the related resource type is incorrectly
resolved in get_related_resource_type.

Introduce a LabResults model to the example code and add it as a related
resource on the ResearchProjectSerializer to provoke an error. This
already causes the test-suite to fail, so no new test has been added.
In get_related_resource_type, check if the parent_serializer is
polymorphic. If so, resolve the parent_model using the parent
serializer's instance to correctly handle related resources on
polymorphic models.
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #767 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #767   +/-   ##
=======================================
  Coverage   97.15%   97.16%           
=======================================
  Files          54       55    +1     
  Lines        2816     2825    +9     
=======================================
+ Hits         2736     2745    +9     
  Misses         80       80           
Impacted Files Coverage Δ
example/migrations/0008_labresults.py 100.00% <0.00%> (ø)

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 c8c5cf1...80121b0. Read the comment docs.

Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up! This looks good to me.

@sliverc sliverc merged commit 9d11e8a into django-json-api:master Feb 7, 2020
@4nickel
Copy link
Contributor Author

4nickel commented Feb 7, 2020

Thanks for the quick merge, your work is much appreciated.

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.

get_related_resource_type failing for polymorphic serializers
2 participants