Decorator for validation - inefficient?

[Bryan]

I want my business objects to be able to do this:
class Person(base):

def __init__(self):
self.name = None

@base.validator
def validate_name(self):
if not self.name: return ['Name cannot be empty']

p = Person()
print p.invalid # Prints ['Name cannot be empty']
p.name = 'foo'
print p.invalid # Prints []
print bool(p.invalid) # Prints False

The invalid attribute and validator decorator would be in the base
class:
class base(object):

@staticmethod # so child can say: @base.validator
def validator(func):
"""Mark the function as a validator."""
func._validator = True
return func

def _get_invalid(self):
"""Collect all validation results from registered validators"""
result = []
for attrName in dir(self):
# Prevent recursive calls
if attrName == 'get_invalid' or attrName == 'invalid':
continue

attr = eval('self.' + attrName) # Get attribute

if str(type(attr)) == "<type 'instancemethod'>": # Check
if is function
if hasattr(attr, '_validator'): # Check if function
is a validator
valerr = attr() # Get result of validation
# Validation result can be a single string, list
of strings, or None.
# If the validation fails, it will be a string or
list of strings
# which describe what the validation errors are.
# If the validation succeeds, None is returned.
if type(valerr) == type([]):
for err in valerr:
result.append(err)
else:
if valerr != None: result.append(valerr)
return result # List of validation error strings
invalid = property(_get_invalid, None, None, "List of validation
errors") # Read-only, so no fset or fdel

I don't really like the _get_invalid() logic that reflects over each
attribute and does ugly string comparisons. Is there a cleaner/more
pythonic way to do this reflection?

Also, I am using a decorator to simply mark a function as being a
validator. This works, but I must enumerate all of the object's
attributes to find all the validators. My original plan was to use a
decorator to "register" a function as being a validator. Then the
_get_invalid() call would only need to enumerate the registered
functions. I ran into problems when I couldn't figure out how to use
a decorator to store a function in an attribute of the function's
class:
# Decorator to register function as a validator
def validator(func):
"""Save func in a list of validator functions on the object that
contains func"""
self._validation_functions.append(func) # ERROR: Cannot access
self this way
return func

I appreciate your feedback. I am relatively new to python but have
become completely enamored by it after looking for alternatives to the
MS languages I use to develop business apps.

[Arnaud Delobelle]

On Oct 31, 6:26*pm, Bryan <[e-mail]:

I want my business objects to be able to do this:
class Person(base):

* * def __init__(self):
* * * * self.name = None

* * @base.validator
* * def validate_name(self):
* * * * if not self.name: return ['Name cannot be empty']

p = Person()
print p.invalid *# Prints ['Name cannot be empty']
p.name = 'foo'
print p.invalid *# Prints []
print bool(p.invalid) *# Prints False

The invalid attribute and validator decorator would be in the base
class:
class base(object):

* * @staticmethod *# so child can say: @base.validator
* * def validator(func):
* * * * """Mark the function as a validator."""
* * * * func._validator = True
* * * * return func

* * def _get_invalid(self):
* * """Collect all validation results from registered validators"""
* * * * result = []
* * * * for attrName in dir(self):
* * * * * * # Prevent recursive calls
* * * * * * if attrName == 'get_invalid' or attrName == 'invalid':
continue

* * * * * * attr = *eval('self.' + attrName) *# Get attribute

* * * * * * if str(type(attr)) == "<type 'instancemethod'>": *# Check
if is function
* * * * * * * * if hasattr(attr, '_validator'): *# Check if function
is a validator
* * * * * * * * * * valerr = attr() *# Get resultof validation
* * * * * * * * * * # Validation result can be a single string, list
of strings, or None.
* * * * * * * * * * # If the validation fails, it will be a string or
list of strings
* * * * * * * * * * # which describe what the validation errors are.
* * * * * * * * * * # If the validation succeeds, None is returned.
* * * * * * * * * * if type(valerr) == type([]):
* * * * * * * * * * * * for err in valerr:
* * * * * * * * * * * * * * result.append(err)
* * * * * * * * * * else:
* * * * * * * * * * * * if valerr != None: result.append(valerr)
* * * * return result *# List of validation error strings
* * invalid = property(_get_invalid, None, None, "List of validation
errors") *# Read-only, so no fset or fdel

I don't really like the _get_invalid() logic that reflects over each
attribute and does ugly string comparisons. *Is there a cleaner/more
pythonic way to do this reflection?

Also, I am using a decorator to simply mark a function as being a
validator. *This works, but I must enumerate all of the object's
attributes to find all the validators. *My original plan was to use a
decorator to "register" a function as being a validator. *Then the
_get_invalid() call would only need to enumerate the registered
functions. *I ran into problems when I couldn't figure out how to use
a decorator to store a function in an attribute of the function's
class:
# Decorator to register function as a validator
def validator(func):
* * """Save func in a list of validator functions on the object that
contains func"""
* * self._validation_functions.append(func) *# ERROR: Cannot access
self this way
* * return func

I appreciate your feedback. *I am relatively new to python but have
become completely enamored by it after looking for alternatives to the
MS languages I use to develop business apps.

Hi

I suggest a simpler approach

class Base(object):
@property
def invalid(self):
invalid = []
for field in self.fields:
validate_field = getattr(self, 'validate_' + field)
if validate_field:
errors = validate_field()
if errors:
invalid.extend(errors)
return invalid

class Person(Base):
fields = 'name', 'age'
def __init__(self):
self.name = None
self.age = None
def validate_name(self):
if not self.name: return ['Name cannot be empty']
def validate_age(self):
if self.age is None: return ['Age cannot be empty']
if not isinstance(self.age, int): return ['Age must be a
number']

Then:

>>p=Person()
p.invalid

['Name cannot be empty', 'Age cannot be empty']

>>p.name='Lorenzo'
p.invalid

['Age cannot be empty']

>>p.age='green'
p.invalid

['Age must be a number']

>>p.age=12
p.invalid

[]

>>>

HTH

--
Arnaud

[Steven D'Aprano]

On Fri, 31 Oct 2008 11:26:19 -0700, Bryan wrote:

I want my business objects to be able to do this:

[snip code]

The code you show is quite long and complex and frankly I don't have the
time to study it in detail at the moment, but I can make a couple of
quick comments. I see by your subject line that you're (probably)
complaining about it being inefficient.

I notice that in the validation code, you do this:

def _get_invalid(self):
"""Collect all validation results from registered validators"""
result = []
for attrName in dir(self):
# Prevent recursive calls
if attrName == 'get_invalid' or attrName == 'invalid':
continue
attr = eval('self.' + attrName) # Get attribute

That's slow and wasteful. The right way to get an attribute is with
getattr:

attr = getattr(self, attname)

I haven't tested this specifically, but in the past my timing tests
suggest that x.attr or getattr() will run about ten times faster than
eval('x.attr').

if str(type(attr)) == "<type 'instancemethod'>":

This is probably also slow and wasteful. Why convert the instance into a
string and then do a string comparison?

At the top level of your module, do this once:

import new

and then in the validation method do this:

if type(attr) is new.instancemethod:

or better still:

if isinstance(attr, new.instancemethod):

valerr = attr() # Get result of validation
# Validation result can be a single string, list
# of strings, or None.
# If the validation fails, it will be a string or
# list of strings
# which describe what the validation errors are.
# If the validation succeeds, None is returned.
if type(valerr) == type([]):
for err in valerr:
result.append(err)
else:
if valerr != None: result.append(valerr)

This whole approach is unpythonic. That doesn't mean it's wrong, but it
does suggest you should rethink it. I recommend that the validation test
should simply raise an exception if the validation fails, and let the
calling code deal with it.
Hope this helps, and I may have another look at your code later today.
--
Steven

[Bryan]

Steven D'Aprano wrote:

On Fri, 31 Oct 2008 11:26:19 -0700, Bryan wrote:

I want my business objects to be able to do this:

[snip code]

The code you show is quite long and complex and frankly I don't have the
time to study it in detail at the moment, but I can make a couple of
quick comments. I see by your subject line that you're (probably)
complaining about it being inefficient.

I notice that in the validation code, you do this:

def _get_invalid(self):
"""Collect all validation results from registered validators"""
result = []
for attrName in dir(self):
# Prevent recursive calls
if attrName == 'get_invalid' or attrName == 'invalid':
continue
attr = eval('self.' + attrName) # Get attribute

That's slow and wasteful. The right way to get an attribute is with
getattr:

attr = getattr(self, attname)

I haven't tested this specifically, but in the past my timing tests
suggest that x.attr or getattr() will run about ten times faster than
eval('x.attr').

if str(type(attr)) == "<type 'instancemethod'>":

This is probably also slow and wasteful. Why convert the instance into a
string and then do a string comparison?

At the top level of your module, do this once:

import new

and then in the validation method do this:

if type(attr) is new.instancemethod:

or better still:

if isinstance(attr, new.instancemethod):

valerr = attr() # Get result of validation
# Validation result can be a single string, list
# of strings, or None.
# If the validation fails, it will be a string or
# list of strings
# which describe what the validation errors are.
# If the validation succeeds, None is returned.
if type(valerr) == type([]):
for err in valerr:
result.append(err)
else:
if valerr != None: result.append(valerr)

This whole approach is unpythonic. That doesn't mean it's wrong, but it
does suggest you should rethink it. I recommend that the validation test
should simply raise an exception if the validation fails, and let the
calling code deal with it.
Hope this helps, and I may have another look at your code later today.
--
Steven

Thanks steven, this is the type of info I was looking for. The
instancemethod string comparison and eval() were making me cringe.

Upon review of my code I decided that I was using decorators to solve
this problem mostly because I just learned about them and I wanted to
use this cool new tool.
Instead of marking functions as being validators and then having to
inefficiently loop over all of an object's attrs to fing them all, I
am going to simply have a get_validators() function on my model
classes that my base class can call to get all the validators. Any
validators for a class would be returned in this function.

The list of validation error descriptions is returned instead of
raising exceptions so clients can show the errors to the user for
fixing. Raising exceptions seems like an uneeded burden for the
client, as there is nothing exceptional about bad user input. Instead,
I want to raise an exception if a client actually tries to save an
invalid entity back to the database. I will have to think on your
suggestion a bit more before I am sure however.

Bryan

[Steven D'Aprano]

On Sat, 01 Nov 2008 17:12:33 -0700, Bryan wrote:

The list of validation error descriptions is returned instead of raising
exceptions so clients can show the errors to the user for fixing.
Raising exceptions seems like an uneeded burden for the client, as there
is nothing exceptional about bad user input.

But of course there is. Exceptional doesn't mean rare. In this case, it
just means it's not the "normal" input which is expected.

Instead, I want to raise an
exception if a client actually tries to save an invalid entity back to
the database. I will have to think on your suggestion a bit more before
I am sure however.

As a general rule, every function should return one "kind" of thing.
Notice I don't say "type", because it doesn't matter what the type/class
of the data is, so long as it is conceptually the same sort of result.

E.g. a function that opens a connection to a database should *only*
return a connection to a data, although the actual type of that
connection may differ depending on the database. It shouldn't return
either a connection or an error code.

I say that this is a general rule, because you can get away with breaking
it, sometimes. E.g. string.find() returns either an offset or an error
signal of -1. But note that this sometimes leads to bugs where people
forget to check for a result of -1, and end up with code doing something
unexpected.
You've suggested a usage:

"The list of validation error descriptions is returned instead of
raising exceptions so clients can show the errors to the user for
fixing."

But you can do that with an exception as well:

while True:
try:
validate(arguments) # returns None on success, or raise Failure
break
except Failure, e:
print e.msg
for error in e.errors:
print "You must fix this error: %s" % error
# when we exit the loop, the arguments are validated.
do_something_with(arguments)
If you're coming from a Java background, you may be concerned that
exceptions are expensive. They aren't. Setting up the try...except block
is very cheap. There's very little overhead to a try block that doesn't
fail.
--
Steven

[Bryan]

On Nov 1, 6:57*pm, Steven D'Aprano <st...@REMOVE-THIS-
cybersource.com.auwrote:

On Sat, 01 Nov 2008 17:12:33 -0700, Bryan wrote:

The list of validation error descriptions is returned instead of raising
exceptions so clients can show the errors to the user for fixing.
Raising exceptions seems like an uneeded burden for the client, as there
is nothing exceptional about bad user input.

But of course there is. Exceptional doesn't mean rare. In this case, it
just means it's not the "normal" input which is expected.

Instead, I want to raise an
exception if a client actually tries to save an invalid entity back to
the database. I will have to think on your suggestion a bit more before
I am sure however.

As a general rule, every function should return one "kind" of thing.
Notice I don't say "type", because it doesn't matter what the type/class
of the data is, so long as it is conceptually the same sort of result.

E.g. a function that opens a connection to a database should *only*
return a connection to a data, although the actual type of that
connection may differ depending on the database. It shouldn't return
either a connection or an error code.

I say that this is a general rule, because you can get away with breaking
it, sometimes. E.g. string.find() returns either an offset or an error
signal of -1. But note that this sometimes leads to bugs where people
forget to check for a result of -1, and end up with code doing something
unexpected.

You've suggested a usage:

"The list of validation error descriptions is returned instead of
raising exceptions so clients can show the errors to the user for
fixing."

But you can do that with an exception as well:

while True:
* * try:
* * * * validate(arguments) *# returns None on success, or raise Failure
* * * * break
* * except Failure, e:
* * * * print e.msg
* * * * for error in e.errors:
* * * * * * print "You must fix this error: %s" % error
# when we exit the loop, the arguments are validated.
do_something_with(arguments)

If you're coming from a Java background, you may be concerned that
exceptions are expensive. They aren't. Setting up the try...except block
is very cheap. There's very little overhead to a try block that doesn't
fail.

--
Steven

I'm coming from a .Net background, and yes, one of the reasons I did
not consider raising exceptions was to avoid the overhead of an
exception handler clause, which in .Net land is expensive.

some more thought on this:

If I were going to be checking for validity during a property setter,
I would probably raise an exception there, because the essence of what
a client was requesting is "set property", and an invalid value
precludes this action from happening.

However, hoping to make client code cleaner and to avoid setter
functions doing expensive db lookup validations, I do not validate
during the setter, but instead defer it until the client explicitly
asks for the validity of the business object. So the essence of the
client's request at that point is "what are the invalid values for the
object", and an exception should only be raised if there was something
stopping this request from being served. Invalid business object
field values do not stop the functionality of the invalid() method.

If I had a validation function that checked the db for a duplicate
primary key, then the invalid() function should raise an exception if
the db could not be contacted. A client should be on the lookout for
that type of exception, but to throw a bunch of exceptions back at a
client who simply requested a list of things that need to be fixed
seems heavy. We would essentially be using Exceptions as an expected
return value of a function. So a doc string would explain: "Returns
None for a valid object, and Exceptions for an invalid object."

Should exceptions be an expected "return value" from a function? Am I
still using my .Net brain?

Bryan

[Arnaud Delobelle]

Bryan <br*******@gmail.comwrites:

However, hoping to make client code cleaner and to avoid setter
functions doing expensive db lookup validations, I do not validate
during the setter, but instead defer it until the client explicitly
asks for the validity of the business object. So the essence of the
client's request at that point is "what are the invalid values for the
object", and an exception should only be raised if there was something
stopping this request from being served. Invalid business object
field values do not stop the functionality of the invalid() method.

This is perfectly fine IMHO, in fact this is similar to how django does
form validation. Each form has a property 'is_valid' and validation is
only triggered when form.is_valid is checked. This doesn't raise
exceptions but makes the errors available to the form user.

--
Arnaud

[Steven D'Aprano]

On Sun, 02 Nov 2008 09:33:41 -0800, Bryan wrote:

I'm coming from a .Net background, and yes, one of the reasons I did not
consider raising exceptions was to avoid the overhead of an exception
handler clause, which in .Net land is expensive.

Actually catching an exception in Python is expensive, so I wouldn't
recommend you use exceptions for message-passing in time-critical code.

As I understand it, in your case you actually need to stop for user-input
if the user's data fails the validation. If that's the case, an extra few
milliseconds to catch an exception isn't going to matter much.

some more thought on this:

If I were going to be checking for validity during a property setter, I
would probably raise an exception there, because the essence of what a
client was requesting is "set property", and an invalid value precludes
this action from happening.

However, hoping to make client code cleaner and to avoid setter
functions doing expensive db lookup validations, I do not validate
during the setter, but instead defer it until the client explicitly asks
for the validity of the business object.

Presumably if they *don't* explicitly ask, you validate anyway at some
point?

So the essence of the client's
request at that point is "what are the invalid values for the object",
and an exception should only be raised if there was something stopping
this request from being served. Invalid business object field values do
not stop the functionality of the invalid() method.

If I had a validation function that checked the db for a duplicate
primary key, then the invalid() function should raise an exception if
the db could not be contacted.

Yes!

A client should be on the lookout for
that type of exception, but to throw a bunch of exceptions back at a
client who simply requested a list of things that need to be fixed seems
heavy.

We would essentially be using Exceptions as an expected return
value of a function. So a doc string would explain: "Returns None for a
valid object, and Exceptions for an invalid object."

Should exceptions be an expected "return value" from a function? Am I
still using my .Net brain?

Because exceptions are first-class objects just like lists, ints and
strings, there is a difference between *returning* an exception and
*raising* an exception.

E.g.:

def build_exception(n):
if n < 0:
raise ValueError('unexpected negative code')
else:
exc = TypeError('error code #%d' % n)
exc.foo = "More info here"
return exc
try:
obj = build_exception(57)
print obj.foo
another_obj = build_exception(-1)
print "We never get here"
except ValueError, e:
print "Failed with error message:", e.message

Making the exception part of your code's API is perfectly legitimate and
I would recommend it. The docstring could say this:

"Return None for a valid object, otherwise raises InvalidDataException
(subclass of ValueError). You can get a list of errors from the
exception's errorlist attribute."
Here's a minimal way to generate the exception:

class InvalidDataException(ValueError):
pass
and then in your validation code:

def validate(obj):
print "Testing many things here"
e = InvalidDataException("failed because of %d errors") % 4
e.errorlist = [
"too many questions",
"too few answers",
"not enough respect",
"and your query's hair is too long (damn hippy)"]
raise e
That's one way. There are others. Have a browse through the standard
library or the Python docs and see how other exceptions are used.
But now that I have a better picture of your use-case, I'm leaning
towards a completely different model. Rather than testing if the business
object is valid, and raising an error if it isn't, you test it for
errors, returning an empty list if there aren't any.
def check_for_errors(obj):
errorlist = []
print "Testing many things here"
if too_many_questions():
errorlist.append("too many questions")
# and any others
return errorlist
I've used strings as errors, but naturally they could be any object that
makes sense for your application.

--
Steven

[Bryan]

Steven D'Aprano wrote:

On Sun, 02 Nov 2008 09:33:41 -0800, Bryan wrote:

I'm coming from a .Net background, and yes, one of the reasons I did not
consider raising exceptions was to avoid the overhead of an exception
handler clause, which in .Net land is expensive.

Actually catching an exception in Python is expensive, so I wouldn't
recommend you use exceptions for message-passing in time-critical code.

As I understand it, in your case you actually need to stop for user-input
if the user's data fails the validation. If that's the case, an extra few
milliseconds to catch an exception isn't going to matter much.

some more thought on this:

If I were going to be checking for validity during a property setter, I
would probably raise an exception there, because the essence of what a
client was requesting is "set property", and an invalid value precludes
this action from happening.

However, hoping to make client code cleaner and to avoid setter
functions doing expensive db lookup validations, I do not validate
during the setter, but instead defer it until the client explicitly asks
for the validity of the business object.

Presumably if they *don't* explicitly ask, you validate anyway at some
point?

So the essence of the client's
request at that point is "what are the invalid values for the object",
and an exception should only be raised if there was something stopping
this request from being served. Invalid business object field values do
not stop the functionality of the invalid() method.

If I had a validation function that checked the db for a duplicate
primary key, then the invalid() function should raise an exception if
the db could not be contacted.

Yes!

A client should be on the lookout for
that type of exception, but to throw a bunch of exceptions back at a
client who simply requested a list of things that need to be fixed seems
heavy.

We would essentially be using Exceptions as an expected return
value of a function. So a doc string would explain: "Returns None for a
valid object, and Exceptions for an invalid object."

Should exceptions be an expected "return value" from a function? Am I
still using my .Net brain?

Because exceptions are first-class objects just like lists, ints and
strings, there is a difference between *returning* an exception and
*raising* an exception.

E.g.:

def build_exception(n):
if n < 0:
raise ValueError('unexpected negative code')
else:
exc = TypeError('error code #%d' % n)
exc.foo = "More info here"
return exc
try:
obj = build_exception(57)
print obj.foo
another_obj = build_exception(-1)
print "We never get here"
except ValueError, e:
print "Failed with error message:", e.message

Making the exception part of your code's API is perfectly legitimate and
I would recommend it. The docstring could say this:

"Return None for a valid object, otherwise raises InvalidDataException
(subclass of ValueError). You can get a list of errors from the
exception's errorlist attribute."
Here's a minimal way to generate the exception:

class InvalidDataException(ValueError):
pass
and then in your validation code:

def validate(obj):
print "Testing many things here"
e = InvalidDataException("failed because of %d errors") % 4
e.errorlist = [
"too many questions",
"too few answers",
"not enough respect",
"and your query's hair is too long (damn hippy)"]
raise e
That's one way. There are others. Have a browse through the standard
library or the Python docs and see how other exceptions are used.
But now that I have a better picture of your use-case, I'm leaning
towards a completely different model. Rather than testing if the business
object is valid, and raising an error if it isn't, you test it for
errors, returning an empty list if there aren't any.
def check_for_errors(obj):
errorlist = []
print "Testing many things here"
if too_many_questions():
errorlist.append("too many questions")
# and any others
return errorlist
I've used strings as errors, but naturally they could be any object that
makes sense for your application.

--
Steven

I like the errorlist attribute on the exception, I think I have a
place for that in my project. Thanks for all the help, now the next
thing I want to investigate is how I can write my validation code once
and make it work both on my objects, and on webforms that edit those
objects. I will always have strict validation in the object model
that will be checked on the server side, but it would be convenient
for the user if they were warned before making a round trip to the
server that the values they entered are probably not correct.

This project is using Pylons, so I'm off to investigate the
possibilities.

Bryan