@error_handler and @validate interactions
Mike
Running into a weird problem with Turbogears on a client project.
Basically I have this situation (I appologize for the pseudo-code, but
I'm having to describe this from memory):
class V1( validators.FancyValidator ):
"""Validator which converts argument to a Subject object
This is a domain-specific object, with lots of complex
internal structure. It represents the "subject" of operations
for multiple controller methods.
"""
my_first_form = form_with_a_V1_validator_as_subject
my_second_form = form_with_a_V1_validator_and_more
class X( Controller ):
@expose( template = 'index' )
def index(self):
"Produce page to select a subject"
@expose( template = 'blah' )
@validate( my_first_form )
@error_handler( index )
def edit( self, subject ):
assert isinstance( subject, Subject ), "Argh"
return {
'form': my_first_form,
}
@expose( )
@validate( my_second_form )
@error_handler( edit )
def update( self, subject, other_args_that_fail ):
assert isinstance( subject, Subject ), "Argh"
With the validator converting the "subject" parameter to a Subject
object. This works fine when there are no errors on the editing form
submission, the validator does a to_python to get a Subject and then
we can work on the Subject inside the method.
However, when there is a validation error in the values passed to the
update method, instead of redirecting to the edit method (and
validating the field that match (subject) to produce a Subject), I get
an error from update where it seems to have allowed the "subject"
argument through as a unicode object.
AFAIK the point of a validator is that an unvalidated value should
never get through to the underlying method. Why bother with the
validator if you have to check for validation within your method
anyway?
The error_handler should AFAICS catch the validation errors on update
and chain onto the edit method (registered error_handler) with the
edit method validation running. That is, if you make a mistake on the
editing page you should see the form you saw before with the errors
highlighted and the values filled in that you tried to submit.
It may be that I'm missing the point, and I welcome any clue-sticks
that show me the "normal" way people do this kind of thing.
Felix Schwarz
Mike wrote:
> However, when there is a validation error in the values passed to the
> update method, instead of redirecting to the edit method (and
> validating the field that match (subject) to produce a Subject), I get
> an error from update where it seems to have allowed the "subject"
> argument through as a unicode object.
I'm not entirely sure if I understood your problem 100% but it looks like
you are annoyed by the same bug/feature/behavior which took me several
hours to track down.
I wrote some documentation back then, maybe you can look at this paragraph:
http://docs.turbogears.org/1.0/ValidateDecorator#beware-of-the-recursion-guard
Honestly, I'm not sure why the recursion guard was put in in the first
place. I only did some quick experiments when I removed the check and
everything seemed to work but removing this in 1.0 will almost certainly
break some applications.
fs
Mike
...
> you are annoyed by the same bug/feature/behavior which took me several
> hours to track down.
call validate twice during the same request.
> I wrote some documentation back then, maybe you can look at this paragraph:http://docs.turbogears.org/1.0/ValidateDecorator#beware-of-the-recurs...
> Honestly, I'm not sure why the recursion guard was put in in the first
> place. I only did some quick experiments when I removed the check and
> everything seemed to work but removing this in 1.0 will almost certainly
> break some applications.
@validate for our application then, just something that does a
straightforward validate whenever the function is called, regardless
of what else has happened... or something.
The real thrust of my question was more "what am I doing that's not
'normal' here"?
It seems to me like this is the *normal* use case for validators, how
can it be broken and yet people use the TurboGears system? I ran into
this on day one, with an extremely simple CRUD-style application, how
could the whole TurboGears community not run into it daily unless they
are doing something fundamentally different?
* Are people really not using validation rigorously enough to run into
it (save for yourself)?
* Are they using a traversal pattern for the "subject lookup" stuff
and thus never using the validators for object lookup/resolution?
I'm reasonably early in this project, so if there is an anti-pattern
I'm coding into the application here I'd like to avoid it rather than
hack around the system's services to support my anti-pattern.
Thanks,
Mike
Felix Schwarz
> It seems to me like this is the *normal* use case for validators, how
> can it be broken and yet people use the TurboGears system? I ran into
> this on day one, with an extremely simple CRUD-style application, how
> could the whole TurboGears community not run into it daily unless they
> are doing something fundamentally different?
Honestly, I don't know.
The code was added in http://trac.turbogears.org/changeset/617 in order to
fix http://trac.turbogears.org/ticket/258. Currently I don't understand why
it was possible to check for recursions, though.
I just disabled the check in the validate decorator which caused two tests
to fail:
test_two_form_in_the_same_page
test_request_related_features.test_repeating_fields
Just in case, anyone likes to dig into that.
fs
Christoph Zwerschke
> The real thrust of my question was more "what am I doing that's not
> 'normal' here"?
>
> It seems to me like this is the *normal* use case for validators, how
> can it be broken and yet people use the TurboGears system? I ran into
> this on day one, with an extremely simple CRUD-style application, how
> could the whole TurboGears community not run into it daily unless they
> are doing something fundamentally different?
>
> * Are people really not using validation rigorously enough to run into
> it (save for yourself)?
I'm using similar patterns in my projects and never ran into such
problems, so there must be something additional involved. Can you reduce
this to a simple test case that we can check out? There was also a
recent fix for #1396, can you check whether this applies to your case
(from looking at your pseudo code it doesn't seem to apply, though).
-- Christoph
Diez B. Roggisch
You validate two times - instead of only in the update-method. Which is
senseless, because update is the action of the form, not edit.
Additionally you need to have a tg_errors=None in update-methods.
Diez
Diez B. Roggisch
> senseless, because update is the action of the form, not edit.
Discard this. I didn't realize you had two different forms in place - sorry
for the noise.
However, tg_errors might still play a role.
Diez
Mike
http://www.vrplumber.com/programming/Validator-Problem.tar.gz
start the dev mode, type any value in the "login" box and click submit
button. On the edit page, enter an invalid password (2 characters for
instance) and click the submit button. The assertion in the code will
fail as the Consultant object is not converted by the validator.
As for the fix, I'm attempting to Monkey-Patch TurboGears with it
(code in the controllers module) and still seeing the original error.
I may have fudged the Monkey-Patch, though.
Thanks,
Mike
Mike
in validate and monkey-patching TurboGears to use the patched
validate.
That is, just deleting the two lines as Felix suggested seems to work
fine, but it still seems that there's something broken in Denmark.
Florent Aide
Mike,
thanks for your time. I've also seen your blog entries on this
subject. We'll make sure to have this fixed.
Florent.
Mike
...
>
> thanks for your time. I've also seen your blog entries on this
> subject. We'll make sure to have this fixed.
>
> Florent.
Have just reached another impasse. The validate decorator overwrites
the first validation pass's values, so you can't see the form errors
on the form page. I managed to work around that:
http://www.vrplumber.com/programming/tgmonkeypatches.py
But then wound up with an as-yet-undiagnosable error where the
validators were getting a str( obj ) version of the converted objects.
At this point the client is just wanting the validators reduced to
doing primitive data-type validation only, as we know that works.
We'll use direct checking/conversion for the subject-object lookups.
Take care,
Mike
Christoph Zwerschke
> I seem to have "solved" the problem by disabling the recursion check
> in validate and monkey-patching TurboGears to use the patched
> validate.
Hi Mike, to answer your first question, I think the behavior that
validation happens only once was not considered as broken, but as the
standard behavior, because this usually makes sense (see below).
Now after I fixed this in r4329 so that only really recursion is
inhibited, I wonder whether we should really fix it that way.
Btw, I noticed your monkey-patch was the one attached to that ticket,
which is different from my final patch, part of changeset 4329 (not yet
released). After r4329, the recursion guard works properly and the
behavior changes.
Let me recap your controller:
@expose( template = "valproblem.templates.editpage" )
@error_handler( index )
@validate( choose_consultant_form )
def edit_consultant( self, consultant ):
...
@expose()
@error_handler( edit_consultant )
@validate( edit_consultant_form )
def update_consultant( self, consultant ):
...
Here, the choose_consultant_form is weaker than the edit_consultant_form
validator - it checks only the id, while the other one checks all
fields, e.g. the length of a password.
We are talking about the situation where we submit the form,
update_consultant is called and there is a validation error, because the
length of the password is wrong, so that the error handler for
update_consultant is called, which is edit_consultant.
Now your problem was that the validator of edit_consultant is not called.
In fact, this was the behavior of TurboGears before r4329: The recursion
guard saw the second validate call on the stack and believed it was a
recursion (because that call_on_stack function was broken and did not
check the name of the entangled function as it was supposed to, but only
checked for the validate decorator). Therefore, the second validate was
not executed.
After r4329, the recursion guard works correctly and notices that there
are two validations, but of different controllers, so it executes the
second validator as well.
BUT...
My question is: Do we really want this new behavior? Please note that it
has the following side effect: The error message about the password
length will not show up any more in the form! This is because the second
validate call does not give any errors, so it masks the errors in the
first one. I don't think this is what we want. And this would also break
backward compatibility. I am sure a lot of applications rely on the old
behavior that validation happens only once (at least some of mine do).
-- Christoph
Christoph Zwerschke
> Have just reached another impasse. The validate decorator overwrites
> the first validation pass's values, so you can't see the form errors
> on the form page.
I just mentioned this problem in my answer to your last posting ;-)
> I managed to work around that:
> http://www.vrplumber.com/programming/tgmonkeypatches.py
Unfortunately, cannot view this, permission denied.
-- Christoph
Christopher Arndt
> After r4329, the recursion guard works correctly and notices that there
> are two validations, but of different controllers, so it executes the
> second validator as well.
>
> BUT...
>
> My question is: Do we really want this new behavior? Please note that it
> has the following side effect: The error message about the password
> length will not show up any more in the form! This is because the second
> validate call does not give any errors, so it masks the errors in the
> first one. I don't think this is what we want. And this would also break
> backward compatibility. I am sure a lot of applications rely on the old
> behavior that validation happens only once (at least some of mine do).
Wouldn't the new behaviour also mean that the validator of the
error_handler method has to be compatible (i.e. does the same checks for
the same fields or a subset) with the validator of the originally called
method? That would be a severe limitation.
Chris
Diez B. Roggisch
> has the following side effect: The error message about the password
> length will not show up any more in the form! This is because the second
> validate call does not give any errors, so it masks the errors in the
> first one. I don't think this is what we want. And this would also break
> backward compatibility. I am sure a lot of applications rely on the old
> behavior that validation happens only once (at least some of mine do).
I'd say no - we don't need that. In similar situations, it's easy enough
to use a load, display and store-method. Load and store essentially only
differ wrt to the form they use (for the validation).
Diez
Christoph Zwerschke
> Wouldn't the new behaviour also mean that the validator of the
> error_handler method has to be compatible (i.e. does the same checks for
> the same fields or a subset) with the validator of the originally called
> method? That would be a severe limitation.
They don't need to check the same things, but then you'll get the
problem already mentioned that a chained validator can mask a former
error. We could merge errors and validator states, but this would need a
lot of changes in the error handling and does not feel right.
So my suggestion is to ge the old behavior back. This can be achieved by
using {} instead of recursion_guard = dict(func=func) in controllers.py.
-- Christoph
Mike
http://www.vrplumber.com/programming/tgmonkeypatches.py_txt
The big change being that it tries to make a reasonable guess as to
how to merge the errors from the second validation into those from the
first.
Mark Ramm
general use case for validation and error_hander is that the
error_handler wil be used to display validation errors back to the
user. But the question is should we provide access to both the
unvalidated items as strings, and the validated items as the correct
python types. I'm in favor of putting the successfully validated
params in cherrypy.request.validated_params (on tg1, and on
tmpl_context.validated_params in tg2) and then just passing the error
handler the dictionary of unvalidated params.
The only change required here would be to save out the validated
params before we move on to call the error_handler.
In TG2 I've thought about changing the error_handler to always use
the same signature:
def my_error_handler(self, unvalidated_params, validated_params, error_messages)
But that seemed like too big of a change, and too big of a divergence
from the way other controller methods work.
Also, there's no reason you can't call the validator directly inside
your controller object, bypassing the whole TG validation decorator
thing all together, and for your use case, that seems reasonable. In
TG2 we just merge the unvalidated params back into the params
dictionary, so everything that can be validated is, and the
error_handler is called with a mixed set of validated and unvalidated
data (so that it can have the same signature as the method it's
handling errors for).
But the more I think about it the more I think we should be keeping
validated and unvalidated data separated.
--Mark Ramm
--
Mark Ramm-Christensen
email: mark at compoundthinking dot com
blog: www.compoundthinking.com/blog
Christoph Zwerschke
> The big change being that it tries to make a reasonable guess as to
> how to merge the errors from the second validation into those from the
> first.
Ok, this is the other idea I mentioned, to merge nested validations
together. But I think we should stay with the old behavior of doing only
one validation per request, and let this be the documented behavior.
Nested validation would get us into really hot water, plus it would
break old applications.
-- Christoph
Christoph Zwerschke
> I would think we should not run the validator the second time. The
> general use case for validation and error_hander is that the
> error_handler wil be used to display validation errors back to the
> user.
Agree.
> But the question is should we provide access to both the unvalidated
> items as strings, and the validated items as the correct python
> types. I'm in favor of putting the successfully validated params in
> cherrypy.request.validated_params (on tg1, and on
> tmpl_context.validated_params in tg2) and then just passing the error
> handler the dictionary of unvalidated params.
The current behavior of TG1 is as follows (and this should also be
better documented):
* If you use a form or a single validator schema for validation, then
you get the validated parameters only if the validation for the whole
form passes completely
* If you use a dict for validation, then you get the validated
parameters even if other parameters could not be validated
I'm not sure whether this behavior should be changed, and I think
getting partial validation results in validated_params would be also
difficult if you have a form, due to the way formencode works.
-- Christoph
Mike
Tell users *not* to use Validators for determining the "Subject" of
the controller. Without that guidance they're going to hit this
problem if they follow best practices (i.e. rigorous use of validators
for data conversion and error handler decoration).
Counsel the use of "traversal based" lookup of Subject-objects. That
is, counsel users to use controllers like this:
/admin/consultants/henry/update?password=hello
rather than:
/admin/consultants/update?login=henry&password=hello
Have a link to the technical discussion of why it's broken/unusable so
people can know *why* not to do it, but make sure they know up front
that it's not going to work.
That counsel should be in your Validator documentation at the minimum,
and possibly in your Error Handling documentation as well.
I've stripped out the use of Validators for subject determination and
am now manually validating each parameter in each controller method in
a series of calls at the start of the controller... maybe I'll write a
decorator method to do that eventually ;) .
Christoph Zwerschke
> Tell users *not* to use Validators for determining the "Subject" of
> the controller. Without that guidance they're going to hit this
> problem if they follow best practices (i.e. rigorous use of validators
> for data conversion and error handler decoration).
Ok, I'm going to update the validate documentation in the wiki if nobody
else wants to do this and if everybody agrees the current implementation
of validators in SVN is ok - i.e. basically the old behavior, only the
first validate per request is executed.
-- Christoph
Mark Ramm
> better documented):
>
> * If you use a form or a single validator schema for validation, then
> you get the validated parameters only if the validation for the whole
> form passes completely
>
> * If you use a dict for validation, then you get the validated
> parameters even if other parameters could not be validated
>
> I'm not sure whether this behavior should be changed, and I think
> getting partial validation results in validated_params would be also
> difficult if you have a form, due to the way formencode works.
That's true. And I don't think we should change 1.1 behavior at all.
The question is what to do in TG2. RIght now it works exactly the
same way as TG1, And your point is good, getting partial information
form from schema validators or other self-validating systems will be
hard, since they don't expose that information.
So, my guess would be that we should either keep the same behavior in
TG2, or always pass unvalidated params to the error_handler, and just
save off the partial validated params dict somewhere for people to
grab if they want it.
What do you think?
Christoph Zwerschke
> So, my guess would be that we should either keep the same behavior in
> TG2, or always pass unvalidated params to the error_handler, and just
> save off the partial validated params dict somewhere for people to
> grab if they want it.
>
> What do you think?
I would keep the same behavior, only document it better.
-- Chris
Felix Schwarz
> So, my guess would be that we should either keep the same behavior in
> TG2, or always pass unvalidated params to the error_handler, and just
> save off the partial validated params dict somewhere for people to
> grab if they want it.
How useful are partially validated parameters? The application must be
prepared to handle the case where no parameter validated successfully
anyway so I don't see a big advantage exposing partially validated parameters.
I'm envisioning some bad effects because this behavior may lead people to
bad coding practices when there is an implicit assumption that some
parameters are always valid (which introduces bugs or even security holes
in their applications).
fs
Mark Ramm
> > TG2, or always pass unvalidated params to the error_handler, and just
> > save off the partial validated params dict somewhere for people to
> > grab if they want it.
>
> How useful are partially validated parameters? The application must be
> prepared to handle the case where no parameter validated successfully
> anyway so I don't see a big advantage exposing partially validated parameters.
I hear you. I'm thinking if we provide a quick function to run a
particular validator against a paricular param inside the controller
method, we may be able to just return the origonal params if there's a
validation error, and expect the user to try revalidating the things
they need.
Alternatively, we can as I said before drop the validated_params off
in the context so people can get at them.
Either way is fine with me (the second has the advantage of only
running the validation once, but the first would work even if you
don't use the @validate decorator, so it might be more reusable).
Felix Schwarz
custom decorator which I named 'strict_validate'. This decorator does
not do any recursion checks but just validates the input parameters
before the method is called. With my new decorator I was able to clean
up quite some code and the some old cruft could be removed in my app.
I do have very similar use cases as Mike: One function (e.g. save) has
strict requirements, the edit function is weaker but still needs to
validate some parameters and there is a listing page which just shows
all 'things' for the current user. Both edit and save take a parameter
which is an id. A custom validator ('TransformingValidator') converts
that key into an Elixir entity (e.g. an order). As I have to show some
information about this order, I need an Elixir entity (not just a
number) on both the edit and the save page.
The user does not need to care about/can not edit the id so there
should never be an error. If there is a problem with the id I can't
help the user and there is nothing the user can do to fix the problems
(besides mailing the webmaster that something is broken). So it seems
to be perfectly sensible to do another validation just before edit (in
case save threw some Invalid exceptions and edit is the error handler
for save) to get at least the order id converted into an entity.
No, I don't want to break backwards compatibility but I think
TurboGears should provide another 'flavor' of the validate decorator
called 'strict_validate' or something like that with does not check if
there was already another validation.
fs
Christoph Zwerschke
> No, I don't want to break backwards compatibility but I think
> TurboGears should provide another 'flavor' of the validate decorator
> called 'strict_validate' or something like that with does not check if
> there was already another validation.
We could add a parameter once=True to the standard validate decorator,
and if you set this to False, then it will validate more than once.
Some details needs to be discussed when alloweing validatino chains.
E.g. Should tg_errors contain the errors of the last validation or all
validation errors merged? The decorator needs to be modified if you want
the latter.
-- Christoph
Christopher Arndt
> No, I don't want to break backwards compatibility but I think
> TurboGears should provide another 'flavor' of the validate decorator
> called 'strict_validate' or something like that with does not check if
> there was already another validation.
Can you write a patch and open a ticket? I think this is a valid issue,
but I think it's easier to discuss an actual implementation than just
ideas and principles.
Chris