Skip to content
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

Unique validator is executed and breaks if field is invalid #3381

Closed
Glueon opened this issue Sep 9, 2015 · 13 comments
Closed

Unique validator is executed and breaks if field is invalid #3381

Glueon opened this issue Sep 9, 2015 · 13 comments
Labels
Milestone

Comments

@Glueon
Copy link

@Glueon Glueon commented Sep 9, 2015

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.

@Glueon Glueon changed the title Unique validator is executed if field is invalid Unique validator is executed and breaks if field is invalid Sep 9, 2015
@xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Sep 9, 2015

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
Copy link
Author

@Glueon Glueon commented Sep 9, 2015

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
Copy link
Author

@Glueon Glueon commented Sep 10, 2015

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
Copy link
Member

@tomchristie tomchristie commented Sep 10, 2015

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
Copy link
Author

@Glueon Glueon commented Sep 10, 2015

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
Copy link
Member

@tomchristie tomchristie commented Sep 10, 2015

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
Copy link
Author

@Glueon Glueon commented Sep 10, 2015

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
Copy link
Member

@tomchristie tomchristie commented Sep 10, 2015

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
Copy link
Author

@Glueon Glueon commented Sep 10, 2015

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
Copy link
Member

@tomchristie tomchristie commented Sep 11, 2015

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

@Glueon
Copy link
Author

@Glueon Glueon commented Sep 11, 2015

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

@tomchristie
Copy link
Member

@tomchristie tomchristie commented Sep 11, 2015

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

@Glueon
Copy link
Author

@Glueon Glueon commented Sep 11, 2015

OK, waiting for a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants