- chooseName now never fails if the suggested name is in the wrong type.

- checkName now checks the type first in checkName
- Convert most namechooser doctests into unittests
- Added more test cases

CHANGES.txt

@@ -9,6 +9,10 @@ CHANGES
   it directly. Once we can rely on the new ZODB3 version exclusively, we can
   remove the dependency onto the zope.broken distribution.
 
+- never fail if the suggested name is in a wrong type (#227617)
+
+- checkName first checks the parameter type before the emptiness
+
 3.11.0 (2009-12-31)
 -------------------
 

src/zope/container/contained.py

@@ -124,7 +124,7 @@ def dispatchToSublocations(object, event):
 
          >>> component.provideHandler(dispatchToSublocations,
          ...   [None, IObjectMovedEvent])
- 
+
        We can then call the dispatcher for the root object:
 
          >>> event = ObjectRemovedEvent(c)
@@ -359,7 +359,7 @@ def setitem(container, setitemf, name, object):
     ...   [IItem, IObjectAddedEvent])
     >>> component.provideHandler(lambda obj, event: obj.setMoved(event),
     ...   [IItem, IObjectMovedEvent])
-    
+
     >>> item = Item()
 
     >>> container = {}
@@ -684,41 +684,34 @@ class NameChooser(object):
         >>> container['foo'] = 'bar'
         >>> from zope.container.contained import NameChooser
 
-        All these names are invalid:
+        An invalid name raises a ValueError:
 
         >>> NameChooser(container).checkName('+foo', object())
         Traceback (most recent call last):
         ...
         ValueError: Names cannot begin with '+' or '@' or contain '/'
-        >>> NameChooser(container).checkName('@foo', object())
-        Traceback (most recent call last):
-        ...
-        ValueError: Names cannot begin with '+' or '@' or contain '/'
-        >>> NameChooser(container).checkName('f/oo', object())
-        Traceback (most recent call last):
-        ...
-        ValueError: Names cannot begin with '+' or '@' or contain '/'
+
+        A name that already exists raises a KeyError:
+
         >>> NameChooser(container).checkName('foo', object())
         Traceback (most recent call last):
         ...
         KeyError: u'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
 
         We can reserve some names by providing a IReservedNames adapter
         to a container:
-        
-        >>> NameChooser(container).checkName('reserved', None)
-        True
-        >>> NameChooser(container).checkName('other', None)
-        True
 
         >>> from zope.container.interfaces import IContainer
         >>> class ReservedNames(object):
@@ -728,29 +721,24 @@ class NameChooser(object):
         ...     def __init__(self, context):
         ...         self.reservedNames = set(('reserved', 'other'))
 
-        >>> zope.component.provideAdapter(ReservedNames)
+        >>> zope.component.getSiteManager().registerAdapter(ReservedNames)
 
         >>> NameChooser(container).checkName('reserved', None)
         Traceback (most recent call last):
         ...
         NameReserved: reserved
-        >>> NameChooser(container).checkName('other', None)
-        Traceback (most recent call last):
-        ...
-        NameReserved: other
-
         """
 
-        if not name:
-            raise ValueError(
-                _("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 ValueError(
+                _("An empty name was provided. Names cannot be empty.")
+                )
+
         if name[:1] in '+@' or '/' in name:
             raise ValueError(
                 _("Names cannot begin with '+' or '@' or contain '/'")
@@ -773,33 +761,51 @@ class NameChooser(object):
         """See zope.container.interfaces.INameChooser
 
         The name chooser is expected to choose a name without error
-        
+
         We create and populate a dummy container
 
         >>> from zope.container.sample import SampleContainer
         >>> container = SampleContainer()
-        >>> container['foo.old.rst'] = 'rst doc'
+        >>> container['foobar.old'] = 'rst doc'
 
         >>> from zope.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:]
@@ -807,7 +813,6 @@ class NameChooser(object):
         else:
             suffix = ''
 
-
         n = name + suffix
         i = 1
         while n in container:

src/zope/container/tests/test_contained.py

@@ -23,10 +23,13 @@ import transaction
 from persistent import Persistent
 
 import zope.interface
+import zope.component
 from zope.testing import doctest
 
-from zope.container.contained import ContainedProxy
+from zope.container.contained import ContainedProxy, NameChooser
+from zope.container.sample import SampleContainer
 from zope.container import testing
+from zope.container.interfaces import NameReserved, IContainer, IReservedNames, IReservedNames
 
 class MyOb(Persistent):
     pass
@@ -313,15 +316,99 @@ 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()))
+
+        # reserved names
+        class ReservedNames(object):
+            zope.component.adapts(IContainer)
+            zope.interface.implements(IReservedNames)
+            def __init__(self, context):
+                self.reservedNames = set(('reserved', 'other'))
+        zope.component.getSiteManager().registerAdapter(ReservedNames)
+
+        self.assertRaises(NameReserved, checkName, 'reserved', object())
+        self.assertRaises(NameReserved, checkName, 'other', object())
+        self.assertRaises(NameReserved, checkName, u'reserved', object())
+        self.assertRaises(NameReserved, checkName, u'other', 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.container.contained',
                              setUp=testing.setUp,
                              tearDown=testing.tearDown),
         doctest.DocTestSuite(optionflags=doctest.NORMALIZE_WHITESPACE),
+        unittest.makeSuite(TestNameChooser),
         ))
 
 if __name__ == '__main__': unittest.main()