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

Opened 10 years ago

Last modified 9 years ago

FieldStorageUploadConverter doesn't support "not_empty" option to make it optional

Reported by: godoy Owned by: faide
Priority: normal Milestone: 1.0.x bugfix
Component: TurboGears Version: 1.0.4.2
Severity: normal Keywords:
Cc:

Description

When specifying a FileField? widget and assigning to it a validator of FieldStorageUploadConverter? it doesn't allow the field to be made optional by specifying "not_empty=False" on the validator. The following patch accomplishes that:

Index: validators.py
===================================================================
--- validators.py       (revisão 4084)
+++ validators.py       (cópia de trabalho)
@@ -177,13 +177,17 @@
 # see #1464357 on FE bugtracker (http://tinyurl.com/lm9ae).
 # Custom version of FieldStorage validator that does not break FE schema validator.
 class FieldStorageUploadConverter(TgFancyValidator):
+    def __init__(self, *args, **kwargs):
+        super(TgFancyValidator, self).__init__(*args, **kwargs)
+        self.not_empty = kwargs.get('not_empty')
+
     def to_python(self, value, state=None):
         if isinstance(value, cgi.FieldStorage):
             if value.filename:
                 return value
-            raise Invalid('invalid', value, state)
-        else:
-            return value
+            if self.not_empty:
+                raise Invalid('invalid', value, state)
+        return value

 # For translated messages that are not wrapped in a Validator.messages
 # dictionary, we need to reinstate the Turbogears gettext function under

If you agree with this change, I'll commit it to the code.

Change History

comment:1 Changed 10 years ago by godoy

  • Owner changed from anonymous to godoy

comment:2 Changed 10 years ago by chrisz

I haven't really looked into this, but maybe the real problem is that what is called to_python here should be really _to_python?

comment:3 Changed 10 years ago by godoy

Not without the changes I proposed, at least. If I revert the above patch on my local copy and just rename the method, then it doesn't work as expected.

Implementing just the init method, as proposed above, with a rename of the method from to_python to _to_python also doesn't work.

Keeping the patch and renaming the method also works, but the above patch is needed.

With regards to the method name, no change is really needed, though it should be done to conform to the documentation on inherited and overriden methods. From formencode.validators.FancyValidator?:

 |  Validators have two important external methods:
 |
 |  * .to_python(value, state):
 |    Attempts to convert the value.  If there is a problem, or the
 |    value is not valid, an Invalid exception is raised.  The
 |    argument for this exception is the (potentially HTML-formatted)
 |    error message to give the user.
(...)

and

 |  There are five important methods for subclasses to override,
 |  however none of these *have* to be overridden, only the ones that
 |  are appropriate for the validator:
 |
(...)
 |  * ._to_python(value, state):
 |    This returns the converted value, or raises an Invalid
 |    exception if there is an error.  The argument to this exception
 |    should be the error message.
(...)

A new patch, then, would be:

Index: validators.py
===================================================================
--- validators.py       (revisão 4084)
+++ validators.py       (cópia de trabalho)
@@ -177,13 +177,17 @@
 # see #1464357 on FE bugtracker (http://tinyurl.com/lm9ae).
 # Custom version of FieldStorage validator that does not break FE schema validator.
 class FieldStorageUploadConverter(TgFancyValidator):
-    def to_python(self, value, state=None):
+    def __init__(self, *args, **kwargs):
+        super(TgFancyValidator, self).__init__(*args, **kwargs)
+        self.not_empty = kwargs.get('not_empty')
+
+    def _to_python(self, value, state=None):
         if isinstance(value, cgi.FieldStorage):
             if value.filename:
                 return value
-            raise Invalid('invalid', value, state)
-        else:
-            return value
+            if self.not_empty:
+                raise Invalid('invalid', value, state)
+        return value
 
 # For translated messages that are not wrapped in a Validator.messages
 # dictionary, we need to reinstate the Turbogears gettext function under

comment:4 Changed 10 years ago by chrisz

Since TgFancyValidator inherits from Declarative, it sets instance variables automatically in the constructor. So setting self.not_empty explicitly in __init__ shouldn't be needed.

I still think the only problem is that TG overrides to_python instead of _to_python, because the not-empty-case is handled in to_python, and if you override it, it doesn't work any more.

