[Freeipa-devel] DN patch and documentation

[Rich Megginson]

On 08/09/2012 01:31 PM, John Dennis wrote:
> 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.

hmm - did you see this comment at the top of the file?

# Policy related schema.
# This file should not be loaded.
# Remove this comment and assign right OIDs when time comes to do something
# about this functionality.



>
>>
>> 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.
>
>
>