[Freeipa-devel] [PATCH] 93 Add custom mapping object for LDAP entry data

[Jan Cholasta]

On 16.1.2013 19:05, Petr Viktorin wrote:
> On 01/16/2013 04:45 PM, Jan Cholasta wrote:
>> Hi,
>>
>> 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
> work:
>
>      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.

Honza

-- 
Jan Cholasta