Skip to content
27 changes: 17 additions & 10 deletions rest_framework/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"""
from django.core.exceptions import FieldError
from django.db import DataError
from django.db.models import Exists
from django.db.models import Exists, Model
from django.utils.translation import gettext_lazy as _

from rest_framework.exceptions import ValidationError
Expand Down Expand Up @@ -69,7 +69,7 @@ def exclude_current_instance(self, queryset, instance):
If an instance is being updated, then do not include
that instance itself as a uniqueness conflict.
"""
if instance is not None:
if instance is not None and isinstance(instance, Model):
return queryset.exclude(pk=instance.pk)
return queryset

Expand Down Expand Up @@ -149,10 +149,11 @@ def filter_queryset(self, attrs, queryset, serializer):

# If this is an update, then any unprovided field should
# have it's value set based on the existing instance attribute.
if serializer.instance is not None:
instance = serializer.instance if isinstance(serializer.instance, Model) else None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
instance = serializer.instance if isinstance(serializer.instance, Model) else None

if instance is not None:
for source in sources:
if source not in attrs:
attrs[source] = getattr(serializer.instance, source)
attrs[source] = getattr(instance, source)

# Determine the filter keyword arguments and filter the queryset.
filter_kwargs = {
Expand All @@ -166,35 +167,41 @@ def exclude_current_instance(self, attrs, queryset, instance):
If an instance is being updated, then do not include
that instance itself as a uniqueness conflict.
"""
if instance is not None:
if instance is not None and isinstance(instance, Model):
return queryset.exclude(pk=instance.pk)
return queryset

def __call__(self, attrs, serializer):
# When many=True is used, the parent ListSerializer's queryset is
# propagated as the child's instance. Treat that as a create so that
# per-object uniqueness checks don't try to call .pk / field attrs on
# the queryset.
instance = serializer.instance if isinstance(serializer.instance, Model) else None

self.enforce_required_fields(attrs, serializer)
queryset = self.queryset
queryset = self.filter_queryset(attrs, queryset, serializer)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
queryset = self.filter_queryset(attrs, queryset, serializer)
queryset = self.filter_queryset(attrs, queryset, instance)

queryset = self.exclude_current_instance(attrs, queryset, serializer.instance)
queryset = self.exclude_current_instance(attrs, queryset, instance)

checked_names = [
serializer.fields[field_name].source for field_name in self.fields
]
# Ignore validation if any field is None
if serializer.instance is None:
if instance is None:
checked_values = [attrs[field_name] for field_name in checked_names]
else:
# Ignore validation if all field values are unchanged
checked_values = [
attrs[field_name]
for field_name in checked_names
if attrs[field_name] != getattr(serializer.instance, field_name)
if attrs[field_name] != getattr(instance, field_name)
]

condition_sources = (serializer.fields[field_name].source for field_name in self.condition_fields)
condition_kwargs = {
source: attrs[source]
if source in attrs
else getattr(serializer.instance, source)
else getattr(instance, source)
for source in condition_sources
}
if checked_values and None not in checked_values and qs_exists_with_condition(queryset, self.condition, condition_kwargs):
Expand Down Expand Up @@ -273,7 +280,7 @@ def exclude_current_instance(self, attrs, queryset, instance):
If an instance is being updated, then do not include
that instance itself as a uniqueness conflict.
"""
if instance is not None:
if instance is not None and isinstance(instance, Model):
return queryset.exclude(pk=instance.pk)
return queryset

Expand Down
32 changes: 32 additions & 0 deletions tests/test_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,38 @@ def test_unique_constraint_default_message_code(self):
assert serializer.errors == {"non_field_errors": [expected_message]}
assert serializer.errors["non_field_errors"][0].code == UniqueTogetherValidator.code

def test_unique_constraint_with_many_true_does_not_crash(self):
"""
UniqueConstraint validators must not crash with AttributeError when
a queryset is passed as the instance argument together with many=True.

When run_child_validation is not overridden the child serializer has
no per-object instance, so uniqueness checks operate as if all items
are new creates. The important thing is that the code path no longer
raises AttributeError by trying to call .pk on the queryset.

Refs: https://github.com/encode/django-rest-framework/issues/9484
"""
instances = UniqueConstraintModel.objects.all()
# Use global_id values that don't exist yet so uniqueness checks pass.
data = [
{
'race_name': 'new_race',
'position': 10,
'global_id': 100,
'fancy_conditions': 100,
},
{
'race_name': 'other_race',
'position': 20,
'global_id': 200,
'fancy_conditions': 200,
},
]
serializer = UniqueConstraintSerializer(instances, data=data, many=True)
# Before the fix this raised: AttributeError: 'QuerySet' object has no attribute 'pk'
Comment thread
auvipy marked this conversation as resolved.
Outdated
assert serializer.is_valid(), serializer.errors

Comment thread
auvipy marked this conversation as resolved.

# Tests for `UniqueForDateValidator`
# ----------------------------------
Expand Down
Loading