qs_filter is quietly hiding unique errors resulting in database errors

[wolph]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

1. Create a model with 1 or 2 foreign keys to another model
2. Add a unique_together constraint to two keys with at least one being the foreign key

class Eggs(models.Model):
    big = models.IntegerField(default=0)
    small = models.IntegerField(default=0)


class Sandwich(models.Model):
    eggs = models.ForeignKey(Eggs);
    spam = models.IntegerField()

    class Meta:
        unique_together = ('spam', 'eggs'),

3. Created a serializer for the parent model with a nested serializer for the child model

class EggsSerializer(serializers.ModelSerializer):
    ...

class SandwichSerializer(serializers.ModelSerializer):
    eggs = EggsSerializer(many=False)
    ...

4. Try and add an item that should raise a unique constraint

Expected 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

	def qs_filter(queryset, **kwargs):
	try:
	return queryset.filter(**kwargs)
	except (TypeError, ValueError, DataError):
	return queryset.none()

When using nested serializers the qs_filter gets passed something like dict(eggs=dict(big=123, small=456)) which results in an error. That makes qs_filter return queryset.none(). As you can expect, a unique constraint will never occur from queryset.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:

1. qs_filter is silenty hiding exceptions
2. UniqueTogetherValidator doesn't work for nested serializers

[xordoquy]

Unfortunately, the steps to reproduce are not clear to me. Not sure which model has the unique constraint.
I also don't get how qs_filter hiding exceptions relates to this issue.

[wolph]

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 qs_filter method simply returns queryset.none() when an error is encountered. Now guess how many duplicates you find when you have no items in your queryset :)
I'm not sure in which cases it would break if the try/except was dropped, but at the very least it needs to be more specifice and/or have logging to let the user know if something goes wrong.

[xordoquy]

Is the fact that your serializers inherit from Serializer a typo ? It should be Modelerializer.

[wolph]

No, that's not related. I was simply writing up an example here on github and used the wrong base class :)

[wolph]

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

[wolph]

To solve the qs_filter hiding issue we could make DRF test for the regular validators before the unique validators. A simple modification to rest_framework.fields.Field.run_validators could do the trick:

    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

	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 = []
	for validator in self.validators:
	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))
	if errors:
	raise ValidationError(errors)

[xordoquy]

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 UniqueTogetherConstraint can not be checked.
I doubt DRF can do something about it since there's no guarantee the nested data have an id.

[wolph]

That's true, the test is still limited but the patch I proposed for qs_filter at least takes care of a basic test already and doesn't simply skip the testing.

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.