Handle Django's ValidationErrors in ListField by sigvef · Pull Request #6423 · encode/django-rest-framework (original) (raw)
Yeah. I had to do a little bit of spelunking in the code base to refresh my memory 😅, but here is an improved explanation. (incoming wall-of-text, sorry!)
Most Django ValidationErrors are already properly handled by DRF. Django ValidationErrors raised during validation (i.e., inside a DRF Field's run_validators(..)) are already properly handled by DRF without the change in this PR. That is why the tests posted by @smithdc1 pass already without this pull request -- they raise Django ValidationErrors from within validate(..), which is called from within run_validators(..). DRF already knows how to properly handle that.
However, some Django ValidationErrors partially sneak past DRF's handling. DRF/Django can raise Django ValidationErrors from a DRF Field's to_internal_value(..) as well. This is not currently covered by the same exception handling as run_validators(..) is. One case where that can happen is if a serializer has a ListField, that ListField has PrimaryKeyRelatedField as its child, that PrimaryKeyRelatedField does not have a pk_field set explicitly, and the relation itself uses UUIDs as the key.
How does that happen, specifically? Where does the Django ValidationError come from in the aforementioned case? It comes from line 262 (the queryset.get(pk=data)), part of PrimaryKeyRelatedField's to_internal_value(..):
| def to_internal_value(self, data): |
|---|
| if self.pk_field is not None: |
| data = self.pk_field.to_internal_value(data) |
| queryset = self.get_queryset() |
| try: |
| if isinstance(data, bool): |
| raise TypeError |
| return queryset.get(pk=data) |
| except ObjectDoesNotExist: |
| self.fail('does_not_exist', pk_value=data) |
| except (TypeError, ValueError): |
| self.fail('incorrect_type', data_type=type(data).__name__) |
Within the queryset.get(..) call, Django tries to convert data to a UUID via Django's UUIDField.to_python(..). If that doesn't work, say, if the data is not a valid UUID, Django raises a Django ValidationError. This Django Validation ends up bubbling out beyond the PrimaryKeyRelatedField. When tested in isolation, this exception is uncaught, and that is what fails the tests in this PR if they are run without the rest of the changes in this PR.
Okay, what about DRF Serializer's "outer" exception handling? And why hasn't anyone else complained about this? DRF Serializer's exception handling in Serializer.to_internal_value(..) does kick in, which is why in most cases where you try to validate an invalid UUID you wouldn't have any issues. In order to observe any difference, you need to have the specific aforementioned combo of a DRF Serializer with a DRF ListField using a DRF PrimaryKeyRelatedField child with no explicitly declared pk_field, and the primary key relation itself needs to use a UUID pk. (Maybe other combos have similar issues too -- this is the combo that I know of). And even then, the output itself can be considered correct, when viewed in isolation. The only issue is that it is structurally inconsistent with other error messages that could be returned from the same list of uuids being passed in. That makes this issue pretty niche, and probably explains why nobody else has complained about this.
Quick reminder, what are we actually trying to fix? The goal is to fix a slight inconsistency in how certain error messages are structured in the API. The commit message/PR text contains an example of the difference -- although take heed, I've fixed a typo in the output that I discovered while typing up this comment. Mentioning it in case anyone has been following this thread for a while.
How we fix this? The change in this PR is one way of solving this issue. It adds handling of Django ValidationErrors to DRF's ListField. Different options are also available:
- Add handling of Django ValidationErrors to DRF's ListField (implemented is in this PR)
- We could add an extra except clause to the try-except in DRF's
PrimaryKeyRelatedField.to_internal_value(..)(embedded as a code snippet above). After all, Django internally documents thatqueryset.get(..)(indirectly, via code comments of methods called from within.get(..)), that it can throw Django ValidationErrors if it is not happy with the data that is passed in, so it does not seem out of place to check for Django ValidationErrors here. - Explicitly pass DRF's UUIDField as
pk_fieldon DRF's PrimaryKeyRelatedField when automatically constructing them via DRF's ModelSerializer, as DRF's UUIDField has similar error handling that would catch this case. - Ignore it and don't fix it at all, given how DRF has since become a mature project that generally does not accept new changes. This PR does introduce a breaking change that could break someone's project, after all.
I don't really have any particular preference for any which option. For what it's worth, at my day job we've been using a parallel reimplementation of DRF's ListField to fix this and some other small things for some time.