Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #613 (closed enhancement: fixed)

Opened 13 years ago

Last modified 12 years ago

Unified validation

Reported by: simon Owned by: anonymous
Priority: normal Milestone: 0.9
Component: TG Widgets Version:
Severity: normal Keywords: validate, exception, formencode
Cc: michele, alberto, simon

Description

It would be nice if form.validate (widget.validate) behaved same as formencode.validate. Namely it should return coerced input and throw an exception for erroneous input not unlike formencode.schema.to_python.

Attachments

form-errors.patch Download (6.5 KB) - added by michele 13 years ago.
first attempt
widgets_unification.patch Download (24.7 KB) - added by alberto 13 years ago.
w_tests.py Download (318 bytes) - added by alberto 13 years ago.
sample test script
widget_unification.patch Download (26.5 KB) - added by alberto 13 years ago.
This patch addresses the "expose" magic, but I don't like it…
widget_unification.2.patch Download (36.5 KB) - added by alberto 13 years ago.
no full qualification, added skip_in_path param equivalent to former skip_parent
widget_unification.3.patch Download (34.7 KB) - added by alberto 13 years ago.
schema-for-validation.patch Download (5.7 KB) - added by michele 13 years ago.
here
schema-for-validation.2.patch Download (6.5 KB) - added by michele 13 years ago.
patch
passing-all-tests.patch Download (10.3 KB) - added by michele 13 years ago.
New version
passing-all-tests-2.patch Download (10.3 KB) - added by michele 13 years ago.
New version, corrects updating of chained_validators and pre_validators
validation5.patch Download (28.4 KB) - added by alberto 13 years ago.
new validation patch
validation6.patch Download (34.2 KB) - added by alberto 13 years ago.
Oops, forgot to svn add a new test, here it is.…
validation7.patch Download (35.4 KB) - added by alberto 13 years ago.
almost there…
validation9.patch Download (43.4 KB) - added by alberto 13 years ago.
with meta.py refactoring
filter_extra_true.patch Download (4.7 KB) - added by alberto 13 years ago.
Filtering extraneous fields from the input
filter_extra_true2.patch Download (5.4 KB) - added by alberto 13 years ago.
No ugly hack at the schema
cleanup-display-field-for-and-required-fields.patch Download (13.6 KB) - added by michele 13 years ago.
cool
newversion.patch Download (14.5 KB) - added by michele 13 years ago.
new version
move-to-forms.patch Download (32.7 KB) - added by michele 13 years ago.
move to forms.py
test-kid.py Download (1.1 KB) - added by michele 13 years ago.
just using the webspace :D

Change History

comment:1 Changed 13 years ago by alberto

I'm not sure I understand you very well. You mean that form's validate should throw an exception when there's invalid input AND build the error_dict with individual field errors, right? So that a global "Your form only contains crap, stupid!" message can appear above invalid forms as well as more concise errors beside individual fields...

Is that it?

comment:2 Changed 13 years ago by simon

I would like to be able to write

try:
  kw = form.validate(kw)
except formencode.Invalid, e:
  errors = e.error_dict

(this is how we handle formencode.Schema in controllers.validate)

comment:3 Changed 13 years ago by alberto

I see... That will not be possiblle as how it's implemented now, mainly beacuse the structure of the error_dict is no like formencode's:

formencode's -> {'age':Invalid1, 'fieldset':Invalid2}
where Invalid2.error_dict  == {'foo':Invalid3}

turbogear's -> {'age':Invalid1, 'fieldset':{'foo':Invalid3}}

This is what _adapt_error_dict at CompundWidget? "adapts". Something like:

def validate(self, value):
    super(Form, self).validate(value)
    errors = getattr(cherrypy.request, 'validation_errors', None)
    if errors:
         raise Invalid(error_dict=errors)

Could be done, but the error dict would be TGs error_dict, not formencode's, and I think it's pretty ugly too as this is something that should go in CompoundWidget? IMO.

However, if this goes into CompoundWidget? it would require *loads* of code rewrite and testing as it changes how errors are propagated through nested widgets completely. And the code that displays errors too, as the error_dict would be formencode-style.

It would have a good side effect though: It would decouple the whole widget API from cherrypy as we won't depend on cherrypy.request.validation_errors to pass the error dict around from widget to childwidget...

The error_dict will be built recursively and could be placed at cherrypy.request.validation_errors with a code snippet like yours in the validate() decorator (which is highly coupled with cherrypy anyway) so it can be passed to the error handling method.

I think this topic requires more discussion, it's would be great to decouple widgets from cherrypy IMO...

Alberto

P.S Michele, where are you? ;)

comment:4 Changed 13 years ago by simon

I think we should unify error format throughout. Right now we return:

1) exception 2) list of exceptions 3) dict a la TG 4) dict a la FE

I propose we reduce this to either just 3) or 4). My preference would be 4), as FE is used outside of TG as well, therefore this format will appeal to more users.

But this is beyond the scope of this ticket. First the protocol (return coerced input, throw errors) should be unified.

comment:5 Changed 13 years ago by michele

Arg, mind air collision.

I like this and I think we can do this easily, what Simon is proposing is not decoupling widget errors managing from cherrypy but just the validation logic.

If I got it right what Simon would like to do is populating cherrypy.request.validation_errors from controller.py (and it makes sense IMHO) even when validataing a form, what we can do is just changing the validate method used by a form to not touch cherrypy.request but instead, if the errors dict is not empty it will just raise an exception like the one raised by formencode that contains our error dict (the one we are actually copying into validation_errors), this shouldn't be that hard but we will need to think a bit about the way of passing an error dict along to form fields (during validation).

comment:6 Changed 13 years ago by simon

Michele, yes. Although I wouldn't mind a single bit to get rid of cherrypy.request.x altogether (I strongly prefer explicit passing of state).

comment:7 Changed 13 years ago by anonymous

I think I understand you and agree with the protocol you're proposing:

try:
  kw = form.validate(kw)
except formencode.Invalid, e:
  errors = e.error_dict

That really boils down to that ALL widgets should raise an Invalid if validation fails , right? CompundWidgets? (being compound, sic) should return the exceptions of their childs in a *flat* nested dict at e.errors_dict, right? This is 4)

