Opened 8 years ago
Closed 8 years ago
#27263 closed New feature (wontfix)
Allow validators to short-circuit in form field validation
Reported by: | Alexey Rogachev | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | form, validator |
Cc: | Triage Stage: | Someday/Maybe | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I have a couple of validators for files, FileSizeValidator
and FileTypeValidator
.
I attach it to form field like this:
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:
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:
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.
Change History (12)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Description: | modified (diff) |
---|
comment:3 by , 8 years ago
Description: | modified (diff) |
---|
comment:4 by , 8 years ago
Description: | modified (diff) |
---|
comment:5 by , 8 years ago
Summary: | Don't apply the rest of validators if the first one failed → Allow validators to short-circuit in form field validation |
---|---|
Triage Stage: | Unreviewed → Someday/Maybe |
comment:6 by , 8 years ago
Description: | modified (diff) |
---|
comment:7 by , 8 years ago
What about adding additional kwarg to Field
in django.forms.fields
called for example short_circuit_validation
:
def __init__(self, required=True, widget=None, label=None, initial=None, help_text='', error_messages=None, short_hidden_initial=False, validators=(), show_circuit_validation=True, localize=False, disabled=False, label_suffix=None):
And apply according changes in run_validators
method:
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
.
comment:8 by , 8 years ago
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 redundant. 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.
comment:9 by , 8 years ago
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.
comment:10 by , 8 years ago
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. In "clean" methods it's possible and easy to achieve by just conditionally and sequentially raising ValidationError
. Why not to have same possibility when using common validators? 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.
comment:11 by , 8 years ago
Based on idea of Aymeric Augustin I've come up with this workaround:
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
:
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:
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:
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?
comment:12 by , 8 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
There isn't any consensus on the django-developers thread or here that changes should be made in Django.
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.