Unique validator is executed and breaks if field is invalid

[Glueon]

Suppose there is a simple model:

 class Server(Model):
      ip = GenericIPAddressField(protocol='IPv4', unique=True)

With a basic ModelSeriailzer serializer:

class ServerSerialiser(ModelSerializer):
    class Meta:
        model = Server
        fields = ['ip']

So if I pass an incorrect ip like 123 Django will throw an exception on serializer.is_valid() call.

PostgreSQL complains on a SELECT statement saying that you can't compare inet field with a 123 string.

Here is a traceback:

File "/usr/local/lib/python3.4/site-packages/rest_framework/serializers.py" in is_valid
  197.                 self._validated_data = self.run_validation(self.initial_data)
File "/usr/local/lib/python3.4/site-packages/rest_framework/serializers.py" in run_validation
  541.         value = self.to_internal_value(data)
File "/usr/local/lib/python3.4/site-packages/rest_framework/serializers.py" in to_internal_value
  577.                 validated = self.child.run_validation(item)
File "/usr/local/lib/python3.4/site-packages/rest_framework/serializers.py" in run_validation
  391.         value = self.to_internal_value(data)
File "/usr/local/lib/python3.4/site-packages/rest_framework/serializers.py" in to_internal_value
  421.                 validated_value = field.run_validation(primitive_value)
File "/usr/local/lib/python3.4/site-packages/rest_framework/fields.py" in run_validation
  689.         return super(CharField, self).run_validation(data)
File "/usr/local/lib/python3.4/site-packages/rest_framework/fields.py" in run_validation
  479.         self.run_validators(value)
File "/usr/local/lib/python3.4/site-packages/rest_framework/fields.py" in run_validators
  493.                 validator(value)
File "/usr/local/lib/python3.4/site-packages/rest_framework/validators.py" in __call__
  62.         if queryset.exists():
File "/usr/local/lib/python3.4/site-packages/django/db/models/query.py" in exists
  586.             return self.query.has_results(using=self.db)
File "/usr/local/lib/python3.4/site-packages/django/db/models/sql/query.py" in has_results
  484.         return compiler.has_results()
File "/usr/local/lib/python3.4/site-packages/django/db/models/sql/compiler.py" in has_results
  811.         return bool(self.execute_sql(SINGLE))
File "/usr/local/lib/python3.4/site-packages/django/db/models/sql/compiler.py" in execute_sql
  840.             cursor.execute(sql, params)
File "/usr/local/lib/python3.4/site-packages/debug_toolbar/panels/sql/tracking.py" in execute
  159.         return self._record(self.cursor.execute, sql, params)
File "/usr/local/lib/python3.4/site-packages/debug_toolbar/panels/sql/tracking.py" in _record
  101.             return method(sql, params)
File "/usr/local/lib/python3.4/site-packages/django/db/backends/utils.py" in execute
  79.             return super(CursorDebugWrapper, self).execute(sql, params)
File "/usr/local/lib/python3.4/site-packages/django/db/backends/utils.py" in execute
  64.                 return self.cursor.execute(sql, params)
File "/usr/local/lib/python3.4/site-packages/django/db/utils.py" in __exit__
  97.                 six.reraise(dj_exc_type, dj_exc_value, traceback)
File "/usr/local/lib/python3.4/site-packages/django/utils/six.py" in reraise
  658.             raise value.with_traceback(tb)
File "/usr/local/lib/python3.4/site-packages/django/db/backends/utils.py" in execute
  64.                 return self.cursor.execute(sql, params)

So it dies trying to execute a query. DRF does not stop iterating over validators in run_validators even if one of them failed. I think it should stop as soon as data is bad. Because otherwise next validators can fail badly, like a UniqueValidator.

Looks like Django does the same thing ... This is wrong.

[xordoquy]

Looks like Django does the same thing ... This is wrong.

Do you have a reference to a Django issue for this one ?
We'll probably align on their position.

[Glueon]

Sorry, no. Long ago I tried to submit a bug to Django and that was hard. So I did not even try to do that now.
Can't you add a setting or something which will cause serializer to fail as soon as possible?

[Glueon]

Sorry, I tried Django's ModelForm - it works. It only executes SQL query IF the field is correct. They do not have a UniqueValidator. There is a seperate validate_unique method which is executed after all validators ran in a for loop like you do. For formsets they run unque validation only on valid ones. So I do not know what to suggest here. In order to mimic Django and do not change much I guess your UniqueValidator shoud check if the field it is going to check is valid. And it should be always at the end.

[tomchristie]

Seems valid. Not sure I completely agree with the assessment re. changing how the validators run, but UniqueValidator should probably catch TypeError/ValueError or whatever else is being raised here.

[Glueon]

Silently swallowing DB errors isn't good either. Because you'll get a non-unique-field error when the field in fact doest even exist at all. For example due to some unapplied migrations. Which is very misleading.

[tomchristie]

Silently swallowing DB errors isn't good

All we're doing here is ensuring that .filter(id=123) returns an empty queryset rather than raising an error. That's perfectly valid, and also how we handle object lookups (eg allow us to sensibly return a 404 for /users/123 rather than raising a 500 error).

[Glueon]

Well, maybe. You can run into different sorts of problems. For example .filter(non_exsting_field=123) will raise FieldError. What error fill you output in this case?

And for GenericIPField query filter(ip='blabla') will fail with a DataError. I do not if it's ok to show 404 or field is not unique in such case.

[tomchristie]

I do not if it's ok to show 404 or field is not unique in such case.

We won't. We'll display a validation error. (But the list of validation errors won't include 'this field value is not unique')

We're not masking anything, we've just got a different requirement for how the filter API ought to operate.

[Glueon]

Regarding 404 error I was just responding to this:

eg allow us to sensibly return a 404 for /users/123 rather than raising a 500 error).

How do you want to fix this bug? By catching DataError/FieldError exceptions and adding error messages to the errors array?

[tomchristie]

I think this is going off topic. There's a simple enough bug here - UniqueValidator shouldn't raise exceptions on incorrect types.

[Glueon]

OK, I am just asking if I'll get a Item is not unique when the field is incorrect but unique for example.

[tomchristie]

Nope, you won't. (Once we resolve this)

[Glueon]

OK, waiting for a fix.