Handle Django's ValidationErrors in ListField

[sigvef]

Without this, Django's ValidationErrors will bypass the error collection
from ListField's children.

Here is an example that illustrates this change.

Consider a Serializer that uses ListField like this:

class SomeSerializer(serializers.Serializer):
    uuids = serializers.ListField(
	child=serializers.PrimaryKeyRelatedField(
	    queryset=Model.objects.something(),
	    validators=[SomeCustomValidator()]
	)
    )

Validating data that looks like this works fine:

{uuids: ['some-valid-uuid', 'some-valid-uuid']}

Raising a DRF ValidationError for one of the children works fine, giving
an error object like:

{'uuids': {0: ErrorDetail(string='Some validation error')}}

Raising a Django ValidationError for one of the children works
differently (which serializers.PrimaryKeyRelatedField can do in some
cases, like when the uuid is malformed). It gives an error object like:

{'uuids': ["'X' is not a valid UUID."]}

Handling Django's ValidationErrors in ListField explicitly (like in this
pull request), will maintain a regular error interface in this case:

{'uuids': {0: ErrorDetail(string="'X' is not a valid UUID.")}}

[carltongibson]

Right, hi, @sigvef.

Can you add test cases for each of the new behaviours you're looking to add here.

Really I'm thinking there's two things going on, so two PRs.

Just catching DjangoValidationErrors here isn't I think something we'd go for.
(This has a long history.)

[sigvef]

Hi! Thanks for taking a look so quickly. I split out the first commit into a separate pull request: #6424

As the the DjangoValidationError change, I'd be happy to read up on some previous discussions. If you have any links laying about I'd be grateful :)

[carltongibson]

Hi @sigvef. Still haven't sat down to think this through yet, but can you remove the str() calls as per #6424. Thanks.

[sigvef]

I've updated this pull request accordingly. I'll add some tests if this change seems reasonable.

[sigvef]

Some additional context on what this does:

We have a Serializer that uses ListField like this:

class SomeSerializer(serializers.Serializer):
    uuids = serializers.ListField(child=serializers.PrimaryKeyRelatedField(queryset=Model.objects.something(), validators=[SomeCustomValidator()]))

Validating data that looks like this works fine:

{uuids: ['some-valid-uuid', 'some-valid-uuid']}

Raising a DRF ValidationError for one of the children works fine, giving an error object like:

{'uuids': {0: ErrorDetail(string='Some validation error')}}

Raising a Django ValidationError for one of the children works differently (which serializers.PrimaryKeyRelatedField can do in some cases, like when the uuid is malformed). It gives an error object like:

["'X' is not a valid UUID."]

Handling Django's ValidationErrors in ListField explicitly (like in this pull request), will maintain a regular error interface in this case:

{'uuids': {0: ErrorDetail(string="'X' is not a valid UUID.")}}

[rpkilby]

Sounds reasonable enough to me? Would need to verify the results with test cases. @carltongibson?

[carltongibson]

Hey @rpkilby. Just haven't had a chance to think this through yet. Very happy for you to advise. 😀

[rpkilby]

Yeah - I would go ahead and write tests for this. You may want to parameterize the tests against both Django and DRF validation errors to ensure both are handled the same.

[kevin-brown]

Still seems reasonable, but I agree with having tests before accepting this.

[smithdc1]

Hi All

Appreciate it's been a while and therefore don't expect anyone to recall this. I tried to write tests for this patch, but I must be miss-understanding this change.

The try block where this PR proposes a change always seems to return a ValidationError as this line here converts DjangoValidationErrors to the required format before re-raising the captured errors as a ValidationError.

As an example, the tests below raise a DjangoValidationError on the child, as suggested above. These tests currently pass on the main branch. The tests are also here.

+class TestListDjangoValidation(FieldValues):
+    valid_inputs = [
+    ]
+    invalid_inputs = [
+        ([1, 2], {0: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+                  1: [ErrorDetail(string='A Django Validation Error', code='invalid')]})]
+
+    outputs = [
+    ]
+
+    def validate(value):
+        raise DjangoValidationError('A Django Validation Error')
+
+    field = serializers.ListField(child=serializers.IntegerField(validators=[validate]))

+class TestNestedListFieldDjangoValidators(FieldValues):
+    valid_inputs = [
+    ]
+    invalid_inputs = [
+        (
+            [[1], [2]],
+            {
+                0: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+                1: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+            },
+        ),
+    ]
+    outputs = [
+    ]
+
+    def validate(value):
+        raise DjangoValidationError(A Django Validation Error')
+
+    field = serializers.ListField(child=serializers.ListField(child=serializers.IntegerField(), validators=[validate]))

[stale]

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.

[sigvef]

If anyone is still interested in this: turns out that the PrimaryKeyRelatedField + UUID pk combination was important to reproduce the issue. I've updated the commit with a test case that fails without the proposed change. An alternative solution to the same testcase could perhaps be to add a corresponding change inside PrimaryKeyRelatedField instead.

[stale]

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.

[auvipy]

I will review myself and check if anything else is needed. in the mean time it would be great if you rebase this again on top of master branch

[sigvef]

Great! Rebased!

[auvipy]

Hi All

Appreciate it's been a while and therefore don't expect anyone to recall this. I tried to write tests for this patch, but I must be miss-understanding this change.

The try block where this PR proposes a change always seems to return a ValidationError as this line here converts DjangoValidationErrors to the required format before re-raising the captured errors as a ValidationError.

As an example, the tests below raise a DjangoValidationError on the child, as suggested above. These tests currently pass on the main branch. The tests are also here.

+class TestListDjangoValidation(FieldValues):
+    valid_inputs = [
+    ]
+    invalid_inputs = [
+        ([1, 2], {0: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+                  1: [ErrorDetail(string='A Django Validation Error', code='invalid')]})]
+
+    outputs = [
+    ]
+
+    def validate(value):
+        raise DjangoValidationError('A Django Validation Error')
+
+    field = serializers.ListField(child=serializers.IntegerField(validators=[validate]))

+class TestNestedListFieldDjangoValidators(FieldValues):
+    valid_inputs = [
+    ]
+    invalid_inputs = [
+        (
+            [[1], [2]],
+            {
+                0: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+                1: [ErrorDetail(string='A Django Validation Error', code='invalid')],
+            },
+        ),
+    ]
+    outputs = [
+    ]
+
+    def validate(value):
+        raise DjangoValidationError(A Django Validation Error')
+
+    field = serializers.ListField(child=serializers.ListField(child=serializers.IntegerField(), validators=[validate]))

can you confirm you addressed this concern?

[sigvef]

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(..):

django-rest-framework/rest_framework/relations.py

Lines 255 to 266 in cc3c89a

	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 that queryset.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_field on 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.

[auvipy]

thanks for your detail break downs. I think while we can accept this PR as minimal effort to fix the issue, the other two suggestion of overhaul also seems valid to me. can you open related PR or some failing tests two other suggestion you have in this Thread?
the following two --
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 that queryset.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_field on DRF's PrimaryKeyRelatedField when automatically constructing them via DRF's ModelSerializer, as DRF's UUIDField has similar error handling that would catch this case.

[auvipy]

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.

Also if possible can you open source that implementation for the community?

[sigvef]

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.

Also if possible can you open source that implementation for the community?

Taking a look at our parallel reimplementation, the parts that would maybe be interesting to the community would be covered by the changes in this PR and in #6424.

[sigvef]

can you open related PR or some failing tests two other suggestion you have in this Thread?

As far as I can tell, the test added in this PR uncovers behaviour that each of the suggested fixes would sufficiently fix by itself.