Well, I think it is an excellent idea wich will simplfy things *much* because that way errors will propagate in a more natural manner through the widgets tree. This is:

  • Widgets (non-compound) return coerced or raise an Invalid.
  • CompoundWidgets loop through all their childs and collect errors in a flat dict and raise it at Invalid.error_dict, return a coerced value dict if no errors.

If i'm not on crack, applying this recursively to a widget tree should raise Invalid with a nested error_dict with all the individual exceptions at it's leafs. Looks beautiful ;)

What I wanted to point out is that propagating errors this way will make cherrypy.request.validation_errors unnecesary at a widget level. Which is *very good* in my POV: "input_values" and "validation_errors" are cherrypy's ONLY symbols imported at the widgets (well, cherrypy.config.get too, but that can be easy sorted out).

I think the whole widgets package should have no knowledge about cherrypy: input_values should be passed by the validate() decorator and validation_errors should be catched by validate() decorator too. Like your snippet shows.

So I mean, if we shall implement this, let's get two for the price of one :)

Alberto

P.S: I hope i got you right, if not, please explain... :)

Alberto

P.P.S Mind air collision! :) I've skimmed your posts and I think we all more or less think the same way...

comment:8 Changed 13 years ago by alberto

Michele:

I think we should do this, but to the whole widget tree, that is, let recursion's magic flourishand get rid of cherrypy on the way... ;) Doing it only to the form widget looks like a hack to me...

Regards, Alberto

comment:9 Changed 13 years ago by anonymous

What looks saner to me is that the error dict is not "passed" to the child widget's but *catched* from the child widgets.

comment:10 Changed 13 years ago by alberto

correcting myself: what I wanted to say here:

If i'm not on crack, applying this recursively to a widget tree should raise Invalid with a nested error_dict with all the individual exceptions at it's leafs. Looks beautiful ;)

is

If i'm not on *bad* crack ;), applying this recursively to a widget tree should raise Invalid with a flat error dict with Invalid exceptions as leafs (a la FE). Looks beautiful ;)

comment:11 Changed 13 years ago by alberto

anonymous was me, sorry, forgot my password ;)

comment:12 Changed 13 years ago by michele

Remember that we also need input_values when we call display, ATM I can't see any way to avoid it, we need it for the automatic form redisplay logic.

I think we can make this thing happen in gradual steps if it's needed, not all at once and I would also like to hear what Kevin thinks since it's the real widget API designer and he may have a different opinion here.

I think getting ride of cherrypy.request from widgets is an important step but we should keep it balanced because things can get increasingly complex.

For the moment we can try to raise an exception that returns our dict, then we can discuss it further for children catching them and returning a flat dict because that's another big shift from out current API I'm not so sure we should make it keep in mind that 0.9a1 is out but we should be on feature freeze now and just bug fixing IMHO.

The reason I don't think putting it in a Form is going to be a hack is that validate expects a form to validate not another widget type since the only way to post values is by using a form, yes a form is a CompoundWidget? but a special one since it's the only one you can and should use to receive and validate input from the web.

comment:13 Changed 13 years ago by alberto

Hi Michele,

input_values could (and should, IMO) be passed at the controller:

#py
def controller(method):
    values = cherrypy.request.input_values
    return dict(form=form, values=values)

Or could be automagically done by expose.

But I agree with you in that this should be gradual, a start could be to modify Form's update_data like you and I suggested at previous posts. Sorry, I shoudn't have called it "hack" as its a very valid way of gradual transition, really sorry, I just got too excited about decoupling widgets from cherrypy :)

I think t wouldn't be a major shift from the current API either, no extra feautures really, just saner value passing and exception catching which is really an internal implemenation issue and a change to the methods that iteract with widget's validate() which I believe Simon is willing to accept. I really think it's going to make things more simple, not increasingly complex, *really*.

Anyway, neither of us should make the finally decision on this so let's wait for the comments.

Regards, Alberto

comment:14 Changed 13 years ago by michele

Hi Alberto,

I'm almost done with the gradual transition patch, anyway your example will work, but damn that's supposed to work automatically I don't want to tell TG user that if they want to automatically populate the form after errors they need to import cherrypy and pass value in this way, then it's far better a cherrypy dependency on TG widgets than forcing it on user code and explaining to everyone why they will need it. :)

Ah, we also need cherrypy for the errors_for method...

comment:15 Changed 13 years ago by alberto

I know... that's why i mentioned it could be done at expose() ;)

Changed 13 years ago by michele

first attempt

comment:16 Changed 13 years ago by michele

The patch I posted is a first hacky attempt, tests should be fixed (only one fixed), things are getting more complex in some points and easier in others (_set_errors only), dunno if I like it, I really think we should hear Kevin's opinion.

Alberto: sorry didn't notice the expose thing, yes it can be done here but it's another hidden layer of complexity and magic, you find value in your template without returning it from the method and you have to call the form display by passing that to it...

comment:17 Changed 13 years ago by alberto

I think that "pushing" validation errors as a parameter to validate is the wrong approach, it feels more natural to me to catch them in an exception, build an error dict with them, and raisng them in another exception.

Alberto

comment:18 Changed 13 years ago by alberto

I'm going to post a patch tomorrow trying what i was explaining before, that is, raising the errors in an exception and pushing values as parameters at every widget, not just the form. I'll try for the moment to stay with formencode's error_dict style. See how it goes...

Good night people, Alberto

comment:19 Changed 13 years ago by michele

Yep, I also hate this error parameter being passed along.

If you can do it in a formencode way and by using try/except that would rock for sure.

Thanks and Ciao! ;-)

comment:20 Changed 13 years ago by alberto

  • Cc simon added

Hi people,

His what i've got for now. Haven't even tried the nosetests so dont' get nervous ;), they'll probably go wacko...

I'll write them along this afternoon but first need some feedback.

Some things:

  • compunds generate their own schema validator if none is present (this *greatly* simplifies things, no recursive update, no crap, let formencode do the dirty work)
  • to display errors, we need to pass the to display (sounds reasonable to me, expose can prepopulate the template namespace with them after a failed validation).
  • same goes for input values
  • bye, bye cherrypy :)

I'm attaching the patch plus a simple hand-driven test. Some simple examples:

