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:
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)
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.
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?
On Fri, Jan 1, 2010 at 3:30 PM, Joseph Kocherhans <jkocherh...@gmail.com> 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:
> 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): > 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.
> You received this message because you are subscribed to the Google Groups "Django developers" group. > To post to this group, send email to django-developers@googlegroups.com. > To unsubscribe from this group, send email to django-developers+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
On Sat, Jan 2, 2010 at 4:34 PM, Sean Bleier <seble...@gmail.com> wrote:
> 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.
> 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
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.
On Sun, Jan 3, 2010 at 7:31 PM, Honza Král <honza.k...@gmail.com> wrote: > Hi everybody,
> On Fri, Jan 1, 2010 at 10:30 PM, Joseph Kocherhans > <jkocherh...@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?
>> 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
> 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
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group. > To post to this group, send email to django-developers@googlegroups.com. > To unsubscribe from this group, send email to django-developers+unsubscribe@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
On Sun, Jan 3, 2010 at 7:34 PM, Alex Gaynor <alex.gay...@gmail.com> wrote:
> 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.
On Sun, Jan 3, 2010 at 7:50 PM, Joseph Kocherhans <jkocherh...@gmail.com> wrote: > On Sun, Jan 3, 2010 at 7:34 PM, Alex Gaynor <alex.gay...@gmail.com> wrote:
>> 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
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group. > To post to this group, send email to django-developers@googlegroups.com. > To unsubscribe from this group, send email to django-developers+unsubscribe@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
-- "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
