[Django] #27263: Don't apply the rest of validators if the first one failed
Django
---------------------------------+-----------------------------
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords: form, validator
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------
I have a couple of validators for files, `FileSizeValidator` and
`FileTypeValidator`.
I attach it to form field like this:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',))
])
}}}
Validators are extended from `object` and implement `__init__` and
`__call__` methods (latter raises `ValidationError`).
The problem is, when I load the bigger file and with different extension,
both validators execute and both error messages are shown for field (about
exceeding max limit and wrong extension).
What I want - if first validator failed, don't even apply the rest of
validators. For this specific scenario is not crucial, but in case of
bigger amount of validators and more complex checks it can be a problem.
For example if we need to read the file and verify that something exists
there, why whe should do that if it's a size limit already exceeded.
Another option can be customizing each validator to not execute in case of
existing errors.
I tried to add additional `field` argument to validator:
{{{#!python
class FileSizeValidator(object):
def __init__(self, max_size, field=None):
self.max_size = max_size
self.field = field
}}}
and pass it in form's `__init __` method:
{{{#!python
class ChartForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super(ChartForm, self).__init__(*args, **kwargs)
for validator in self.fields['import_file'].validators:
validator.form = self.fields['import_file']
}}}
but I don't know how to properly call `self.field.clean()` which requires
`data`. Besides that, this is not good approach, because some other
validators (for example from Django core) can be attached to field.
Also this will require creating some additional mixin that must be
attached along with validators.
I think there is should be much simpler and elegant solution.
Can't find related questions or info in docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Alexey Rogachev):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:1>
Django
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Description changed by Alexey Rogachev:
Old description:
New description:
I have a couple of validators for files, `FileSizeValidator` and
`FileTypeValidator`.
I attach it to form field like this:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',))
])
}}}
Validators are extended from `object` and implement `__init__` and
`__call__` methods (latter raises `ValidationError`).
The problem is, when I load the bigger file and with different extension,
both validators execute and both error messages are shown for field (about
exceeding max limit and wrong extension).
What I want - if first validator failed, don't even apply the rest of
validators. For this specific scenario is not crucial, but in case of
bigger amount of validators and more complex checks it can be a problem.
For example if we need to read the file and verify that something exists
there, why whe should do that if a size limit already exceeded.
Another option can be customizing each validator to not execute in case of
existing errors.
I tried to add additional `field` argument to validator:
{{{#!python
class FileSizeValidator(object):
def __init__(self, max_size, field=None):
self.max_size = max_size
self.field = field
}}}
and pass it in form's `__init __` method:
{{{#!python
class ChartForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super(ChartForm, self).__init__(*args, **kwargs)
for validator in self.fields['import_file'].validators:
validator.form = self.fields['import_file']
}}}
but I don't know how to properly call `self.field.clean()` which requires
`data`. Besides that, this is not good approach, because some other
validators (for example from Django core) can be attached to field.
Also this will require creating some additional mixin that must be
attached along with validators.
I think there is should be much simpler and elegant solution.
Can't find related questions or info in docs.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:2>
Django
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Description changed by Alexey Rogachev:
Old description:
> I have a couple of validators for files, `FileSizeValidator` and
> `FileTypeValidator`.
>
> I attach it to form field like this:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> import_file = forms.FileField(required=False, label='File for
> import', validators = [
> FileSizeValidator(max_size='500 kb'),
> FileTypeValidator(extensions=('xlsx',))
> ])
> }}}
>
> Validators are extended from `object` and implement `__init__` and
> `__call__` methods (latter raises `ValidationError`).
>
> The problem is, when I load the bigger file and with different extension,
> both validators execute and both error messages are shown for field
> (about exceeding max limit and wrong extension).
>
> What I want - if first validator failed, don't even apply the rest of
> validators. For this specific scenario is not crucial, but in case of
> bigger amount of validators and more complex checks it can be a problem.
> For example if we need to read the file and verify that something exists
> there, why whe should do that if a size limit already exceeded.
>
> Another option can be customizing each validator to not execute in case
> of existing errors.
>
> I tried to add additional `field` argument to validator:
>
> {{{#!python
> class FileSizeValidator(object):
> def __init__(self, max_size, field=None):
> self.max_size = max_size
> self.field = field
> }}}
>
> and pass it in form's `__init __` method:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> def __init__(self, *args, **kwargs):
> super(ChartForm, self).__init__(*args, **kwargs)
> for validator in self.fields['import_file'].validators:
> validator.form = self.fields['import_file']
> }}}
>
> but I don't know how to properly call `self.field.clean()` which requires
> `data`. Besides that, this is not good approach, because some other
> validators (for example from Django core) can be attached to field.
>
> Also this will require creating some additional mixin that must be
> attached along with validators.
>
> I think there is should be much simpler and elegant solution.
>
> Can't find related questions or info in docs.
New description:
I have a couple of validators for files, `FileSizeValidator` and
`FileTypeValidator`.
I attach it to form field like this:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',))
])
}}}
Validators are extended from `object` and implement `__init__` and
`__call__` methods (latter raises `ValidationError`).
The problem is, when I load the bigger file and with different extension,
both validators execute and both error messages are shown for field (about
exceeding max limit and wrong extension).
What I want - if first validator failed, don't even apply the rest of
validators. For this specific scenario is not crucial, but in case of
bigger amount of validators and more complex checks it can be a problem.
For example if we need to read the file and verify that something exists
there, why whe should do that if a size limit already exceeded.
Another option can be customizing each validator to not execute in case of
existing errors.
I tried to add additional `field` argument to validator:
{{{#!python
class FileSizeValidator(object):
def __init__(self, max_size, field):
self.max_size = max_size
self.field = field
}}}
and pass it in form's `__init __` method:
{{{#!python
class ChartForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super(ChartForm, self).__init__(*args, **kwargs)
for validator in self.fields['import_file'].validators:
validator.form = self.fields['import_file']
}}}
but I don't know how to properly call `self.field.clean()` which requires
`data`. Besides that, this is not good approach, because some other
validators (for example from Django core) can be attached to field.
Also this will require creating some additional mixin that must be
attached along with validators.
I think there is should be much simpler and elegant solution.
Can't find related questions or info in docs.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:3>
Django
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Description changed by Alexey Rogachev:
Old description:
> I have a couple of validators for files, `FileSizeValidator` and
> `FileTypeValidator`.
>
> I attach it to form field like this:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> import_file = forms.FileField(required=False, label='File for
> import', validators = [
> FileSizeValidator(max_size='500 kb'),
> FileTypeValidator(extensions=('xlsx',))
> ])
> }}}
>
> Validators are extended from `object` and implement `__init__` and
> `__call__` methods (latter raises `ValidationError`).
>
> The problem is, when I load the bigger file and with different extension,
> both validators execute and both error messages are shown for field
> (about exceeding max limit and wrong extension).
>
> What I want - if first validator failed, don't even apply the rest of
> validators. For this specific scenario is not crucial, but in case of
> bigger amount of validators and more complex checks it can be a problem.
> For example if we need to read the file and verify that something exists
> there, why whe should do that if a size limit already exceeded.
>
> Another option can be customizing each validator to not execute in case
> of existing errors.
>
> I tried to add additional `field` argument to validator:
>
> {{{#!python
> class FileSizeValidator(object):
> def __init__(self, max_size, field):
> self.max_size = max_size
> self.field = field
> }}}
>
> and pass it in form's `__init __` method:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> def __init__(self, *args, **kwargs):
> super(ChartForm, self).__init__(*args, **kwargs)
> for validator in self.fields['import_file'].validators:
> validator.form = self.fields['import_file']
> }}}
>
> but I don't know how to properly call `self.field.clean()` which requires
> `data`. Besides that, this is not good approach, because some other
> validators (for example from Django core) can be attached to field.
>
> Also this will require creating some additional mixin that must be
> attached along with validators.
>
> I think there is should be much simpler and elegant solution.
>
> Can't find related questions or info in docs.
New description:
I have a couple of validators for files, `FileSizeValidator` and
`FileTypeValidator`.
I attach it to form field like this:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',))
])
}}}
Validators are extended from `object` and implement `__init__` and
`__call__` methods (latter raises `ValidationError`).
The problem is, when I load the bigger file and with different extension,
both validators execute and both error messages are shown for field (about
exceeding max limit and wrong extension).
What I want - if first validator failed, don't even apply the rest of
validators. For this specific scenario is not crucial, but in case of
bigger amount of validators and more complex checks it can be a problem.
For example if we need to read the file and verify that something exists
there, why whe should do that if a size limit already exceeded.
Another option can be customizing each validator to not execute in case of
existing errors.
I tried to add additional `field` argument to validator:
{{{#!python
class FileSizeValidator(object):
def __init__(self, max_size, field):
self.max_size = max_size
self.field = field
}}}
and pass it in form's `__init __` method:
{{{#!python
class ChartForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super(ChartForm, self).__init__(*args, **kwargs)
for validator in self.fields['import_file'].validators:
validator.form = self.fields['import_file']
}}}
but I don't know how to properly call `self.field.clean()` which requires
`data`. Besides that, this is not good approach, because some other
validators (for example from Django core) can be attached to field.
Also this will require creating some additional mixin that must be
attached along with validators.
I think there should be much simpler and elegant solution.
Can't find related questions or info in docs.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:4>
Django
---------------------------------+-----------------------------------------
Reporter: Alexey Rogachev | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Someday/Maybe
Comment:
I think the idea makes sense but I'm not sure about a suitable API. If you
have a design proposal, please share it on the DevelopersMailingList to
get feedback.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:5>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Alexey Rogachev:
Old description:
> I have a couple of validators for files, `FileSizeValidator` and
> `FileTypeValidator`.
>
> I attach it to form field like this:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> import_file = forms.FileField(required=False, label='File for
> import', validators = [
> FileSizeValidator(max_size='500 kb'),
> FileTypeValidator(extensions=('xlsx',))
> ])
> }}}
>
> Validators are extended from `object` and implement `__init__` and
> `__call__` methods (latter raises `ValidationError`).
>
> The problem is, when I load the bigger file and with different extension,
> both validators execute and both error messages are shown for field
> (about exceeding max limit and wrong extension).
>
> What I want - if first validator failed, don't even apply the rest of
> validators. For this specific scenario is not crucial, but in case of
> bigger amount of validators and more complex checks it can be a problem.
> For example if we need to read the file and verify that something exists
> there, why whe should do that if a size limit already exceeded.
>
> Another option can be customizing each validator to not execute in case
> of existing errors.
>
> I tried to add additional `field` argument to validator:
>
> {{{#!python
> class FileSizeValidator(object):
> def __init__(self, max_size, field):
> self.max_size = max_size
> self.field = field
> }}}
>
> and pass it in form's `__init __` method:
>
> {{{#!python
> class ChartForm(forms.ModelForm):
> def __init__(self, *args, **kwargs):
> super(ChartForm, self).__init__(*args, **kwargs)
> for validator in self.fields['import_file'].validators:
> validator.form = self.fields['import_file']
> }}}
>
> but I don't know how to properly call `self.field.clean()` which requires
> `data`. Besides that, this is not good approach, because some other
> validators (for example from Django core) can be attached to field.
>
> Also this will require creating some additional mixin that must be
> attached along with validators.
>
> I think there should be much simpler and elegant solution.
>
> Can't find related questions or info in docs.
New description:
I have a couple of validators for files, `FileSizeValidator` and
`FileTypeValidator`.
I attach it to form field like this:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',))
])
}}}
Validators are extended from `object` and implement `__init__` and
`__call__` methods (latter raises `ValidationError`).
The problem is, when I load the bigger file and with different extension,
both validators execute and both error messages are shown for field (about
exceeding max limit and wrong extension).
What I want - if first validator failed, don't even apply the rest of
validators. For this specific scenario is not crucial, but in case of
bigger amount of validators and more complex checks it can be a problem.
For example if we need to read the file and verify that something exists
there, why whe should do that if a size limit already exceeded.
Another option can be customizing each validator to not execute in case of
existing errors.
I tried to add additional `field` argument to validator:
{{{#!python
class FileSizeValidator(object):
def __init__(self, max_size, field):
self.max_size = max_size
self.field = field
}}}
and pass it in form's `__init __` method:
{{{#!python
class ChartForm(forms.ModelForm):
def __init__(self, *args, **kwargs):
super(ChartForm, self).__init__(*args, **kwargs)
for validator in self.fields['import_file'].validators:
validator.field = self.fields['import_file']
}}}
but I don't know how to properly call `self.field.clean()` which requires
`data`. Besides that, this is not good approach, because some other
validators (for example from Django core) can be attached to field.
Also this will require creating some additional mixin that must be
attached along with validators.
I think there should be much simpler and elegant solution.
Can't find related questions or info in docs.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:6>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexey Rogachev):
What about adding additional kwarg to `Field` in `django.forms.fields`
called for example `show_circuit_validation`:
{{{#!python
def __init__(self, required=True, widget=None, label=None, initial=None,
help_text='', error_messages=None, show_hidden_initial=False,
validators=(), show_circuit_validation=False, localize=False,
disabled=False, label_suffix=None):
}}}
And apply according changes in `run_validators` method:
{{{#!python
def run_validators(self, value):
if value in self.empty_values:
return
errors = []
for v in self.validators:
try:
v(value)
except ValidationError as e:
if hasattr(e, 'code') and e.code in self.error_messages:
e.message = self.error_messages[e.code]
# Change start
if self.short_circuit_validation:
raise ValidationError(e.error_list)
else:
errors.extend(e.error_list)
# Change end
if errors:
raise ValidationError(errors)
}}}
Not sure about default value for that argument, in my opinion it should be
`True`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:7>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexey Rogachev):
Another option can be customizing each validator to not execute in case of
existing errors.
Usually we need either to apply all validators or just first one, so
probably this is not needed. But it can be implemented by adding
additional argument to validator class and adding check with `getattr` for
example in case it not exist. But validator can be also defined as simple
function, so it brings additional complexity.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:8>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
`short_circuit_validation` seems a bit narrow. It covers the specific use
case where you can write a sequence of validator that should pass exactly
in order, and if one fails, it isn't worth checking the next ones. I'm not
convinced that will be sufficient in general "in case of bigger amount of
validators and more complex checks".
I think writing a validator that wraps other validators and delegates to
the appropriate ones, according to your business logic, is the most
flexible approach. It doesn't require changes in Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:9>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexey Rogachev):
The name of parameter (`show_circuit_validation`) is approximate, can be
discussed and changed. I disagree that it covers specific use case. It's
needed quite often, and in my opinion it's more logical than apply all
validators one by one regardless of previous errors and should be default
strategy. I'm not sure that creating a wrapper validator will be a good
solution because validators in set can vary. Also this will require to
change the way it's defined or subclass fields to override this logic,
because this is defined in field.
As a workaround I can just do a set of calls in form's `clean_import_file`
method like that:
def clean_import_file(self):
import_file = self.cleaned_data.get('import_file')
if import_file:
FileSizeValidator(max_size='500 kb')(import_file)
FileTypeValidator(extensions=('xlsx',))(import_file)
return import_file
But I don't like this approach, even code is DRY, validators is not a part
of configuration anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:10>
Django
---------------------------------+-----------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexey Rogachev):
Based on [https://code.djangoproject.com/ticket/27263#comment:8 idea of
Aymeric Augustin] I've come up with this workaround:
{{{#!python
class MultiValidator(object):
def __init__(self, validators):
self.validators = validators
def __call__(self, value):
for validator in self.validators:
validator(value)
}}}
Then we can group multiple validators using this `MultiValidator`:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators=[MultiValidator([
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',)),
])])
}}}
I don't like the additional nesting and mess with brackets, but besides
that I think it's OK.
We can improve the look by subclassing and creating specific class:
{{{#!python
class ChartFileMultiValidator(MultiValidator):
def __init__(self):
validators = [
FileSizeValidator(max_size='500 kb'),
FileTypeValidator(extensions=('xlsx',)),
]
super(ChartFileMultiValidator, self).__init__(validators)
}}}
Then the code reduces to:
{{{#!python
class ChartForm(forms.ModelForm):
import_file = forms.FileField(required=False, label='File for import',
validators=[ChartFileMultiValidator])
}}}
I personally prefer first approach.
In this case maybe add info about default strategy of applying validators
for form field and possible option for short circuit validation to
documentation?
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:11>
Django
---------------------------------+-----------------------------------------
Component: Forms | Version: master
Severity: Normal | Resolution: wontfix
Keywords: form, validator | Triage Stage: Someday/Maybe
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => wontfix
Comment:
There isn't any consensus on the [https://groups.google.com/d/topic
/django-developers/NJ9yOSQrrPE/discussion django-developers thread] or
here that changes should be made in Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/27263#comment:12>