>>> from w_tests import *
>>> form.validate(dict(age="kaboom"))
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "turbogears/widgets/base.py", line 346, in validate
    return self.validator.to_python(value, state)
  File "/Users/alberto/src/turbogearsCOMMIT/thirdparty/formencode/formencode/api.py", line 304, in to_python
    value = tp(value, state)
  File "/Users/alberto/src/turbogearsCOMMIT/thirdparty/formencode/formencode/schema.py", line 175, in _to_python
    error_dict=errors)
formencode.api.Invalid: age: Please enter an integer value

>>> form.validate(dict(age="kaboom", sub=dict(age="bang!"))) 
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "turbogears/widgets/base.py", line 346, in validate
    return self.validator.to_python(value, state)
  File "/Users/alberto/src/turbogearsCOMMIT/thirdparty/formencode/formencode/api.py", line 304, in to_python
    value = tp(value, state)
  File "/Users/alberto/src/turbogearsCOMMIT/thirdparty/formencode/formencode/schema.py", line 175, in _to_python
    error_dict=errors)
formencode.api.Invalid: age: Please enter an integer value
sub: age: Please enter an integer value
>>> 

>>> form.validate(dict(age="21", sub=dict(age="20")))
{'age': 21, 'sub': {'age': 20}}

>>> try:form.validate(dict(age="kaboom", sub=dict(age="bang!")))
... except Exception, e: pass
>>> print form.render(value=e.value, errors=e)

# we still need to play with formencode's validate_partial, etc... to get the prev. input values

comments please?

Alberto

Changed 13 years ago by alberto

Changed 13 years ago by alberto

sample test script

comment:21 Changed 13 years ago by michele

I'm trying the get my head around that, there are "many" things that I'm liking and other not, Alberto is there a way we can have a version of this patch that's not related to what we are discussing about in #626?

The validate work you've done seems really great but I would like to concentrate on it first of all and analyze only that, there are so many changes here that's becoming a bit difficult to follow all of them in the right way for my small mind. :D

Other than that your idea of building a schema on the fly seems just brilliant! The only thing I really don't like is this:

+            if len(decoded) == 1 and isinstance(a.values()[0], dict):
+                # A decoded form, skip the root level
+                cherrypy.request.params = decoded.popitem[1]

this seems a bad hack and something avoidable, what's the problem there? I can't grasp every little thing going on now.

What I would like if I'm not asking too much is a patch that only covers this ticket.

Another thing, as I said removing cherrypy from widgets can be a nice goal but if that makes the user experience more difficult I can't see any advantage, I know we can use expose and automatically (that means magic inside) preparing those variables in the template scope but then we also need to tell our user they have to pass them to display and they are already there for a magic reason.

I really like the work you've done on the validation side because it's really the right way of doing this thing and is simplifying things a lot but I would like to work only on this this before #626.

Great work!

comment:22 Changed 13 years ago by michele

Mmmm, I'm not sure passing errors at display time is the right thing to do is yet another parameter, ideally there should just be value and options maybe, parent is already bothering me (and also you :)), for what is worth I would like to point out that before the new widget API errors were passed also at insert... but we removed them.

My ideal vision is:

1) Doing validation as Simon wanted it to be and as you've done by constructing a schema on the fly, that's really a very nice solution IMHO and removes a lot of complexity

2) controllers.py puts every validation error on cherrypy.request.validation_errors

3) widget still use input_values and validation_errors automatically, as I said removing cherrypy dependency can be a nice point but only and only if this will not mean making things more complex for widgets users, at this point is better a widget dependency on cherrypy than telling every one that value and errors are magically appearing on their templates and that they should pass them at display, moreover cherrypy is just a TG component like formencode.

Again great work, I would really appreciate to look a patch that only touches form/widget error handling if possible.

Thanks! ;-)

comment:23 Changed 13 years ago by alberto

