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
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
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
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
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,
thanks for your time. I've also seen your blog entries on this
subject. We'll make sure to have this fixed.
Florent.
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
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
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
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
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
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
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
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
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
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?
I would keep the same behavior, only document it better.
-- Chris
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
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).
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
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