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:
What is the significance of L here:
'2.16.840.1.113730.3.8.L.8' : DN, # ipaTemplateRef
These came from:
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
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
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
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 <jden...@redhat.com>
Looking to carve out IT costs?
Freeipa-devel mailing list