:(((( I've lost my post because a damn air collision :((( IRC?

comment:24 Changed 13 years ago by alberto

Hi,

I'd try to explain again as best as I can...

Sorry for the #626 sneak-in ;) I can do without the *_for but It would really complicate things and not change anything really. Let me explain:

<span py:content="field.display(value_for(field), path=path, options=options_for(field), errors=error_for(field))" />

will become:

<?python
    def error_for(exception, name):
        if exception:
            errors = exception.error_dict
            if errors:
               errors.get(name)
?>
<span py:content="field.display(value.get(field.name), path=path, options=options.get(field.name), errors=error_for(erros, field.name))" />

As you see, value_for and option_for are easy, though not as robust, but error_for is trickier as it has to do the extra logic to handle Invalid exceptions.

Of course, we could do this at update_data, but I *really* see no harm in exposing the function at the template, that way, anyone subclassing form can easy change the template (I know your opinion on this.... ;)

I rather spend the next copuple of hours writing and tweaking tests than changing this, but I will if you *really* want to (after giving it a closer look, please).

Regarding the expose issue, I don't see that exposing input_values and validation_errors automatically to the template is more magical than depending on a global variable (cherrypy.request.X *really* plays the role of a global here) being set code in ANOTHER module. I think its much more explicit to receive it as a parameter where it belongs. It's even more efficient I think because using cherrypy.request.X requires every widget finding their node on the tree from the root, this way widgets receive as parameters their current node in the error/value/options dict and work on it with no knowledge of their position in the tree. it's a more natural recursive algorithm and simplifies the implemenation *greatly* (we even get rid of cherrypy.request for "free" :).

The filter hack: This is needed beacause now every name is fully qualified: form.sub.name. Fully qualifying makes everything simpler: no special cases (like multiple roots) to take care of.

What the filter does is to heuristically determine if NestedVariables?() has done any decoding (which is obvious if we only receive one key AND it is a dict) and in that case, "chop" it off so controllers get the parameters they expect. Rememeber that we need to do this here or else the signatures won't match.

I had other things in mind but I'm really bored writing all this again :P, please ask/suggest anything else you might think of..

Regards, Alberto

Changed 13 years ago by alberto

This patch addresses the "expose" magic, but I don't like it...

comment:25 Changed 13 years ago by alberto

just a quick observation:

alberto-valverdes-imac-g5:~/src/turbogears alberto$ grep '^-' widget_unification.patch | wc
     255    1111   11772
alberto-valverdes-imac-g5:~/src/turbogears alberto$ grep '^+' widget_unification.patch | wc
     185     872    8600

Thats 70 lines less of code! :)

Alberto

comment:26 Changed 13 years ago by michele

Hi Alberto,

Just got home... sorry. :(

Well I will try to take a closer look for sure.

While driving back to home I realized you added the fully qualified thing for widgets name, that's IMHO really unneeded, the first sign of this is that you're trashing it away in the filter, the second and most important reason is that we can only receive one form as input in a method not two or more so fully qualifying a form is pointless.

Regarding passing errors, as I said I'm just against it for no other reason that I just removed this thing with the new API so I don't want it back just for removing the cherrypy dependency, also why passing errors to a widget? if the form that's displaying errors a widget should just validate itself and let the form display errors, look at this:

<span py:replace="field.display(value_for(field), path=path, errors=error_for(field), options=options_for(field))" />
<span py:if="error_for(field)" class="fielderror" py:content="error_for(field)" />

we are passing the error to widget display and then we are displaying them? there is clearly a conflict here, and #125 says this thing should be handled by the form so that you can make a template that displays things at the top if you want to.

When a widget know is path and know that errors and eventually input_values are inside cherrypy.request it can just retrieve them from here by using it's path.

Function inside templates, well I prefer them to be passed from update_data, but the way you have used now is far better then the one you had in mind inside #626, so this is another thing to look at but after this ticket, it doesn't look so bad. ;-)

If you don't mind I will try to hack on the same thing using your approach of generating a schema on the fly that as I said is just brilliant, that's just to be sure I notice every problem we can have and we can discuss with the same knowledge, ATM I can't comment any further because there are many changes together and I will just say blah, blah, blah. :D

Anyway great work again, I do think we should ask Kevin regarding the cherrypy issue, I don't see all this need of removing it from widgets when it can make the widget user life easier.

Ciao!!!

comment:27 Changed 13 years ago by alberto

Hi Michele,

I'm glad you see that value_for, etc is useful :)

If you take a close look, you'll notice errors are passed to widget's at display not for them to display (the form does) but so subwidgets can have their new root (get_errors goes one level "down" in the tree) and display their children errors (like fieldset). This really simplifies things, as you don't need to walk the whole tree for every widget... and you get rid of cherrypy... and you leverage recursivities goodness ;). Same goes for value_for and and options_for.

I think this cherrypy independence is a very good thing, because it really doesn't need to know about it (and makes automated and command line testing easier).

Reagarding the filter, if you can hack it and have it like we had it before your absoulutely welcomed to do it, shouldn't be to difficult as you just need to slice the path (I've renamed parent to path as it's realyy a path).

I've noticed to things we have to study better, one being on the definitive style of the error_dict. Right now it needs some change at the validate decorator as the "validators" validation (not the form) is using the old-style, Simon, where are you ;)

I've almost ported test_widgets to the new style and will look into the full_path issue, more to come soon...

c'ya, Alberto

Changed 13 years ago by alberto

no full qualification, added skip_in_path param equivalent to former skip_parent

comment:28 Changed 13 years ago by michele

Hi Alberto,

I haven't yet said they are useful, just that they are not so bad as the one you proposed on #626, but this is another issue we will talk about later. :P

Some other things:

  • CompoundWidget? is starting to look like a validator more than a widget I don't like the chained and before stuff, can't we just extend a given schema containing already those things?
  • Your solution to the expose magic (that I also don't like) just adds back the cherrypy dependency as before but in a different and less useful way IMHO, but I still have to look more deeply at it anyway.
  • Full qualification is not needed, and your previous hack was also dangerous if someone was just receiving in input a dict. :D
  • The errors dict format (the one that will end-up in validation_errors), I think that we must decide between the nested one and the formencode one, if we go for the latter widgets should convert it when looking at validation_errors using your function
  • I like renaming parent to be path. ;)
  • Yep, I know fields are not displaying their errors but it's still a controversy passing errors to them and also displaying them right after that.
  • Bad guy you remove field_for, and error_for, they are useful for giving back to templates their View role in the MVC paradigm if someone wants this, widget are somehow breaking the MVC but this is an acceptable thing, but again that's #626 and not the point here. :P

Ciao! :D

Now I will look into this more deeply while hacking a bit...

comment:29 Changed 13 years ago by michele

Ah, if you get a mind air collision just hit the back button and FF will bring back your form with everything you typed! or maybe you're using Mosaic? :P

Ciao!

comment:30 Changed 13 years ago by alberto

Ok, I'm going to post tomorrow with more tests and more polishing...

I'm not so sure about extending a given schema... See, right now it's: 1) a given schema, 2) an automatic schema

I thing a combination of both will be trickier than what it's worth (what if both a subwidget and a schema want to validate the same field?). We could, however, override all of a subwidget's validator's with the schema, but that will get wetter (I mean, unDRYer ;)

I think it's getting to look like a validator beacuse of the "unification" part of all this (what we're having here, really is a "widgetvalidator" which delegates the validation part to formencode). That could be sorted out later...

What I think we need to get straight as soon as possible is:

1) FE error_dicts or TG error_dicts? (My vote: FE, as it's well tested and all that "don't reinveint the wheel" reasons, we still have unpack_errors if the need arises)

2) Cherrypy.request or no Cherrypy.request? (My vote: NO, please no... just complicates an otherwise quite simple recursive algorithm AND increases coupling for the gain of reducing 2 params at display (value & error).

I think I'ts not that bad for users to do ${form.display(input_values, errors=validation_errors)} It's even more explicit and allows users to override if they want to and don't have prefilled forms (who knows.. some people are really twisted ;) we already have tg as an "automatic" template variable so I don't buy the extra "magic" argument... ;)

Hey! I didn't remove field_for! (just moved it to don't-remember-where... probably WidgetWithFields? as it can also apply to fieldsets). the former error_for method had to go because there's no way to get them without cherrypy request and all associated hassle, see, error is now passed to display as dynamic data. you can still call it with an error_dict node as first parameter or use the WidgetWithFields?' "error_for template method"

