-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
qs_filter
is quietly hiding unique errors resulting in database errors
#7291
Comments
Unfortunately, the steps to reproduce are not clear to me. Not sure which model has the unique constraint. |
To illustrate, here's how a model like that would look (typed here in the comment box, not tested): class Eggs(models.Model):
pass
class Sandwich(models.Model):
eggs = models.ForeignKey(Eggs);
spam = models.IntegerField()
class Meta:
unique_together = ('spam', 'eggs'), The |
Is the fact that your serializers inherit from |
No, that's not related. I was simply writing up an example here on github and used the wrong base class :) |
As a fix I currently have this: def qs_filter(queryset, **filters):
if not hasattr(queryset, 'model'):
# No model available, can't safely expand filters
return filters
meta = queryset.model._meta
FOREIGN_FIELDS = (
related.ManyToManyField,
related.OneToOneField,
related.ForeignKey,
)
for field_name, value in list(filters.items()):
if not isinstance(value, dict):
continue
field = meta.get_field(field_name)
if field and not isinstance(field, FOREIGN_FIELDS):
continue
# Remove the old and broken filter
del filters[field_name]
# We have a related field with a dictionary as filter value, unpack
# the dict to separate values
for sub_field_name, sub_value in value.items():
filters[field_name + '__' + sub_field_name] = sub_value
try:
return queryset.filter(**filters)
except (TypeError, ValueError, django.db.DataError) as e:
logging.exception('Unable to filter query %r using: %r', queryset,
filters)
return queryset.none() But I'm not sure if that would be good for a pull request or not |
To solve the def run_validators(self, value):
"""
Test the given value against all the validators on the field,
and either raise a `ValidationError` or simply return.
"""
errors = []
unique_validators = (
validators.BaseUniqueForValidator,
validators.UniqueValidator,
validators.UniqueTogetherValidator,
)
# Make sure to run the non-unique validators first
for validator in self.validators:
if not isinstance(validator, unique_validators):
self._run_validator(errors, validator, value)
# If there are errors, raise before the unique tests
if errors:
raise ValidationError(errors)
for validator in self.validators:
if isinstance(validator, unique_validators):
self._run_validator(errors, validator, value)
if errors:
raise ValidationError(errors)
def _run_validator(self, errors, validator, value):
if hasattr(validator, 'set_context'):
warnings.warn(
"Method `set_context` on validators is deprecated and will "
"no longer be called starting with 3.13. Instead set "
"`requires_context = True` on the class, and accept the "
"context as an additional argument.",
RemovedInDRF313Warning, stacklevel=2
)
validator.set_context(self)
try:
if getattr(validator, 'requires_context', False):
validator(value, self)
else:
validator(value)
except ValidationError as exc:
# If the validation error contains a mapping of fields to
# errors then simply raise it immediately rather than
# attempting to accumulate a list of errors.
if isinstance(exc.detail, dict):
raise
errors.extend(exc.detail)
except DjangoValidationError as exc:
errors.extend(get_error_detail(exc)) django-rest-framework/rest_framework/fields.py Lines 570 to 602 in 3eef5f4
|
I jut realise that nested serializer can't fulfil unicity constraints because the parent serializer does not have the egg identifier during validation hence the |
That's true, the test is still limited but the patch I proposed for To properly check, the nested serializer would have to be finished saving before the parent can start validation. But that's definitely not ideal either. |
Checklist
master
branch of Django REST framework.Steps to reproduce
unique_together
constraint to two keys with at least one being the foreign keyExpected behavior
Validation error due to failing unique constraint
Actual behavior
The
create
method on the serializer is called, indicating that the unique validation didn't do anything.I've managed to trace the cause down to the
qs_filter
function which is quietly ignoring all exceptions:django-rest-framework/rest_framework/validators.py
Lines 26 to 30 in 3eef5f4
When using nested serializers the
qs_filter
gets passed something likedict(eggs=dict(big=123, small=456))
which results in an error. That makesqs_filter
returnqueryset.none()
. As you can expect, a unique constraint will never occur fromqueryset.none()
If all of these exceptions should be caught, at the very least there should be an acompanying log message so I see 2 bugs:
qs_filter
is silenty hiding exceptionsUniqueTogetherValidator
doesn't work for nested serializersThe text was updated successfully, but these errors were encountered: