[Freeipa-devel] DN patch and documentation

[Petr Viktorin]

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:
>
> http://jdennis.fedorapeople.org/dn_summary.html
>
> 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.
>
> John
>

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()
         checked.add(member_dn)
         ...
         if m not in checked:
             checkmembers.add(m)



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:
     _SCHEMA_OVERRIDE = {
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'
             else:
                 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
Consider:
     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)

-- 
Petr³