Avoid exception when trying to include skipped relationship

[stas]

Fixes

It is a very weird error I hit. We are using the DRF-JSONAPI to build
https://github.com/occrp/id-backend

One of the packages we use is django-simple-activity. Where this package provides a model with a polymorphic relationship which we renamed in our project based on the relationship type:
https://github.com/occrp/id-backend/blob/master/api_v3/serializers/action.py#L55-L62

I get a strange error when I try to include dynamically in the payload the renamed relationships.

Description of the Change

I tracked down the error here:
https://github.com/django-json-api/django-rest-framework-json-api/blob/master/rest_framework_json_api/renderers.py#L356

I was not able to reproduce it as part of the DRF-JSONAPI test suite, unfortunately (the most related changes set is #412). I'll be more than happy to provide some tests if you could direct me or just explain a bit the purpose of the extract_included() arguments... Maybe I'm doing something wrong.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• changelog entry added to CHANGELOG.md
• author name in AUTHORS

Thanks in advance for any help or for reviewing this!!!

[codecov-io]

Codecov Report

Merging #453 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
+ Coverage    95.6%   95.64%   +0.03%     
==========================================
  Files          55       55              
  Lines        2843     2845       +2     
==========================================
+ Hits         2718     2721       +3     
+ Misses        125      124       -1

Impacted Files	Coverage Δ
example/tests/integration/test_polymorphism.py	100% <100%> (ø)	⬆️
rest_framework_json_api/renderers.py	87.33% <100%> (+0.32%)	⬆️

---

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 fe3ed18...b05f10c. Read the comment docs.

[sliverc]

Not quite clear what is happening would also need to do more investigation. I think it would be important to be able to reproduce this in a DJA test as well before merging this change as otherwise the error might occur in the future again after the next refactoring.

In terms of extract_included arguments just out of my head (hope got it right but might be good to add a doc string there I guess):
fields serializer fields of resource
resource the serialized resource (usually type and id)
resource_instance the model instance of the resource
included_resources list of relations which need to be included (this gets reduced as includes are processed)
included_cache list of already extracted relationship

#412 has only been introduced in version 2.5.0. Can you confirm that the issue also exists in version 2.4.0 or not? This way we at least know whether it is a regression or not.

[stas]

@sliverc based on the changelog, it looks like it is a regression:
occrp/id-backend@8cb3c59...master#diff-8af41323990a2c8baf45a65159722ba5L67 (the last two files in the diff).

I will try to replicate the issue as part of a test case.
Thanks for the help so far!

[sliverc]

@stas Have you been able to investigate this issue further? It would be great to see what causes this regression.

[stas]

@SilverC, apologies for the late reply!

I reviewed again the latest version and the issue persists.
Here's a similar use-case to mine.

I have a model with a polymorphic relationship which I need to serialize only if a specific polymorphic type of relationship is present. Using the rest_framework.fields.SkipField() leads to the issue.

Latest commit includes a test case.

To be honest, I'd expect that the serializers.PolymorphicModelSerializer would pick up the right serializer based on the object type so that I don't have to patch it. But this is another issue which is up to you if you think it's relevant to discuss.

[sliverc]

Thanks for looking into this again.

I am bit confused that CI is green whereas the reproducible test should actually fail in my opinion showing what the error is. Or am I missing something here?

In terms of PolymorphicModelSerializer: Using the right serializer is a different question to be discussed in another issue if need to.

[stas]

The fix is included in 8856e0f

[sliverc]

😄 of course I have only looked at the last commit...

In general the change and test looks good. Some small nitpicks. And it would also be good if you could adjust AUTHORS file if desired and add a changelog entry.

[stas]

@sliverc please let me know if there's anything else I can help with to get it merged. Thank you!

[sliverc]

Merged. Thanks for your contribution.