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 #721 (closed defect: fixed)

Opened 13 years ago

Last modified 12 years ago

Problems with Schema validation

Reported by: Baruch <turbogears@…> Owned by: anonymous
Priority: high Milestone: 0.9a5
Component: TG Widgets Version: 0.9a3
Severity: normal Keywords:
Cc:

Description

After upgrading from 0.9a1 to 0.9a3 I have a problem with the Schema validation, it can't find the fields of the form.

Attached is a 0.9a3 quickstarted project that shows the problem, steps to reproduce:

Attachments

tgtest.tar.bz2 Download (14.8 KB) - added by Baruch <turbogears@…> 13 years ago.
quickstart project that shows the bug
tgtest-corrected.tar.bz2 Download (14.6 KB) - added by alberto 13 years ago.
Corrected tgtest
721_fix.patch Download (978 bytes) - added by alberto 13 years ago.
Should this fix it?

Change History

Changed 13 years ago by Baruch <turbogears@…>

quickstart project that shows the bug

Changed 13 years ago by alberto

Corrected tgtest

comment:1 Changed 13 years ago by alberto

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

It's not a bug in TG code but a horrible lack of docs ;) The problem was that you decorated the "register" method with validate so the form was being validated when requesting "/register". Too bad no data was being submitted, hence the KeyError? exception. On the other hand, this case should have been handle gracefully (That is, display an error on the form) but the error handler was not rediecting the the method that displays the form.

Well, better see the corrected tgtest and see for yourself (I've modified register.kid and controllers.py, you can diff to see the changes).

Alberto

comment:2 Changed 13 years ago by Baruch <turbogears@…>

Ok, but that means that between 0.9a1 and 0.9a3 you removed the option to use the same method to validate and be the error handler. It generates a kind of ugly interface where I'll have a register method to display the form and doregister to get it submitted.

Guess I'll have to live with it.

comment:3 Changed 13 years ago by alberto

  • Status changed from closed to reopened
  • Resolution invalid deleted

Just replied in the ML. I'll reopen and take another look tomorrow...

comment:4 Changed 13 years ago by simon

  • Priority changed from normal to high

This should have been handled gracefully as is, with the method being it's own error handler, otherwise we again introduce subtitle differences between validation approaches (form vs. validators).

comment:5 Changed 13 years ago by Alberto

I've traced the cause to line 374 of controllers.py:

                # Remove any non-request-params from value
                tg_util.remove_keys(value, ifilterfalse(                        
                            cherrypy.request.params.has_key, kw.iterkeys()))  

However, I'm not sure ATM how to proceed. Excess args need to be removed in order to validate the form (or else 'self' and 'tg_errors' will get in the form and validation will fail miserably). However, if cherrypy.request.params is empty all defaults you provide at the method's signature will be wiped out.

This is certainly a bug. IIRC there was some discussion a while ago on how we should handle method's default args but I can't remember right now (too lazy ;) what was done about it. I'm pinging Simon and Michele too see if they can shed some light on this before I take a closer look.

Thanks for reporting this. Alberto.

comment:7 Changed 13 years ago by alberto

Not exactly... I think the problem in II is not exactly a problem (if I understood it correctly). Andrey says that validation worked when he added an extra field to the method (pwd3 I believe). This is normal because adapt_call will remove the 'pwd3' field from the args as it isn't in the methods signature (right?) and validation will pass as ignore_key_missing = True in the Schema. This is somewaht related to this  thread I believe.

BTW: I think we should remove ignore_key_missing = True from the Schema, there was some discussion on this a week ago but I haven't following the MLs too closely lately to see what was decided. I'll try to dig into this tomorrow more closely...

Alberto

comment:8 Changed 13 years ago by simon

I see, I must admit I did not heed Widget development lately.

comment:9 Changed 13 years ago by alberto

Can you please try the patch I'm about to attach? I get get an expat error from kid with your project, but it's probably my fault.

I've removed the above snippet from validate() and added allow_extra_fields to the Schema. This shouldn't do any harm as the Schema will filter them after validation (by the grace of filter_extra_fields). All tests are passing so I guess I'm right ;)

I'll commit it tomorrow if it works for you.

Changed 13 years ago by alberto

Should this fix it?

comment:10 Changed 13 years ago by kevin

  • Milestone changed from 0.9a4 to 0.9a5

comment:11 Changed 13 years ago by Baruch <turbogears@…>

The patch fixes it indeed!

comment:12 Changed 13 years ago by alberto

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

Fixed at [1099]. Thanks for the feedback! alberto

Note: See TracTickets for help on using tickets.