Do not catch ValueError in ``node.ext.ldap._node.LDAPStorage.batc…

…hed_search``. Increase test coverage. Some more cleanup

CHANGES.rst

@@ -4,6 +4,10 @@ History
 1.0b7 (unreleased)
 ------------------
 
+- Do not catch ``ValueError`` in
+  ``node.ext.ldap._node.LDAPStorage.batched_search``.
+  [rnix]
+
 - Use property decorators for ``node.ext.ldap._node.LDAPStorage.changed``
   and ``node.ext.ldap.session.LDAPSession.baseDN``.
   [rnix]

src/node/ext/ldap/__init__.py

@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+# do not change import order
 from node.ext.ldap.scope import BASE
 from node.ext.ldap.scope import ONELEVEL
 from node.ext.ldap.scope import SUBTREE

src/node/ext/ldap/_node.py

@@ -83,10 +83,10 @@ def load(self):
         )
         # result length must be 1
         if len(entry) != 1:
-            raise RuntimeError(                            # pragma NO COVERAGE
-                u"Fatal. Expected entry does not exist "   # pragma NO COVERAGE
-                u"or more than one entry found"            # pragma NO COVERAGE
-            )                                              # pragma NO COVERAGE
+            raise RuntimeError(                            #pragma NO COVERAGE
+                u"Fatal. Expected entry does not exist "   #pragma NO COVERAGE
+                u"or more than one entry found"            #pragma NO COVERAGE
+            )                                              #pragma NO COVERAGE
         # read attributes from result and set to self
         attrs = entry[0][1]
         for key, item in attrs.items():
@@ -443,7 +443,7 @@ def exists(self):
             )
             # this probably never happens
             if len(res) != 1:
-                raise RuntimeError()
+                raise RuntimeError()                        #pragma NO COVERAGE
             return True
         except NO_SUCH_OBJECT:
             return False
@@ -559,13 +559,10 @@ def batched_search(self, page_size=None, search_func=None, **kw):
         cookie = None
         kw['page_size'] = page_size
         while True:
-            try:
-                kw['cookie'] = cookie
-                matches, cookie = search_func(**kw)
-                for item in matches:
-                    yield item
-            except ValueError:
-                break
+            kw['cookie'] = cookie
+            matches, cookie = search_func(**kw)
+            for item in matches:
+                yield item
             if not cookie:
                 break
 

src/node/ext/ldap/base.py

@@ -5,7 +5,6 @@
 from node.ext.ldap.interfaces import ICacheProviderFactory
 from node.ext.ldap.properties import LDAPProps
 from zope.component import queryUtility
-
 import hashlib
 import ldap
 import logging
@@ -17,14 +16,11 @@
 def testLDAPConnectivity(server=None, port=None, props=None):
     """Function to test the availability of the LDAP Server.
 
-    server
-        Server IP or name
-
-    port
-        LDAP port
-
-    props
-        LDAPProps object. If given, server and port are ignored.
+    :param server: Server IP or name
+    :param port: LDAP port
+    :param props: LDAPProps object. If given, server and port are ignored.
+    :return object: Either string 'success' if connectivity, otherwise ldap
+        error instance.
     """
     if props is None:
         props = LDAPProps(server=server, port=port)
@@ -39,7 +35,10 @@ def testLDAPConnectivity(server=None, port=None, props=None):
 
 
 def md5digest(key):
-    """abbrev to create a md5 hex digest
+    """Abbrev to create a md5 hex digest.
+
+    :param key: Key to create a md5 hex digest for.
+    :return digest: hex digest.
     """
     m = hashlib.md5()
     m.update(key)
@@ -69,7 +68,9 @@ class LDAPConnector(object):
     """
 
     def __init__(self, props=None):
-        """Initialize LDAPConnector.
+        """Initialize LDAP connector.
+
+        :param props: ``LDAPServerProperties`` instance.
         """
         self.protocol = ldap.VERSION3
         self._uri = props.uri
@@ -96,7 +97,7 @@ def bind(self):
         if self._start_tls:
             # ignore in tests for now. nevertheless provide a test environment
             # for TLS and SSL later
-            self._con.start_tls_s()                        # pragma NO COVERAGE
+            self._con.start_tls_s()                        #pragma NO COVERAGE
         self._con.simple_bind_s(self._bindDN, self._bindPW)
         return self._con
 
@@ -116,9 +117,9 @@ class LDAPCommunicator(object):
     """
 
     def __init__(self, connector):
