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 <jden...@redhat.com>

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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to