On 07/07/2012 08:45 PM, John Dennis wrote:
The DN work I was doing on master is ready for review and testing. It's
been a long haul and I've been working relentlessly to get this work
completed. I am on PTO for a week starting today (I know bad timing) but
I spent yesterday and my first day of PTO today writing up extensive
documentation for the work so others can start taking a look at it while
I'm gone. The documentation as well as where to find the code can be
found here:


The document is long but I felt it was better to provide explanations
for as much as possible.

I may check in during the week but I'm going to try and discipline
myself not to and take an actual much needed break.


I went over the changes to ipaserver/plugins/ldap2.py; now I can start the testing.

I must ask about the wrapped _ext methods. I assume python-ldap exposes them only to mimic the C interface, which has them because C doesn't have default arguments. Would we lose anything if the class only defined the _ext methods, and aliased the non-ext variants to them? e.g.

    def add_ext(self, dn, modlist, serverctrls=None, clientctrls=None):
        assert isinstance(dn, DN)
        dn = str(dn)
        modlist = self.encode(modlist)
        return self.conn.add_ext(dn, modlist, serverctrls, clientctrls)
    add = add_ext

The non-_ext variants are deprecated in the underlying C library, anyway.

Why are some of the wrapper methods left out? (compare, delete_ext, modify, etc.)

Line 1519:
    checkmembers = copy.deepcopy(members)
    for member_dn in checkmembers:
        if m not in checkmembers:
checkmembers.append(m) # FIXME jrd; can't modify list during traversal
NACK. You really can't modify lists during traversal, a FIXME won't cut it.
A smaller issue is that the list `in` operator does alinear scan, so calling it in a loop can very slow as the list grows. Sets are much better for membership tests.
So I believe you want something like:

    checkmembers = set(DN(m, locked=True) for m in members)
    checked = set()
    while checkmembers:
        member_dn = checkmembers.pop()
        if m not in checked:

Lines 203, 825:
    os.environ['KRB5CCNAME'] = ccache_file
Should KRB5CCNAME be unset/restored once the code's done with it.

Line 228:
        except IndexError:
The try clause is too long for such a general exception, this can hide real errors.


And as usual I have a lot of nitpicks. Sometimes I just want to share my ideas about how good code should look like, don't take them too seriously if you think otherwise :)

Line 107 & 127:
    return utf8_codec.decode(val)[0]
    return utf8_codec.encode(unicode(val))[0]
I believe the following would be more readable:
    return val.decode('utf-8')
    return unicode(val).encode('utf-8')

Line 134:
    class ServerSchema(object):

Don't use nested classes without reason

Line 148:
    def get_schema(self, url, conn=None, force_update=False):

Is the force_update & flush() useful? None of the code seems to use it.

Line 354:
We have a case-insensitive dict class, why not use it here?

Lines 388, 750:
    def is_dn_syntax(self, attr):
        Check the schema to see if the attribute uses DN syntax.
uses_dn_syntax or has_dn_syntax would be a better name.

Line 395:
        if syntax is None:
            return False
        return syntax == DN_SYNTAX_OID
The if is redundant.

Line 406:
        if isinstance(val, bool):
            if val:
                val = 'TRUE'
                val = 'FALSE'
            return self.encode(val)
Encoding an ASCII string should be a no-op, why bother doing it?

Line 421:
    dct = dict()
    for (k, v) in val.iteritems():
        dct[self.encode(k)] = self.encode(v)
    return dct
dct = dict((self.encode(k), self.encode(v)) for k, v in val.iteritems())

Line 437:
if type(target_type) is type and isinstance(original_value, target_type): Don't use `type(x) is expected_type` for type checking! Use isinstance(target_type, type) so you also catch type's subclasses. Or better yet, handle the TypeError that isinstance will raise if you don't give it a class. To be honest, I don't think it's worth it to do all this convoluted checking just to make sure you're not copying unnecessarily. Do you have a reason I can't see?

Line 594, 604, 613:
        ipa_result = self.convert_result(ldap_result)
        return ipa_result
Just return the result directly, no need for the extra variable.

Line 876, 894, 1041:
    parent_dn -- DN of the parent entry (default '')
The default is None. In any case it's redundant to specify default values in docstrings, as you can just look at the function itself.

Line 1422:
    self.handle_errors(e, **{})
Wouldn't the following work better?
    self.handle_errors(e, *[{}]*0)

Line 1518:
    checkmembers = copy.deepcopy(members)
deepcopy() is rarely what you want; it either copies too much or too little. The Python developers made a mistake by assigning a short name to such an ill-specified operation. It's better to do this explicitly.
    checkmembers = [DN(m) for m in members]
Or to combine with my previous suggestion,
    checkmembers = set(DN(m, locked=True) for m in members)

Line 1609:
    indirect = memberof[:]  # make a copy of the list
I believe the list() is more readable ­— doesn't need a comment:
    indirect = list(memberof)


Freeipa-devel mailing list

Reply via email to