On Thu, 2011-09-29 at 15:06 -0600, Rich Megginson wrote:
> On 09/28/2011 01:27 PM, John Dennis wrote:
> > We use convenience types (classes) in IPA which make working with LDAP
> > easier and more robust. It would be really nice if the basic python-ldap
> > library understood our utility types and could accept them as parameters
> > to the basic ldap functions and/or the basic ldap functions returned our
> > utility types.
> > Normally such a requirement would trivially be handled in an object-
> > oriented language (which Python is) by subclassing to extend and modify
> > the functionality. For some reason we didn't do this with the python-ldap
> > classes.
> > python-ldap objects are primarily used in two different places in our
> > code, ipaserver.ipaldap.py for the IPAdmin class and in
> > ipaserver/plugins/ldap2.py for the ldap2 class's .conn member.
> > In IPAdmin we use a IPA utility class called Entry to make it easier to
> > use the results returned by LDAP. The IPAdmin class is derived from
> > python-ldap.SimpleLDAPObject. But for some reason when we added the
> > support for the use of the Entry class in SimpleLDAPObject we didn't
> > subclass SimpleLDAPObject and extend it for use with the Entry class as
> > would be the normal expected methodology in an object-oriented language,
> > rather we used an obscure feature of the Python language to override all
> > methods of the SimpleLDAPObject class by wrapping those class methods in
> > another function call. The reason why this isn't a good approach is:
> > * It violates object-oriented methodology.
> > * Other classes cannot be derived and inherit the customization (because
> > the method wrapping occurs in a class instance, not within the class
> > type).
> > * It's non-obvious and obscure
> > * It's inefficient.
> > Here is a summary of what the code was doing:
> > It iterated over every member of the SimpleLDAPObject class and if it was
> > callable it wrapped the method. The wrapper function tested the name of
> > the method being wrapped, if it was one of a handful of methods we wanted
> > to customize we modified a parameter and called the original method. If
> > the method wasn't of interest to use we still wrapped the method.
> > It was inefficient because every non-customized method (the majority)
> > executed a function call for the wrapper, the wrapper during run-time used
> > logic to determine if the method was being overridden and then called the
> > original method. So every call to ldap was doing extra function calls and
> > logic processing which for the majority of cases produced nothing useful
> > (and was non-obvious from brief code reading some methods were being
> > overridden).
> > Object-orientated languages have support built in for calling the right
> > method for a given class object that do not involve extra function call
> > overhead to realize customized class behaviour. Also when programmers look
> > for customized class behaviour they look for derived classes. They might
> > also want to utilize the customized class as the base class for their use.
> > Also the wrapper logic was fragile, it did things like: if the method name
> > begins with "add" I'll unconditionally modify the first and second
> > argument. It would be some much cleaner if the "add", "add_s", etc.
> > methods were overridden in a subclass where the logic could be seen and
> > where it would apply to only the explicit functions and parameters being
> > overridden.
> > Also we would really benefit if there were classes which could be used as
> > a base class which had specific ldap customization.
> > At the moment our ldap customization needs are:
> > 1) Support DN objects being passed to ldap operations
> > 2) Support Entry & Entity objects being passed into and returned from
> > ldap operations.
> > We want to subclass the ldap SimpleLDAPObject class, that is the base
> > ldap class with all the ldap methods we're using. IPASimpleLDAPObject
> > class would subclass SimpleLDAPObject class which knows about DN
> > objects (and possilby other IPA specific types that are universally
> > used in IPA). Then IPAEntrySimpleLDAPObject would subclass
> > IPASimpleLDAPObject which knows about Entry objects.
> > The reason for the suggested class hierarchy is because DN objects will be
> > used whenever we talk to LDAP (in the future we may want to add other IPA
> > specific classes which will always be used). We don't add Entry support to
> > the the IPASimpleLDAPObject class because Entry objects are (currently)
> > only used in IPAdmin.
> > What this patch does is:
> > * Introduce IPASimpleLDAPObject derived from
> > SimpleLDAPObject. IPASimpleLDAPObject is DN object aware.
> > * Introduce IPAEntryLDAPObject derived from
> > IPASimpleLDAPObject. IPAEntryLDAPObject is Entry object aware.
> > * Derive IPAdmin from IPAEntryLDAPObject and remove the funky method
> > wrapping from IPAdmin.
> > * Code which called add_s() with an Entry or Entity object now calls
> > addEntry(). addEntry() always existed, it just wasn't always
> > used. add_s() had been modified to accept Entry or Entity object
> > (why didn't we just call addEntry()?). The add*() ldap routine in
> > IPAEntryLDAPObject have been subclassed to accept Entry and Entity
> > objects, but that should proably be removed in the future and just
> > use addEntry().
> > * Replace the call to ldap.initialize() in ldap2.create_connection()
> > with a class constructor for IPASimpleLDAPObject. The
> > ldap.initialize() is a convenience function in python-ldap, but it
> > always returns a SimpleLDAPObject created via the SimpleLDAPObject
> > constructor, thus ldap.initialize() did not allow subclassing, yet
> > has no particular ease-of-use advantage thus we better off using the
> > obvious class constructor mechanism.
> > * Fix the use of _handle_errors(), it's not necessary to construct an
> > empty dict to pass to it.
> > If we follow the standard class derivation pattern for ldap we can make us
> > of our own ldap utilities in a far easier, cleaner and more efficient
> > manner.
Pushed to master (additional information in a thread for Alexander's
Freeipa-devel mailing list