-        """
-        connector
-            LDAPConnector instance.
+        """Initialize LDAP communicator.
+
+        :param connector: ``LDAPConnector`` instance.
         """
         self.baseDN = ''
         self._connector = connector
@@ -162,36 +163,21 @@ def search(self, queryFilter, scope, baseDN=None,
                page_size=None, cookie=None):
         """Search the directory.
 
-        queryFilter
-            LDAP query filter
-
-        scope
-            LDAP search scope
-
-        baseDN
-            Search base. Defaults to ``self.baseDN``
-
-        force_reload
-            Force reload of result if cache enabled.
-
-        attrlist
-            LDAP attrlist to query.
-
-        attrsonly
-            Flag whether to return only attribute names, without corresponding
-            values.
-
-        page_size
-            Number of items per page, when doing pagination.
-
-        cookie
-            Cookie string returned by previous search with pagination.
+        :param queryFilter: LDAP query filter
+        :param scope: LDAP search scope
+        :param baseDN: Search base. Defaults to ``self.baseDN``
+        :param force_reload: Force reload of result if cache enabled.
+        :param attrlist: LDAP attrlist to query.
+        :param attrsonly: Flag whether to return only attribute names, without
+            corresponding values.
+        :param page_size: Number of items per page, when doing pagination.
+        :param cookie: Cookie string returned by previous search with
+            pagination.
         """
         if baseDN is None:
             baseDN = self.baseDN
             if not baseDN:
                 raise ValueError(u"baseDN unset.")
-
         if page_size:
             if cookie is None:
                 cookie = ''
@@ -209,7 +195,6 @@ def _search(baseDN, scope, queryFilter,
             # in case we do pagination of results
             if type(attrlist) in (list, tuple):
                 attrlist = [str(_) for _ in attrlist]
-
             msgid = self._con.search_ext(
                 baseDN,
                 scope,
@@ -225,7 +210,6 @@ def _search(baseDN, scope, queryFilter,
                 return results, pctrls[0].cookie
             else:
                 return results
-
         args = [baseDN, scope, queryFilter, attrlist, attrsonly, serverctrls]
         if self._cache:
             key_items = [
@@ -251,11 +235,8 @@ def _search(baseDN, scope, queryFilter,
     def add(self, dn, data):
         """Insert an entry into directory.
 
