bugfix backport from zope.container (#227617)

CHANGES.txt

@@ -2,6 +2,13 @@
 CHANGES
 =======
 
+3.6.3 (unreleased)
+------------------
+
+- never fail if the suggested name is in a wrong type (#227617)
+
+- checkName first checks the parameter type before the emptiness
+
 3.6.2 (2008-10-21)
 ------------------
 

setup.py

@@ -22,7 +22,7 @@ def read(*rnames):
     return open(os.path.join(os.path.dirname(__file__), *rnames)).read()
 
 setup(name='zope.app.container',
-      version = '3.6.3',
+      version = '3.6.3dev',
       author='Zope Corporation and Contributors',
       author_email='zope-dev@zope.org',
       description='Zope Container',

src/zope/app/container/contained.py

@@ -714,47 +714,44 @@ def checkName(self, name, object):
         >>> container['foo'] = 'bar'
         >>> from zope.app.container.contained import NameChooser
 
-        All these names are invalid:
+        An invalid name raises a UserError:
 
         >>> NameChooser(container).checkName('+foo', object())
         Traceback (most recent call last):
         ...
         UserError: Names cannot begin with '+' or '@' or contain '/'
-        >>> NameChooser(container).checkName('@foo', object())
-        Traceback (most recent call last):
-        ...
-        UserError: Names cannot begin with '+' or '@' or contain '/'
-        >>> NameChooser(container).checkName('f/oo', object())
-        Traceback (most recent call last):
-        ...
-        UserError: Names cannot begin with '+' or '@' or contain '/'
+
+        A name that already exists raises a UserError:
+
         >>> NameChooser(container).checkName('foo', object())
         Traceback (most recent call last):
         ...
         UserError: The given name is already being used
+
+        A name must be a string or unicode string:
+
         >>> NameChooser(container).checkName(2, object())
         Traceback (most recent call last):
         ...
         TypeError: ('Invalid name type', <type 'int'>)
 
-        This one is ok:
+        A correct name returns True:
 
         >>> NameChooser(container).checkName('2', object())
         True
 
-
         """
 
-        if not name:
-            raise UserError(
-                _("An empty name was provided. Names cannot be empty.")
-                )
-
         if isinstance(name, str):
             name = unicode(name)
         elif not isinstance(name, unicode):
             raise TypeError("Invalid name type", type(name))
 
+        if not name:
+            raise UserError(
+                _("An empty name was provided. Names cannot be empty.")
+                )
+
         if name[:1] in '+@' or '/' in name:
             raise UserError(
                 _("Names cannot begin with '+' or '@' or contain '/'")
@@ -772,41 +769,58 @@ def chooseName(self, name, object):
         """See zope.app.container.interfaces.INameChooser
 
         The name chooser is expected to choose a name without error
-        
+
         We create and populate a dummy container
 
         >>> from zope.app.container.sample import SampleContainer
         >>> container = SampleContainer()
-        >>> container['foo.old.rst'] = 'rst doc'
+        >>> container['foobar.old'] = 'rst doc'
 
         >>> from zope.app.container.contained import NameChooser
-        >>> NameChooser(container).chooseName('+@+@foo.old.rst', object())
-        u'foo.old-2.rst'
-        >>> NameChooser(container).chooseName('+@+@foo/foo', object())
+
+        the suggested name is converted to unicode:
+
+        >>> NameChooser(container).chooseName('foobar', object())
+        u'foobar'
+
+        If it already exists, a number is appended but keeps the same extension:
+
+        >>> NameChooser(container).chooseName('foobar.old', object())
+        u'foobar-2.old'
+
+        Bad characters are turned into dashes:
+
+        >>> NameChooser(container).chooseName('foo/foo', object())
         u'foo-foo'
-        >>> NameChooser(container).chooseName('', object())
-        u'object'
-        >>> NameChooser(container).chooseName('@+@', object())
-        u'object'
+
+        If no name is suggested, it is based on the object type:
+
+        >>> NameChooser(container).chooseName('', [])
+        u'list'
 
         """
 
         container = self.context
 
-        # remove characters that checkName does not allow
-        name = unicode(name.replace('/', '-').lstrip('+@'))
+        # convert to unicode and remove characters that checkName does not allow
+        try:
+            name = unicode(name)
+        except:
+            name = u''
+        name = name.replace('/', '-').lstrip('+@')
 
         if not name:
             name = unicode(object.__class__.__name__)
-
+
+        # for an existing name, append a number.
+        # We should keep client's os.path.extsep (not ours), we assume it's '.'
         dot = name.rfind('.')
         if dot >= 0:
             suffix = name[dot:]
             name = name[:dot]
         else:
             suffix = ''
 
-
         n = name + suffix
         i = 1
         while n in container:

src/zope/app/container/tests/test_contained.py

@@ -23,10 +23,13 @@
 from persistent import Persistent
 
 import zope.interface
+import zope.component
 from zope.testing import doctest
 
-from zope.app.container.contained import ContainedProxy
+from zope.app.container.contained import ContainedProxy, NameChooser
+from zope.app.container.sample import SampleContainer
 from zope.app.testing import placelesssetup
+from zope.container.interfaces import NameReserved, IContainer
 
 class MyOb(Persistent):
     pass
@@ -313,15 +316,86 @@ def test_ContainedProxy_instances_have_no_instance_dictionaries():
 
     >>> p.__dict__ is c.__dict__
     True
-    
+
     """
 
+
+class TestNameChooser(unittest.TestCase):
+    def test_checkName(self):
+        container = SampleContainer()
+        container['foo'] = 'bar'
+        checkName = NameChooser(container).checkName
+
+        # invalid type for the name
+        self.assertRaises(TypeError, checkName, 2, object())
+        self.assertRaises(TypeError, checkName, [], object())
+        self.assertRaises(TypeError, checkName, None, object())
+        self.assertRaises(TypeError, checkName, None, None)
+
+        # invalid names
+        self.assertRaises(ValueError, checkName, '+foo', object())
+        self.assertRaises(ValueError, checkName, '@foo', object())
+        self.assertRaises(ValueError, checkName, 'f/oo', object())
+        self.assertRaises(ValueError, checkName, '', object())
+
+        # existing names
+        self.assertRaises(KeyError, checkName, 'foo', object())
+        self.assertRaises(KeyError, checkName, u'foo', object())
+
+        # correct names
+        self.assertEqual(True, checkName('2', object()))
+        self.assertEqual(True, checkName(u'2', object()))
+        self.assertEqual(True, checkName('other', object()))
+        self.assertEqual(True, checkName(u'reserved', object()))
+        self.assertEqual(True, checkName(u'r\xe9served', object()))
+
+    def test_chooseName(self):
+        container = SampleContainer()
+        container['foo.old.rst'] = 'rst doc'
+        nc = NameChooser(container)
+
+        # correct name without changes
+        self.assertEqual(nc.chooseName('foobar.rst', None),
+                         u'foobar.rst')
+        self.assertEqual(nc.chooseName(u'\xe9', None),
+                         u'\xe9')
+
+        # automatically modified named
+        self.assertEqual(nc.chooseName('foo.old.rst', None),
+                         u'foo.old-2.rst')
+        self.assertEqual(nc.chooseName('+@+@foo.old.rst', None),
+                         u'foo.old-2.rst')
+        self.assertEqual(nc.chooseName('+@+@foo/foo+@', None),
+                         u'foo-foo+@')
+
+        # empty name
+        self.assertEqual(nc.chooseName('', None), u'NoneType')
+        self.assertEqual(nc.chooseName('@+@', []), u'list')
+
+        # if the name is not a string it is converted
+        self.assertEqual(nc.chooseName(None, None), u'None')
+        self.assertEqual(nc.chooseName(2, None), u'2')
+        self.assertEqual(nc.chooseName([], None), u'[]')
+        container['None'] = 'something'
+        self.assertEqual(nc.chooseName(None, None), u'None-2')
+        container['None-2'] = 'something'
+        self.assertEqual(nc.chooseName(None, None), u'None-3')
+
+        # even if the given name cannot be converted to unicode
+        class BadBoy:
+            def __unicode__(self):
+                raise Exception
+
+        self.assertEqual(nc.chooseName(BadBoy(), set()), u'set')
+
+
 def test_suite():
     return unittest.TestSuite((
         doctest.DocTestSuite('zope.app.container.contained',
                              setUp=placelesssetup.setUp,
                              tearDown=placelesssetup.tearDown),
         doctest.DocTestSuite(optionflags=doctest.NORMALIZE_WHITESPACE),
+        unittest.makeSuite(TestNameChooser),
         ))
 
 if __name__ == '__main__': unittest.main()