[Freeipa-devel] [PATCHES] Add new set of base classes for IPA plugins using LDAP.

Tue Jun 9 02:07:40 UTC 2009

Pavel, 

Sorry I was a bit slow to review this.  Comments inline...

On Thu, 2009-06-04 at 20:17 +0200, Pavel Zuna wrote:
> Dear freeipa-devel,
> I know we agreed that we're going to post one patch per email, but it this case 
> I think it's justified. Patches 0001 through 0005 are prerequisites for the new 
> base classes. Before describing the individual patches, let me explain a little 
> bit of what I had in mind when implementing this addition.
> 
> For the last few months, I've been mostly working the new LDAP backend and IPA 
> plugins that use it (i.e. almost all IPA plugins). I tried to make the new 
> backend more powerful and flexible. Unfortunately, to take full advantage of the 
> new potential, old plugins had to be rewritten. As there were several 
> inconsistencies across them (output, naming and sometimes even behavior) and 
> about half of them were still using old features from IPAv1, I thought that some 
> re-factoring couldn't hurt after all.
> 
> After porting a few plugins to the new backend, I realized that most of them 
> were doing pretty much the same thing - manipulating LDAP entries. The only 
> difference was that some plugins were generating attributes in specific ways 
> based on user input. So, I figured that most of the generic work could be done 
> in one place and that's when I started working on these new base classes.
> 
> Let's take a quick look at them:
> 
> LDAPObject
>   - extends frontend.Object
>   - represents a type of LDAP entry
>     (or sub-entry if a parent_key=True parameter is present)
> 
> The following methods are to be used with LDAPObject, they are named after the 
> CRUD methods they extend with an 'LDAP' prefix. They take 1 argument (the 
> primary key) in the case their LDAPObject is an entry and 2 arguments (the 
> parent and primary key, in that order) if their LDAPObject is a subentry:
> 
> LDAPCreate
> LDAPRetrieve
> LDAPUpdate
> LDAPDelete
> 
> Then there's LDAPSearch, which is a bit special. It takes only the optional 
> criteria argument if its LDAPObject is an entry. If its LDAPObject is a 
> sub-entry then it requires the parent key as the first argument. The search 
> logic works as follows:
> 
> 1) If there is no parameters (except for the parent key in the case of 
> sub-entries), all entries of the given type are returned.
> 
> 2) If there is only the criteria parameter, all entries of the given type with 
> one or more of their default attributes containing the value of criteria are 
> returned.
> 
> 3) If there is an options specified, all entries of the given type with the 
> corresponding attribute EQUAL to the value specified are returned. If more than 
> one options is specified this way, only entries matching ALL the conditions are 
> returned.
> 
> 2) and 3) can be combined, so for example, we can make a search for entries who 
> have a specific attribute equal to 'foo' and some of their other attributes 
> containing 'bar'.
> 
> All of these base classes implement a 'callback' method, that is called just 
> before invoking python-ldap through the backend. Subclasses can use it to 
> execute their specific code after all the boring stuff has been done for them.
> 
> In the future I'm planning to write more base classes for other uses cases such 
> as adding/removing values from multivalued attributes.
> 
> 
> Patches descriptions:
> ---------------------
> 
> 0001: Make it possible for subclasses of Command and co. to use their own 
> __public__.
> 
> The problem was with PluginProxy only taking the base class' __public__ into 
> consideration.

ack.  I'm actually planning to remove the use of PluginProxy, which
would be another way to solve this problem.  But you already wrote a fix
that works for you and I want this to move forward without git
monkey-business slowing us down, so I'll later submit a patch to remove
PluginProxy related stuff throughout.  I personally find an "always move
forward" style of DVCS use to be much more productive.

Anyway, despite the fact that PluginProxy is going bye-bye soon, I still
want to by convention stay true to some of the things it was designed to
enforce, which I'll explain a bit:

The reason __public__ on the base class was used is so that a plugin's
interface to the outside world is consistent in each namespace (Command,
Object, etc.).  Otherwise an outside consumer of ipalib (say the web UI
or a hypothetical PyGTK UI) might need specific knowledge of which
sub-class a, say, Command plugin is based on, which defeats the purpose
of the whole architecture.  The one exception is api.Backend, which
doesn't get wrapped in a PluginProxy as it just doesn't make sense
there.

But there are two interfaces in question: 1) the interface to the
outside world; and 2) the interface between plugins (i.e., in your case
api.Command.foo calling methods on api.Object.bar to do work).  The
first should be used consistently so we can keep our components simple
and agnostic, but it's fine for the second to have variability (i.e.,
it's fine for api.Command.user_add to employ methods on api.Object.user
that aren't in the Object base class public API).

Anyway, hopefully it didn't take too long to make the __public__ changes
as they wouldn't be needed if I already removed PluginProxy.  :(  

We should think of a better way to keep each other in the know about
core-related changes we are working on or are in need of.

I know Rob already acked the rest and I don't have any objections to
anything.

> 0002: Add parent_key to keyword arguments of Param base class.
> 
> 
> 0003: Add support for parent key in Object base class.
> 
> 
> 0004: Make CRUD base classes take parent key parameters into consideration when
> generating args.
> 
> 
> 0005: Add delete_tree method to ldap2 allowing recursive deletes of whole LDAP 
> branches.
> 
> Some plugins were doing this "manually", so I figured it might as well be 
> provided by the LDAP API.
> 
> 
> 0006: Add new set of base classes for plugins using LDAP.
> 
> 
> To see the new bases classes in action, take a look at the next patch I'm about 
> to send to freeipa-devel.
> 
> Pavel
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel