Handle unset fields with 'many=True' by stephenfin · Pull Request #7574 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation4 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The docs note:
When serializing fields with dotted notation, it may be necessary to
provide adefaultvalue if any object is not present or is empty
during attribute traversal.
However, this doesn't work for fields with many=True. When using these, the default is simply ignored.
The solution is simple: do in ManyRelatedField what we were already doing for Field, namely, catch possible AttributeError and KeyError exceptions and return the default if there is one set.
I still need to add tests. Input here would be appreciated 😄
Refs: #7550
The docs note:
When serializing fields with dotted notation, it may be necessary to
provide a default value if any object is not present or is empty
during attribute traversal.
However, this doesn't work for fields with 'many=True'. When using these, the default is simply ignored.
The solution is simple: do in 'ManyRelatedField' what we were already doing for 'Field', namely, catch possible 'AttributeError' and 'KeyError' exceptions and return the default if there is one set.
Signed-off-by: Stephen Finucane stephen@that.guru Closes: encode#7550
Signed-off-by: Stephen Finucane stephen@that.guru
Bump. Is there anything else I need to do to get eyes on this? Happy to propose backports to stable branches after the fact too.
stephenfin added a commit to getpatchwork/patchwork that referenced this pull request
This wasn't writeable for reasons I haven't been able to figure out. However, it's not actually needed: the 'PatchSerializer' can do the job just fine, given enough information. This exposes a bug in DRF, which has been reported upstream [1]. While we wait for that fix, or some variant of it, to be merged, we must monkey patch the library.
[1] encode/django-rest-framework#7550 [2] encode/django-rest-framework#7574
Signed-off-by: Stephen Finucane stephen@that.guru Reported-by: Ralf Ramsauer ralf.ramsauer@oth-regensburg.de Closes: #379 Cc: Daniel Axtens dja@axtens.net Cc: Rohit Sarkar rohitsarkar5398@gmail.com
stephenfin added a commit to getpatchwork/patchwork that referenced this pull request
This wasn't writeable for reasons I haven't been able to figure out. However, it's not actually needed: the 'PatchSerializer' can do the job just fine, given enough information. This exposes a bug in DRF, which has been reported upstream [1]. While we wait for that fix, or some variant of it, to be merged, we must monkey patch the library.
Conflicts: patchwork/api/patch.py
NOTE(stephenfin): Conflicts are due to the absence of commit d3d4f9f ("Add Django 3.0 support") which we do not want to backport here.
[1] encode/django-rest-framework#7550 [2] encode/django-rest-framework#7574
Signed-off-by: Stephen Finucane stephen@that.guru Reported-by: Ralf Ramsauer ralf.ramsauer@oth-regensburg.de Closes: #379 Cc: Daniel Axtens dja@axtens.net Cc: Rohit Sarkar rohitsarkar5398@gmail.com (cherry picked from commit fe07f30)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Verified to myself that this brings the behaviour into line with what we do in Field, yup.
We'd normally be risk-averse to a moderately impactful change like this, but because it only changes behaviour in the case of exceptions, this seems like a clear improvement/bug fix to me.
So, yes, let's go with it.
Thanks & appreciate your patience.
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request
- Handle unset fields with 'many=True'
The docs note:
When serializing fields with dotted notation, it may be necessary to
provide a default value if any object is not present or is empty
during attribute traversal.
However, this doesn't work for fields with 'many=True'. When using these, the default is simply ignored.
The solution is simple: do in 'ManyRelatedField' what we were already doing for 'Field', namely, catch possible 'AttributeError' and 'KeyError' exceptions and return the default if there is one set.
Signed-off-by: Stephen Finucane stephen@that.guru Closes: encode#7550
- Add test cases for encode#7550
Signed-off-by: Stephen Finucane stephen@that.guru
2 participants