By the way, I've fixed the full-quallification issue in a similar manner as you did, the filter is clean again... :) Though I don't understand how you say I can you expect a dict from the request params without being processed by NestedVariables?... please explain, I might be missing something.

Tomorrow I really need to get some work done at the office to keep my bosses happy so I'll probably wont be able to get anything of this until the evening... If we decide on this two details of the protocol I can probably get some polishing/changes and the tests up and passing again.

Regards, Alberto

comment:31 Changed 13 years ago by alberto

I'm attaching a patch that removes pre_validators and chained_validators from CompoundWidget? (you were right, looked too much like a validator ;). I've also renamed build_schema to build_or_extend_schema (guess what it does ;) and calling it at WidgetWithFields?' init so it's not a property anymore and I can remove the setattr workaround at Compound.

Fixed the tests at test_widgets, they're all passing now! (had to remove some things, like passing "Krakatoa" as a value to a form, it should be a dict and an exception raised as its clearly bad usage).

Renamed skip_in_path to include_in_path and inverted the conditions (this is totally subjective...)

More tomorrow, though I'd like some consensus so I can start writing tests...

Good night, Alberto

Changed 13 years ago by alberto

comment:32 Changed 13 years ago by michele

Hi Alberto,

sorry if I reply only now but I was offline and I tried working on this, the first thing I noticed is that building recursively a schema validator is quite tricky, are you taking into account eventual schema available on subwidgets? and their precedence? yes I think so by looking at your patch, but the main problem is that while extending a schema we are becoming not thread safe because we end-up to modify some attributes that belongs to the Schema that's also a widget attribute, we don't get the warning since that's an indirect thing but it's still a problem.

At this point I think that as I said we should try to avoid mixing #626 and this ticket together, what we should try to do is searching the right migration path, this means that every test should pass "without any modification" before doing the next step (that means raising an exception when validate is called) otherwise we are introducing some regression and this trashes all the work we have done until now in the others tickets.

Making validator a property is not good either (and you also changed it) because this means that at every display call we hit the recursive Schema creation.

I'm attaching a patch that shows what I've done so far, is just a modification to use a prepared Schema and it's totally backward compatible with what we have now (errors are still in cherrypy.request.validation_errors) that's my migration path because I'm checking tests against this, but they are not all passing yet and there is still the thread-safe problem I mentioned, but now I'm tired to hack any further and I'm going to sleep. :D

I know you want to work on the big patch, but we should be very careful with this thing and that's why I think we should proceed with small steps and not by covering even another ticket. ;)

By the way regarding the change of passing input_values at display time and same for errors, I think that's not doable because we are also in API freeze now (ideally), 0.9 is not out, it's out just a preview of it.

Changed 13 years ago by michele

here

comment:33 Changed 13 years ago by alberto

I was going to sleep so I haven't taken a very close look to your patch (will do tomorrow).

I'd like to reply quickly now:

You're absloutely right on the schema unthreadsafeness i was committing by extending a current schema, I've thought it just after posting the patch but haven't looked yet into alternatives...

Regarding handling subwidget's schemas... sure! they're generated at every compound's init if there is a need (that is, we have subvalidators)

I agree with you on looking for the best migration path and don't change the current interface at all (except with the validate() decorator, which is the whole point of this ticket). However, I think it can be done while keeping cherrypy.request out of the widgets. The big problem is with display() at the users templates, but im really looking to a solution to this.

I'll try to fix the tests except what is absolutely necesary as to make sure no regression get's in (test_widgets.py was changed to carelessly, you're right ;) Some just need to be changed, though, as we are now catching Invalid exceptions, not looking at request.validation_errors.

That said, good night now Alberto

comment:34 Changed 13 years ago by kevin

Hi guys,

I'm only now getting a chance to look at this, and it's getting a bit late for me. So, I don't have a completely lucid mind as I'm reading over all of the discussion.

Requiring the user to manually pass in parameters on display() for the sake of eliminating input_values and validation_errors is not a good thing. User API is more important than implementation convenience.

If you find that the implementation is a lot cleaner with those passed in, it's possible to have the metaclass pass them in to display(). I do agree that passing the errors and input values in is more explicit, but it also seems like "busy work" (pointless action) if the user's never really going to change them.

It's worth noting that those values, while being treated like globals, are truly request-related. For that request, those are the values that came in and the errors that came up during validation.

Generating a schema sounds like a good strategy, and that's something that Karl was just asking me about a couple days ago. Cleaning up the implementation overall is a good idea. But, user convenience comes first.

I don't have anything more specific to say right now about the patches, because I'm too tired to review them coherently.

Thanks for all of the work to make this system better! I'll take another look here tomorrow.

Kevin

comment:35 Changed 13 years ago by michele

The patch I'm attaching seems to work, just 4 tests failing and they are related to values not being converted when a Schema is invalid.

Take a look please, I needed to use a recursive update for schema inspired by your for a dict, anyway things are being simplified.

Precedence is given to schema attribute that are higher in the parent tree, there shouldn't be threadsafe problem since every compoundwidget is making a new schema that's then updated.

That's only the first step and cherrypy.request.validation_errors is still managed by widgets, that's just for testing purpose as I said.

Changed 13 years ago by michele

patch

Changed 13 years ago by michele

New version

comment:36 Changed 13 years ago by michele

The new version is passing every pre-existing test, just 4 modification for taking into account that an invalid Schema doesn't covert good values and that now a Widget just like at display is validating only it's value like a validator, that's just the right thing and before it was asymmetric, a CompoundWidget? validates a dict of values like before and when its validate method is called it generates the right Schema to use on the fly and validates the dict against this. ;)

Changed 13 years ago by michele

New version, corrects updating of chained_validators and pre_validators

comment:37 Changed 13 years ago by alberto

Hi Michele,

Still busy as hell and can't take a close look at your patch (that means, applying and running it) but I think you're not addressing the main issue of the ticket.

As I understand it, what Simon was proposing to define a standard protocol for widgets to validate values and return errors. Which I belive was: 1) return coerced values, 2) raise exception if errors occur.

If what I've seen is correct, widgets are still "returning" their validation errors at cherrypy.request.validation_errors instead of raising them in an exception:

