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. > ack
Pushed to master (additional information in a thread for Alexander's patch 0037). Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
