On 16.1.2013 19:05, Petr Viktorin wrote:
On 01/16/2013 04:45 PM, Jan Cholasta wrote:

this patch adds initial support for custom LDAP entry objects, as
described in <http://freeipa.org/page/V3/LDAP_code>.

The LDAPEntry class implements the mapping object. The current version
works like a dict, plus it has a DN property and validates and
normalizes attribute names (there is no case preservation yet).

The LDAPEntryCompat class provides compatibility with old code that uses
(dn, attrs) tuples. The reason why this is a separate class is that our
code currently has 2 contradicting requirements on the entry object:
tuple unpacking must work with it (i.e. iter(entry) yields dn and
attribute dict), but it also must work as an argument to dict
constructor (i.e. iter(entry) yields attribute names). This class will
be removed once our code is converted to use LDAPEntry.

With this patch, ldap2 produces LDAPEntryCompat objects. I don't see how
that brings us closer to converting the codebase to LDAPEntry.
I'd be happier if this patch made both the tuple unpacking and
dict-style access possible. But perhaps you have a better plan than I
can see from the patch?

My plan was to gradually transform these classes into a single LDAPEntry class in a series of patches.

The dict constructor uses the `keys` method, not iteration, so these two
requirements aren't incompatible -- unless we do specifically require
that iter(entry) yields attribute names. For example the following will

     class NotQuiteADict(dict):
         def __iter__(self):
             return iter(('some', 'data'))

     a = NotQuiteADict(a=1, b=2, c=3)
     print dict(a)  # -> {'a': 1, 'c': 3, 'b': 2}
     print list(a)  # -> ['some', 'data']
     print a.keys()  # -> ['a', 'c', 'b']

While this works for dict, I'm not sure if it applies to *all* dict-like classes that we use.

While overriding a dict's __iter__ in this way is a bit evil, I think
for our purposes it would be better than introducing a fourth entry class.

Well, both are evil :-)

When all our code is converted we could just have __iter__ warn or raise
an exception for a few releases to make sure no one uses it.

Once we completely get rid of entry tuples, we can reintroduce a dict-like __iter__. IMO new code should not be punished (i.e. forced to use .keys()) for the crimes (i.e. tuples) of the old code.

In a couple of places we do iterate over the entry to get the keys, but
not in many, and they're usually well-tested. We just have to use
`entry.keys()` there.

Would these changes to your patch be OK?

Well, I wanted to keep code changes in this patch to LDAPEntry itself, but it's OK I guess...

Another issue is that the DN is itself an attribute, so entry.dn and
entry['dn'] should be synchronized. I guess that's material for another
patch, though.

Is there any code that actually requires entry['dn']?

We should decided whether we want to use entry['dn'] or entry.dn and use only that in new code. I like entry.dn more, as it better corresponds to the special meaning of dn in entries.


Jan Cholasta

Freeipa-devel mailing list

Reply via email to