+    def validate(self, values):
+        # The exception should be managed by controller.py, validate here
+        # should just act as Widget and return the validated dict
+        # controller.py should then manage it in the right way as it's
+        # already doing with normal Schema since validating a Form is now
+        # like validatng a Schema
+        # that's the right thing to do:
+        #
+        # return self.retrieve_validator().to_python(values)
+        try:
+            validated_values = self.retrieve_validator().to_python(values)
+            values.clear()
+            values.update(validated_values)
+        except validators.Invalid, e:
+            self._adapt_schema_error_dict(e.error_dict)
+            self._set_errors(e.error_dict)

Really, this whole algorithm get's much easier if we respect a simple protocol like that.

The only drawback (regarding the API interface) is the explicit passing of state at display() which I've got an idea for a fix that I'd like to implement this afternoon (though I might get fired if caught :D )

Regarding recursive_update_schema at every node, it's not needed at all. If the whole algorithm is recursive, and we make sure every CompoundWidget? is responsible of updating it's schema (which I like your idea of building a new Schema instance and copying the "upgradee's" chained/pre_validators list) at init time then we don't need to do it on each validator lookup (were talking immutable fields, right??), it's already done by inductiveness.

Sorry, but I don't like your approach here :( The whole lot thing is easier if we approach this problem in a easy recursive manner. Every node should respect the same protocol (with a specialization in compunds, of course):

simple_widget:
    def validate(value)
          return coerce or raise Invalid

compund widget (this is already done by FE):
    def validate(value):
          for w in subwidgets:
             try:
                 value[w] = w.validate()
             except Invalid, e:
                 error[w] = e
           if errors:
                 raise Invalid(errors)
            return values

This simple algorithm builds the error and value tree by itself! Don't need to manually look where to place errors and values.

For display:

   form.display(values, errors):
      for w in form.subwidgets:
            w.display(value_for(w), error_for(w))

I know.... display should not receive the errors or input values as it breaks the current API but, as Kevin pointed out, we can make the metaclass (or a delegating proxy) feed the initial input_values and validation_errors, from cherrypy.request, to the *root* of the tree, that is the form.

The internal implemenation of widget templates should, however, use value_for and error_for (im thinking of simplifying this into state_for) to pass values to subchildren and walk down the tree, that is children will have explicit knowledge of *it's* errors and values, not the tree above it, they really should belive they are always the root and have the error and value subdict that corresponds to it (this is what value_for, error_for do: hand out the children values to children)

The good thing about adhering to this is that we let FE take care of building the error dict, which will simplify things when we implement repeating widgets (FE handles this already). I know, im going too far in the future... but the point is that we should simplify the protocol asap so it doesn't get in the way when we want to do more complex stuff. Limiting the knowledge of each subnode to what's essential to do it's action is a good start.

Regarding the cherrypy.request I know that it's not a *real* global but it *is* a global for the current request (threadlocal storage, right?). Widget's, IMO, should be completely request agnostic. Just make sure they're threadsafe so they can be shared among requests. Not only from a decoupling POV but from an efficency POV too: every widget has to follow all the their path from the root to find their value or error, instead of having them handled to them directly by their parent. And it complicates things: get_dict_from_parent is called in every widget, it's not needed with explicit passing of state: parent "gives" dict to children. I know 0.9a is out, but this could get harder to "fix" in the future... And I guess it's obvious that I'm willing to do the work/testing ;)

Well, hope I can back up my claims later with a test passing, API respecting patch :)

Regards, Alberto

comment:38 Changed 13 years ago by michele

Hi Alberto,

Yes, as I said widgets are still returning errors in validation_errors, but that's intentional and my first step for a gradual migration path that's let me run all tests without having to change them to handle the exception, in this way I've verified with no effort that my solution works on all our pre-existing tests as a drop in replacement, the comment I putted above validate clearly states that this is going to change, and it will eventually become just like yours: ;)

!#python
return self.retrieve_validator().to_python(values)

This means that on validation failing of a form you get a formencode exception that return the dict in formencode format that's just what Simon asked for, I'm not building any error dict (that's being done by FE) I'm just converting it to leverage our existings tests, Simone will be able to do this then (I can prepare a patch in 20 seconds if needed):

try:
  form.validate(values)
except validators.Invalid, e:
  errors = e.error_dict <--- our form errors dict formatted by formencode

that's == to it's second post request. ;-)

I think what Simon was asking for was simply this nothing more I can't see why you want to still mix #626 inside this ticket, let's find the best solution for this problem first of all and then discuss about other issues (#626) that are not related to this in any way. :D

The way I've adopted is this, you can validate every widget by passing to him it's value or a dict of values if it's a CompoundWidget?, only at validation you can build the top level Schema that will be used for validating this dict, if you can make it at init inside CompoundWidget? that's the best but I've been unable to do so since at init inside CompoundWidget? you can't know the self.widgets content ad hence provide a generalized solution that works for every compound widget.

I would like again to ask for a patch that only addresses this issue and if possible the first version of this patch should still put validation errors inside cherrypy.request.validation_errors without relying on controllers.py so that we can check that every test we have is still passing.

Regarding the cherrypy issue I do think that's not related to this ticket as I said, anyway I do think it's quite handy using it to provide a better user experience, again user are writing their code many times not once like we are going to do here so we should aim at user happiness not TG developers happiness. Finally even on our actual widgets every widget is just receiving only it's value or value dict not the whole dict, the only case when we need to use _get_dict_by_parent is when we are looking for our value in c.p.input_values and c.p.validation_errors and as I said that's a handy solution from the user POV. ;-), I'm also going to experiment a solution for the parent problem that will leverage cp.request and that again will provide a far better user experience for some things.

It seems you're overlooking something, the way value_of, options_for are working is not any different from what we are actually passing on the form template to widgets that's just the value of the widget being displayed not the entire value dict to be walked as you say.

for field, value, options, error in fields
assert value == value_of(field)

value is the value associated to that field, options the same, error the same, no parent tree walking required, that's only required when we are looking for things in the requests and this makes sense because we are providing a very useful shortcut for all of our users so that they get values and error displaying "for free", that's one of the main point in using the Widgets system, if for whatever reason someone doesn't need this it can just reset them inside the method but let's make life easier to who want to have them then the contrary please. :)

