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

[Petr Viktorin]

On 01/17/2013 09:07 AM, Jan Cholasta wrote:
> 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.

I don't think we have any classes where it doesn't apply.

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

Yes, eventually. Though I'd like it to raise an exception for some time, 
so any external plugins that rely on it fail rather than get bad data.

>> 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']?

Nothing really requires it, of course, but quite a lot of code currently 
uses it.

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

I also like entry.dn, at least in most cases, but if we don't 
synchronize them then we need to remove entry['dn'] completely. Having 
misleading/stale data in the object isn't good.
There are some cases where entry['dn'] makes sense, such as iterating 
over all attributes.
If we're going to have validation for every attribute anyway, I don't 
think synchronizing the two would be a major problem. Especially since 
entry.dn is already a property.

-- 
Petr³