I tried the following:

>>> import formencode
>>> formencode.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
formencode.api.Invalid: Please enter a value

So, in formencode it works. Now with turbogears:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
''

So, turbogears ignores the not_empty flag (this is your bug). Now I fix turbogears.validators by renaming to_python to _to_python in the FieldStorageUploadConverter class, and try again:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
formencode.api.Invalid: Please enter a value

So after the rename, turbogears behaves as expected.

comment:5 Changed 10 years ago by godoy

Actually, the case is making it optional, not mandatory. But it works with formencode, yes.

>>> import formencode
>>> formencode.validators.FieldStorageUploadConverter(
... not_empty=False).to_python('')
>>>

With the original code in TG:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=True).to_python('')
''
>>> 

This is a first failure. It should be invalid.

Renaming the method, as you suggested:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=True).to_python('')
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/usr/local/lib/python2.5/site-packages/FormEncode-0.7.1-py2.5.egg/formencode/api.py", line 357, in to_python
    raise Invalid(self.message('empty', state), value, state)
formencode.api.Invalid: Please enter a value
>>> 

And now making the field optional:

>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=False).to_python('')
>>> 

I have to check if there is some other layer that is failing for a real form or not. I only tested this on the full application and it had still fails, even though it shouldn't according to what I have coded above.

I'll check if there's no other point in the TG Widget system that might be causing this failure.

So, for the validation part, just fixing the name should do it, indeed.

Any objections to commiting that fix?

comment:6 Changed 10 years ago by chrisz

I think instead of renaming the method we can remove the FieldStorageUploadConverter? class completely from TurboGears, since after the rename it would be the same as in FormEncode anyway. It seems this class was only included because of a bug in a very old FormEncode version.

comment:7 Changed 10 years ago by chrisz

  • Status changed from new to closed
  • Version changed from trunk to 1.0.4.2
  • Resolution set to fixed

Fixed now in r4106. Please let me know if is is still not working as expected.

comment:8 Changed 9 years ago by faide

  • Status changed from closed to reopened
  • Resolution fixed deleted

In TG 1.0.x revision 4154 it does not work. It is not yet possible to make an fieldstorage optionnal.

comment:9 Changed 9 years ago by faide

  • Status changed from reopened to new
  • Owner changed from godoy to faide

comment:10 Changed 9 years ago by faide

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

applied in r4155 (1.0) and r4156 (1.1). Thx Jorge!

comment:11 Changed 9 years ago by chrisz

  • Status changed from closed to reopened
  • Resolution fixed deleted

Ok, I see the problem now. FormEncode does not have the "if self.not_empty:" line.

I have two issues with the last checkin, though:

  • The formencode bug #1464357 ( http://tinyurl.com/lm9ae) mentioned there in the comment seems to be a different problem that has been already closed. So we should open a new bug on the formencode bugtracker and point to that instead. It is confusing to point to the wrong place and it makes no sense to solve this on the q.t. in TG without giving FE the chance to know about this bug and fix it.
  • I'm pretty sure that the __init__ method is not needed, since as I said above, instance variables are set automatically for all FancyValidators. Can you confirm that it can be removed? This would avoid some confusion about what we are patching here against the original FE code.

comment:12 Changed 9 years ago by faide

Chris, I will look into this and come back to this ticket to notify...

comment:13 Changed 9 years ago by chrisz

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

I have already cared for this in r4183. There is now a unit test for this ticket, and I have submitted this as FormEncode bug 1905250 with our solution as suggested patch.

Please reopen again if there are still any issues with this.

comment:14 Changed 9 years ago by faide

Applied another fix in r4192 to make this work because the validator was returning a filename (string) instead of the cgi.FieldStorage? instance we need to grab the filedata from.

This now works as excpected but the FE bug report and patch should surely be improved to reflect this.

comment:15 Changed 9 years ago by chrisz

You're right - sorry for botching this up. The name of the FieldStorageUploadConverter? class and its docstring made me believe it actually converted the cgi.FieldStorage? instance to something (the filename) while in fact it should return the instance as it is, and only check if the filename is there. I have now adapted the unittests accordingly and merged this to 1.1 as well. Will also fix the FE bug report.

Note: See TracTickets for help on using tickets.