But again let's focus on this particular problem here, I'm very interested in taking a look at the solution/algorithm you have in mind for this problem (creating a top-level Schema for validating a CompoundWidget?) because I think my solution is not the best, but please make it only related to this problem, implemented inside CompoundWidget? and independent from external attributes like input_fields, and as a first step put errors in validation_errors by using the canonical format we expect as I've done using the adapt_error_dict method so we can check our tests before modifying them. ;-)

Thanks for keeping talking about this because the important thing is that at the end there will always be an improvement.

Ciao Michele

comment:39 Changed 13 years ago by alberto

Mmmm, I feel like banging my head against a wall :/

Anyway, my whole vision can be summarized in that if we do:

try:
  widget.validate(values)
except validators.Invalid, e:
  errors = e <--- our widget errors formatted by formencode

at *every* widget, not just the form everything get's *much simpler*, the rest of my arguments are really consequences of this. Stuffing errors into validation_errors just doesn't need to be done at every widget, the validate decorator should do it with the expection raised by the form. Same goes for input values.

its really easy:

   form.validate(value) 
                        -> sub.validate(value) 
                                                 -> sub2.validate(value)
                                                 <- raise error
                        <- catch and raise error
- catch and raise error
|
V

We have all errors in the exception. This is what FE does, why don't we just let it do it? I've already implemented it and works, as I've said, only tests need to be *adapted* to catch errors in this way, the essence of the tests is the same (this is bad values, I expect some errors. Good values, no errors). Doing it gradually is more cumbersome, the protocol is changing, remember? What I see a waste of time is and source of complication (and possible source of bugs) is to have an "intermediate" protocol just so the way we interface with widgets at the *tests* doesn't change. As I've said, the change at the decorator is the whole point of this (and to simplify things, which has it's own advantages altogether)

Regarding #626, I see no relation except in the name of the utility functions at the template (no currying or magic or anything) and a little bit of refactoring at the FieldSet? and Form (which are now simpler). What' is really changing in my patch is the explicit passing of values/errors at display, and the explicit catching of exceptions.

Anyway, I think we're already losing too much energy discussing this, you see it one way I see it another and we're just repating ourselves over and over... I give up, sorry, I've got loads of piled work to do and this discussion alone is already taking too much time for me. :(( Maybe this weekend I'll have time to get into this again.

Tell me when you're patches are ready and I'll submitt them.

regards, Alberto Alberto

comment:40 Changed 13 years ago by kevin

I think Alberto's plan sounds good. I think the change in raising an exception will be largely invisible to users (because it's hidden away by the decorators). My only complaint was with the change to display(), so as long as we deal with that, it sounds like a good cleanup.

comment:41 Changed 13 years ago by alberto

Ok, I'll try to get this working then, when I can sneak some time...

display() will need to change, but only optional parameters are needed (this is so we can pass state from parent to children while displaying) which will have an impact on how compounds render their subwidgets, that is compund's templates will need to pass value, errors and path to their subwidgets (I might come up with state variable which groups all these...) (see Forms at the patch).

What will not change is display at the user's templates, I'll try this get this done by pulling input_values and validation_errors from the request when value and errors are not passed to display. This should occur only at user templates when rendering the form.

Just to get it clear again, display needs to change signature so I can pass state to subwidgets, but I'll make it work as expected when no state is passed (pull from request) this should *only* happen at the root. This should hide all the details to widget users, but widget developers will need to take it into account when working with nested widgets.

Is this OK with everyone then?

Summary:

1) Widget's will still depend on cherrypy to get this initial state from the request *at display*, but this two vars will not get used again in the whole algorithm. It'll be the deco's responsability to catch the exception from the validation and putting it into the request, same for values.

2) Protocol as described by Simon

3) tests adapted to the new protocol.

4) Formencode-style nested errors (more flexible, no wheel-reinvention)

comment:42 Changed 13 years ago by michele

Well as I said my algorithm is not the best, and I've said I do think Alberto algorithm is the right one to use, I only asked for an medium migration path that let's us use our tests right before modifying the validate decorator, nothing more.

The only thing I want to point out is that with my patch there is only one widget putting errors into c.r.validation_errors and not any widget as Alberto said, that's because I'm not propagating the validate call to children but just building a Schema from the widget who received the validate call and then calling the to_python method on it, and the only reason I'm still putting errors into validation_errors is to use our actual tests nothing more as I said before, it's just a matter or removing 4 lines and changing the validate decorator we are not talking about two different things as you think.

I personally found it easy to hack using a temporary backward compatible way to check that tests are all still passing, anyway...

I will not have too much time myself either, but please at least accept to solve only #613 here and #626 in another step because they really are unrelated things to how errors are being register inside validation_errors (that's only an internal TG detail), the value_for, error_for and removal of cherrypy and other modifications to Form/FieldSet? are not required to make this work as my patch shows, and that's one of the reason I've made it.

I don't have commit access nor any authority to claim what's the best solution (that's clearly not the one I proposed) and that's not what I'm trying to do, I just want to discuss what's going on here and there because it's since November I'm putting many energy on widgets (even when I was supposed to prepare some exams) and I think we should be careful when we change things while 0.9a is already out and we have spent the last 2 weeks doing the parent mechanism and supporting Schema for CompoundWidgets.

What I asked for:

  • a first patch that only covers #613 and still uses validation_errors with a temporary bridge to reuse our tests without touching them
  • applying it to the trunk and chaining test to catch the FE exception and the validate decorator to use the same method
  • talking about #626 after that

All the other things I've written were for explaining what I was doing and asking some of your question not to criticize your solution that again as I said is the right one even for me since it's really very simple. ;-)

If discussing the best solution for you is loosing energy I will stop right now, sorry but that's not my POV. :-/

Sorry again, the final verdict is the same for me, since you're the one with commit access and the one with the blessed solution (even from me, I want this to be clear) , go ahead ASAP and do what you had in mind I will not discuss any further because it seems the problem here is me and I don't want to put stop energy on TG.

Update after your last reply, this means: OK from me, don't take into account my words above regarding #626 and do what you need to do, I just want to say (as Kevin) that changing the display signature is not ok with me and I even have/had a working patch that removes the need to pass parent to it (ideally it should just expect value and kw like options nothing more otherwise thing will be more complex for users). I do think your solution for the error dict is the right one.

