Finalizing model-validation: ComplexValidator
Joseph Kocherhans
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
Sean Bleier
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
> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>
Joseph Kocherhans
>
> 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?
Yeah, I think excluding ComplexValidators from the merge is the route
I'm going to take. I have a couple more ideas about how they could
work, but I need to focus on getting the branch merged. I'm planning
on doing that Monday night. After it's in, I'll work on those ideas a
little more and bring them up here.
Joseph
Honza Král
On Fri, Jan 1, 2010 at 10:30 PM, Joseph Kocherhans
<jkoch...@gmail.com> wrote:
<snip>
> 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.
We could change this by instantiating the ComplexValidator with obj
and all_values (or make it a factory for validator callables, anything
of that sort), so that custom validation code (currently in __call__)
would just call .get_value('other_field'). It might be tricky too --
we would basically be passing classes to the field's validator param,
instantiating them in form/model.clean() and then calling their
.validate() method (or __call__ing them), but the interface of the
code itself would be much less clunky.
We would just need to provide a clean interface for defining such
validator - haven't really thought about this approach so I don't have
an API proposal ready.
<snip>
> 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.
Sure, we could do this, I am +0 on this. We need real life use cases
just to be sure if people want these validators.
> 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.
I am -1 on this since it wouldn't allow people to call methods and
properties on their models from their validators if they wanted to.
>
> 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.
I would like to invite Jacob and Malcolm into this discussion since the API
def my_validator(value, all_values={}, obj=None):
...
was initially their idea during DjangoCon 2008 and they had some
persuasive arguments there.
I decided not to make all validators have this signature to keep the
option of using a Field without a Form or Model, that's where the need
to separate these two types of validators arose.
> 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.
definitely +1
Alex Gaynor
> Hi everybody,
>
>
> On Fri, Jan 1, 2010 at 10:30 PM, Joseph Kocherhans
> <jkoch...@gmail.com> wrote:
> <snip>
>> 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.
>
> We could change this by instantiating the ComplexValidator with obj
> and all_values (or make it a factory for validator callables, anything
> of that sort), so that custom validation code (currently in __call__)
> would just call .get_value('other_field'). It might be tricky too --
> we would basically be passing classes to the field's validator param,
> instantiating them in form/model.clean() and then calling their
> .validate() method (or __call__ing them), but the interface of the
> code itself would be much less clunky.
>
> We would just need to provide a clean interface for defining such
> validator - haven't really thought about this approach so I don't have
> an API proposal ready.
>
What if we had some sort of wrapper class for objs, it could overide
__getattribute__ to return either an attr if it's an obj, or a
subscript if it's a datadict. it seems to me this would solve both
concerns?
Alex
> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Joseph Kocherhans
>
> What if we had some sort of wrapper class for objs, it could overide
> __getattribute__ to return either an attr if it's an obj, or a
> subscript if it's a datadict. it seems to me this would solve both
> concerns?
I was thinking along similar lines, but with __getitem__ instead,
basically just using a dict interface wrapping a model object, but
that hides the model's methods. Honza makes a good point that those
would be good to have access to. I feel a lot more comfortable
treating a model like a dict than vice-versa, although that's
admittedly rather arbitrary.
Joseph
Alex Gaynor
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>
I'm ambivalent as to which would be prefered, my one concern is people
who probably tend to forget that they could do obj["my_method"](),
since the syntax appears wonky (unless the methods were proxied by
__getattribute__).
Alex
mrts
previously:
He also seemed to have further ideas and code, perhaps he can be
pinged for sharing them.
Best,
MS