Honza and Joseph (and others), thanks for all your work on this. It seems to me that we are unnecessarily tying complex validators to model fields. I would rather like to see complex validators tied to the model as a Meta option since all the fields are in the validator's domain. This would get rid of the isinstance checks since you'd be pulling in the validators from different places.
However, you'd lose the implied origin of the check. For instance, you'd have to specify both fields within the __init__ of RequiredIfOtherFieldBlank since the validator isn't tied to the field that is checking for the blankness of the other field. This however isn't that bad IMO. Also converting the model instance to a dictionary might be a little less performant, but definitely would look cleaner. 0+ As for the timeline, I think that if complex validators are left as-is then of course it isn't a deal breaker for inclusion to trunk before the major feature freeze on January 5th. However, If complex validators need to be reworked a little, then I think we can exclude them from the merge since you can do model wide validations through the validate method on the model. Then maybe we can add the complex validators before the complete feature freeze? Just some thoughts. Cheers, --Sean On Fri, Jan 1, 2010 at 3:30 PM, Joseph Kocherhans <[email protected]> wrote: > Model validation is just about ready to go. I have one small issue > with it, namely with ComplexValidator, which I'll describe below, but > I think we can resolve it fairly easily. Here's a bit of background. > Sorry if you're already familiar with the branch. > > Validators are functions that are tied to fields. They take the > field's value, and raise a ValidationError if the value doesn't match > the validator's criteria. For example: > > def validate_integer(value): > try: > int(value) > except (ValueError, TypeError), e: > raise ValidationError('') > > Regular validators only have access to their field's own value, but > there's a second type of validator that is *also* tied to a field, but > has access to *all* of the cleaned form data, or the model instance, > depending on the context. When it raises ValidationError, the errors > are tied to that particular field instead of going into > non-field-errors like the form.clean() hook. It's this second type of > validator that I have a problem with, or rather, its implementation. > > Right now, ComplexValidator's __call__ method perfoms the validation. > It takes obj, and all_values as arguments. Only one of them should be > provided. obj should be a model instance, all_values should be a > dictionary of cleaned data from a form. Here's the current > implementation: > > class ComplexValidator(object): > def get_value(self, name, all_values, obj): > assert all_values or obj, "Either all_values or obj must > be supplied" > > if all_values: > return all_values.get(name, None) > if obj: > return getattr(obj, name, None) > > def __call__(self, value, all_values=None, obj=None): > raise NotImplementedError() > > class RequiredIfOtherFieldBlank(ComplexValidator): > def __init__(self, other_field): > self.other_field = other_field > > def __call__(self, value, all_values=None, obj=None): > if all_values is None: > all_values = {} > if self.get_value(self.other_field, all_values, obj) in > EMPTY_VALUES: > if value in EMPTY_VALUES: > raise ValidationError('This field is required if > %s is blank.' % self.other_field) > > I have two reservations about this implementation: > > The ComplexValidator doesn't know in advance if it's going to be used > for model validation or form validation. ComplexValidator's get_value > method is meant to help with this situation, but it needs to take > *both* the dict and obj values to get the value, and as such, it's a > little clunky. > > The other problem I have with ComplexValidator is that your validators > must subclass it since we use isinstance checks to find them. It's > mostly a knee-jerk reaction. I can live with it, but it smells. > > I know there was some discussion at EuroDjangoCon, and people seemed > OK with the current situation, but I'm -0 on leaving ComplexValidator > as-is. Here are a few options: > > 1) Drop ComplexValidator support for 1.2. > > I think we could come up with a more elegant solution given some > usage and time. The branch is still incredibly useful even without > ComplexValidator. > > 2) Convert a model to a dict before passing it to ComplexValidator > so they always just deal with dicts. > > This wouldn't address my discomfort with isinstance checks, but it > would make writing ComplexValidators a lot less clunky. > > 3) Leave ComplexValidator in as-is. > > I don't need a whole lot of convincing that this is the way to go. > If we come up with a better solution later, ComplexValidator itself > would be fairly easy to deprecate. > > 4) You're awesome idea that has escaped me so far. > > This will probably start with option 1, then we'd add the feature > after the branch is merged, or in the 1.3 timeline. > > Does anyone have strong feelings about the way to go here? > > Thank you Honza for all of your work on this code. It's a pretty > tricky problem, but we're almost there. > > Joseph > > Here are a few relevant parts of the code in case anyone wants to dig > in further. > > ComplexValidator implementation: > http://code.djangoproject.com/browser/django/branches/soc2009/model-validation/django/core/validators.py#L139 > > ComplexValidator usage in model validation: > http://code.djangoproject.com/browser/django/branches/soc2009/model-validation/django/db/models/base.py#L810 > > ComplexValidator usage in form validation: > http://code.djangoproject.com/browser/django/branches/soc2009/model-validation/django/forms/forms.py#L293 > > -- > > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. > > > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
