[Freeipa-devel] DN patch and documentation

[John Dennis]

On 08/09/2012 02:08 PM, Rob Crittenden wrote:
> I've been going through the diffs and have some questions in ldap2.py,
> these are primarily pedantic:

Some of your questions can be answered by:

http://jdennis.fedorapeople.org/dn_summary.html

>
> What is the significance of L here:
>
> '2.16.840.1.113730.3.8.L.8'      : DN,  # ipaTemplateRef

These came from:

install/share/60policyv2.ldif

I didn't notice the .L in the OID which isn't legal (correct?). So beats 
me, I can't explain why the OID is specified that way.

>
> There are quite a few assert isinstance(dn, DN). I assume this is mainly
> meant for developers, we aren't expecting to handle that gracefully at
> runtime?

Correct. They are there to prevent future use of dn strings by 
developers whose habits die hard. The goal is 100% DN usage 100% of the 
time.

If we allow strings some of the time we're on a slippery slope. Think of 
it as type enforcement meant to protect ourselves.

The assertions also proved valuable in finding a number of places where 
functions failed to return values or correct values. This showed up a 
lot in the pre and post callbacks whose signature specifies a return 
value but the developer forgot to return a value. Apparently pylint does 
not pick these things up.

In production we should disable assertions, we should open a ticket to 
disable assertions in production.

>
> It seems to me that allowing a DN passed in as a string would be nice in
> some cases. Calling bind, for example, looks very awkward and isn't as
> readable as cn=directory manager, IMHO. What is the downside of having a
> converter for these?

See the above documentation for the rationale. In particular the section 
called "Why did I use tuples instead of strings when initializing DN's?"

> search_ext() has an extra embedded line which makes the function a bit
> confusing to read.

O.K. you lost me on that one :-)

> Do we need to keep _debug_log_ldap?

I don't think it hurts. I found it very useful to see the actual LDAP 
data when debugging.

> In search_ext_s() (and a few others) should we just return
> self.convert_result() directly?

Petr asked this question too. I don't have strong feelings either way. 
The reason it's stored in another variable is it's easy to add a print 
statement or stop in the debugger and examine it when need be. It also 
makes it clear it's the IPA formatted results. I don't think there is 
much cost to using the variable. I'm not attached to it, it can be changed.
>
> In ldap2::__init__ what would raise an AttributeError and would we want
> to hide that fact with an empty base_dn? Is this attempting to allow the
> use of ldap2.ldap2 in cases where the api is not initialized?

Beats me, that's been in the code for a long time. An empty DN is the 
same as an empty string. Maybe we should set it to None instead so we 
know base_dn was never initialized properly.

> In ipaserver/ipaldap.py there is a continuance of the assertions, some
> of which don't seem to be needed. For example, in toDict() it shouldn't
> be possible to create an Entry object without having self.dn be a DN
> object, so is this assertion necessary?

Many objects now enforce DN's for their dn attribute. A dn attribute may 
only ever be None or an instance of a DN. This is implemented with

dn = ipautil.dn_attribute_property('_dn')

In objects which define their dn property this way an assert 
isinstance(self.dn, DN) is not necessary because the dn property 
enforces it. So you're correct, those particular asserts could be 
removed. They were added before dn type enforcement via object property 
was added. I could go through and clean those up, but perhaps we should 
open a separate ticket for that.



-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/