Sorry again.

Ciao Michele

PS I will be away all day long so don't expect any other reply from me. ;-)

comment:43 Changed 13 years ago by michele

Just to point out that now we (me and Alberto) are working togheter and on the same track to solve this ticket and #626!!! :-)

Ciao!

comment:44 Changed 13 years ago by alberto

yeah, it's true... we decided to keep the yelling in private :D "unified" patch probably comming tomorrow... Alberto

comment:45 Changed 13 years ago by michele

So Alberto's patch looks very good and works at init.

This made me think that we can prepare retrieve_js and retrieve_css when we prepare the Schema instead of doing all the traversal on every request (damn!) since self.js and self.css can not change after init. What about a final_setup (better name absolutely needed) method that we call at the end of every init method of a widget? this prepares the Schema for Compounds, and _css_set, _js_set for retrieve_css and retrieve_js so they just look like:

def retrieve_js():
    return self._css_set

def retrieve_css():
    return self._js_set

what's cool is that we can then reuse the same widget traversal alghoritm to build those thing at the same time, for example in Compound:

    def final_setup(self):
        scripts = setlike()
        css = setlike()
        validator = ...
        [add self js]
        [add self css]
        [build a list of all widgets by iterating over self.widgets]
        for widget in [previous list]:
                    [add js]
                    [add css]
                    [prepare schema, skip if none]
        self._js_set = scripts
        self._css_set = css
        self.validator = validator

where [add js], [add css], [prepare schema] are functions.

This will improve performance for sure even if not much, but well redoing the same thing at every request is really a waste.

Ciao Michele

comment:46 Changed 13 years ago by michele

At this point we can even avoid making retrieve_css and retrieve_js methods (getters!! no please) and we simply use attributes, what about:

css_set
js_set

we need a small patch for controllers.py here.

Am I on crack regarding this thing? better going to sleep now!

Ciao

comment:47 Changed 13 years ago by alberto

Hi, No, no crack there I think :) subwidgets need to "exist", before becoming childs of a widget (the opposite of how we humans function ;) This means that we can fetch like you said all css, js and validators for subwidgets at init time. We just need to make sure to keep this lists separate for every widget as a widget can be a child for many widgets (these little creatures are really weird).

I'll try to refactor the subtree traversal in merge_schemas so we can use for this too, see how it goes....

Alberto

comment:48 Changed 13 years ago by michele

Yep, it should just work as you are doing with the schema, every widget puts it's js and css in js_set and css_set, that's all. I doesn't matter if it's child of two different widgets, this does matter only for the parent widget that just looks at js_set and css_set while preparing it's own js_set and css_set...

Pseudo code:

for widget in widgets:
   for cssitem in widget.css_set:
       self.css_set.add(cssitem)

Ciao!

comment:49 Changed 13 years ago by michele

I think we should use a metaclass (and decorator?) to automatically call this method at the end of every widget init, this way we ensure it's always called end we don't need to ask everyone to put it and the end of init.

comment:50 Changed 13 years ago by kevin

I believe that it is important for CSS and JS to be computable at request time. IIRC, there was a case where overriding that was used for l10n. It may make sense to try to optimize for the (common) case where it doesn't change, but I think there still needs to be a way to tailor the CSS and JS for a request.

comment:51 Changed 13 years ago by godoy

The case where L10N got in was CalendarDatePicker?, that chooses a language calendar. We solved that by passing a new parameter, but it still is fixed and the solution isn't optimal. We just found a way to override the "English only" problem by allowing a "<choose one and only one language> only" problem... :-)

comment:52 Changed 13 years ago by michele

Er, you're right indeed, we need this thing at request time for internationalization support, I don't know if ATM we are already doing this but we should be doing it at some point for sure. :-)

For Jorge, yep I think the actual solution is far from optimal, I'm not an i18n expert but I do think that at some point we should sit down and design a good solution for Javascript i18n.

comment:53 Changed 13 years ago by godoy

Don't worry, Michele. :-) I proposed the calendar solution so I'm very aware that it is suboptimal.

What I expected was being able to pass lazy_gettext objects everywhere I can pass strings / unicode.

If this is true, then a lot of our problems -- including the "everything should accept and work with lazy_gettext and unicode" problem -- should be over.

comment:54 Changed 13 years ago by michele

Hi Jorge,

mmm, I do worry. :D

I think i18n should work without the need to pass lazygettext, as I said I'm not an expert but I think we should use some standard rules for our javascript files and during the request we should just retrieve the right language file for the calendar or whatever by looking at the request language.

comment:55 Changed 13 years ago by godoy

I don't see how the Calendar case can be solved with headers or some request info. For example, there's "pt" and "pt-utf8" versions of the calendar javascript there. The same applies for other languages and there's some win charset as well. It's a mix of language and charset. Since it is a third-part component, I don't see how we can change that.

For other non-UI javascript I don't see a need for I18N (that doesn't mean there will never be another component that needs it...).

I still think that "lazy_gettext" should be mapped to "_" and "_" should be *the* I18N interface (better yet: merge lazy_gettext into gettext's code or use lazy_gettext for everything).

Changed 13 years ago by alberto

new validation patch

comment:56 Changed 13 years ago by alberto

I've just posted the patch. There's something weird going on here, keep getting 412 Precondition failed and just lost a long post commenting the patch :( (No, Safari doesn't save my forms when I press the back button :(( )

Maybe follow in turbogears-trunk ??

Alberto

Changed 13 years ago by alberto

Oops, forgot to svn add a new test, here it is....

Changed 13 years ago by alberto

almost there...

Changed 13 years ago by alberto

with meta.py refactoring

comment:57 Changed 13 years ago by anonymous

  • Status changed from new to closed
  • Resolution set to fixed

Committed at r926

Changed 13 years ago by alberto

Filtering extraneous fields from the input

Changed 13 years ago by alberto

No ugly hack at the schema

Changed 13 years ago by michele

cool

Changed 13 years ago by michele

new version

Changed 13 years ago by michele

move to forms.py

Changed 13 years ago by michele

just using the webspace :D

Note: See TracTickets for help on using tickets.