-        dn
-            adding DN
-
-        data
-            dict containing key/value pairs of entry attributes
+        :param dn: Adding DN
+        :param data: Dict containing key/value pairs of entry attributes
         """
         attributes = [(k, v) for k, v in data.items()]
         self._con.add_s(dn, attributes)

src/node/ext/ldap/base.rst

@@ -28,7 +28,7 @@ LDAP credentials::
     ...     server=host,
     ...     port=port,
     ...     user=binddn,
-    ...     password=bindpw,
+    ...     password=bindpw
     ... )
 
 Test main script, could be used by command line with
@@ -94,6 +94,31 @@ Set base dn and check if previously imported entries are present.::
     >>> len(res)
     7
 
+Test search pagination::
+
+    >>> res, cookie = communicator.search(
+    ...     '(objectClass=*)', SUBTREE, page_size=4, cookie='')
+    >>> len(res)
+    4
+
+    >>> res, cookie = communicator.search(
+    ...     '(objectClass=*)', SUBTREE, page_size=4, cookie=cookie)
+
+    >>> len(res)
+    3
+
+    >>> cookie
+    ''
+
+Pagination search fails if cookie but no page size given::
+
+    >>> res, cookie = communicator.search(
+    ...     '(objectClass=*)', SUBTREE, page_size=4, cookie='')
+    >>> communicator.search('(objectClass=*)', SUBTREE, cookie=cookie)
+    Traceback (most recent call last):
+      ...
+    ValueError: cookie passed without page_size
+
 Test inserting entries.::
 
     >>> entry = {

src/node/ext/ldap/filter.py

@@ -58,8 +58,7 @@ def __or__(self, other):
         return LDAPFilter(res)
 
     def __contains__(self, attr):
-        attr = '(%s=' % (attr,)
-        return attr in self._filter
+        return self._filter.find('({}='.format(attr)) > -1
 
     def __str__(self):
         return self._filter and self._filter or ''
@@ -100,34 +99,21 @@ def __str__(self):
         """turn relation string into ldap filter string
         """
         dictionary = dict()
-
         parsedRelation = dict()
         for pair in self.relation.split('|'):
             k, _, v = pair.partition(':')
             if k not in parsedRelation:
                 parsedRelation[k] = list()
             parsedRelation[k].append(v)
-
         existing = [x for x in self.gattrs]
         for k, vals in parsedRelation.items():
             for v in vals:
-                if (
-                    str(v) == '' or
+                if (str(v) == '' or
                     str(k) == '' or
-                    str(k) not in existing
-                ):
+                    str(k) not in existing):
                     continue
                 dictionary[str(v)] = self.gattrs[str(k)]
-
         self.dictionary = dictionary
-
-        # this "if/else creates an unused result and has no effect
-        # _filter = LDAPFilter()
-        # if len(dictionary) is 1:
-        #    _filter = LDAPFilter(self.relation)
-        # else:
-        #     _filter = dict_to_filter(parsedRelation, self.or_search)
-
         if self.dictionary:
             return str(dict_to_filter(self.dictionary, self.or_search))
         return ''

src/node/ext/ldap/filter.rst

@@ -52,6 +52,12 @@ another LDAPFilter, a string or a None type.::
     >>> foo
     LDAPFilter('(a=ä)')
 
+    >>> 'a' in foo
+    True
+
+    >>> 'objectClass' in foo
+    False
+
     >>> filter = LDAPFilter('(objectClass=person)')
     >>> filter |= LDAPFilter('(objectClass=some)')
     >>> filter
@@ -69,6 +75,9 @@ another LDAPFilter, a string or a None type.::
     >>> filter
     LDAPFilter('(|(objectClass=personä)(objectClass=someä))')
 
+    >>> 'objectClass' in filter
+    True
+
 
 LDAPDictFilter
 --------------
@@ -133,6 +142,13 @@ from relations.::
     >>> node.attrs['someUid'] = u'123\xe4'
     >>> node.attrs['someName'] = 'Name'
 
+    >>> rel_filter = LDAPRelationFilter(node, '')
+    >>> rel_filter
+    LDAPRelationFilter('')
+
+    >>> str(rel_filter)
+    ''
+
     >>> rel_filter = LDAPRelationFilter(node, 'someUid:otherUid')
     >>> rel_filter
     LDAPRelationFilter('(otherUid=123ä)')

src/node/ext/ldap/session.py

@@ -94,17 +94,12 @@ def authenticate(self, dn, pw):
     def modify(self, dn, data, replace=False):
         """Modify an existing entry in the directory.
 
-        dn
-            Modification DN
-
-        #data
-        #    either list of 3 tuples (look at
-        #    node.ext.ldap.base.LDAPCommunicator.modify for details), or
-        #    a dictionary representing the entry or parts of the entry.
-        #    XXX: dicts not yet
-
-        replace
-            if set to True, replace entry at DN entirely with data.
+        :param dn: Modification DN
+        :param data: Either list of 3 tuples (look at
+            ``node.ext.ldap.base.LDAPCommunicator.modify`` for details), or a
+            dictionary representing the entry or parts of the entry.
+            XXX: dicts not yet
+        :param replace: If set to True, replace entry at DN entirely with data.
         """
         self.ensure_connection()
         result = self._communicator.modify(dn, data)

src/node/ext/ldap/tests.py

@@ -55,4 +55,4 @@ def test_suite():
     return suite
 
 if __name__ == '__main__':
-    unittest.main(defaultTest='test_suite')                # pragma NO COVERAGE
+    unittest.main(defaultTest='test_suite')                #pragma NO COVERAGE