[Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry

[Petr Viktorin]

On 10/14/2013 10:59 AM, Jan Cholasta wrote:
> On 10.10.2013 09:45, Jan Cholasta wrote:
>> On 9.10.2013 13:57, Petr Viktorin wrote:
>>> On 09/26/2013 02:22 PM, Jan Cholasta wrote:
>>>> On 24.9.2013 15:35, Jan Cholasta wrote:
>>>>> On 27.2.2013 16:31, Jan Cholasta wrote:
>>>>>> Hi,
>>>>>>
>>>>>> these patches add the ability to access and manipulate raw attribute
>>>>>> values as they are returned from python-ldap to the LDAPEntry class.
>>>>>> This is useful for comparing entries, computing modlists for the
>>>>>> modify
>>>>>> operation, deleting values that don't match the syntax of an
>>>>>> attribute,
>>>>>> etc., because we don't need to care what syntax does a particular
>>>>>> attribute use, or what Python type is used for it in the framework
>>>>>> (raw
>>>>>> values are always list of str). It should also make interaction with
>>>>>> non-389 DS LDAP servers easier in the framework.
>>>>>>
>>>>>> (It might be too late for this kind of changes to get into 3.2 now,
>>>>>> I'm
>>>>>> posting these patches mainly so that you are aware that they exist.)
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>> This is now planned for 3.4:
>>>>> <https://fedorahosted.org/freeipa/ticket/3521>
>>>>>
>>>>> I fixed some issues in these patches and refined the API. Updated
>>>>> patches attached.
>>>>>
>>>>> Also added a patch to use raw values when adding new entries and a
>>>>> patch
>>>>> which refines LDAPEntry.single_value, so that it is consistent with
>>>>> the
>>>>> rest of the changes introduced in the patches.
>>>>>
>>>>> Patch 110 will probably be dropped in favor of Petr Viktorin's schema
>>>>> update patches, but I included it anyway.
>>>>>
>>>>> Incidentally, this also fixes
>>>>> <https://fedorahosted.org/freeipa/ticket/3927> and possibly also
>>>>> <https://fedorahosted.org/freeipa/ticket/2131>.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> Noticed a couple more issues and fixed them. Updated patches attached.
>>>>
>>>> Honza
>>>
>>> Thanks for the patches!
>>>
>>>
>>> 106. Make LDAPEntry a wrapper around dict rather than a dict subclass.

Git rants about one more whitespace error.

[...]
>>> +    def __eq__(self, other):
>>> +        if not isinstance(other, LDAPEntry):
>>> +            return NotImplemented
>>> +        if self._dn != other._dn:
>>> +            return False
>>> +        return super(LDAPEntry, self).__eq__(other)
>>>
>>> I don't think equality checking makes sense on a LDAPEntry, where you
>>> might have different capitalizations/variants of attribute names,
>>> different _orig, or a different set of attributes loaded on the same
>>> entry. It's not obvious which of those differences should make the
>>> entries inequal.
>>> I'd just base it on identity (`self is other`).
>>
>> Right, I'm not sure why I even did it this way. But I remember seeing
>> some code that did comparison of entries with ==...

Thanks.
Please also implement __ne__() when reimplementing __eq__().

>>
>>>
>>>       def __iter__(self):
>>>           yield self._dn
>>>           yield self
>>>
>>> This makes the whole thing sort of hackish -- we need to reimplement
>>> everything in MutableMapping that uses iter() internally :(
>>> Hopefully we can get rid of it soon.
>>
>> Yes, it's a shame MutableMapping uses iter() instead of iterkeys().
>>
>>> I'd welcome FIXME comments on whatever is reimplemented for this reason.
>>
>> I thought the comment above __iter__ would be enough. Apparently I was
>> wrong.

Right, IMO it's not immediately obvious that these are reimplemented 
because they depend on __iter__.

[...]
>>> 109. Decode and encode attribute values in LDAPEntry on demand.
>>>
>>> The syncing looks rather over-engineered to me.
>>> Did you consider a custom MutableSequence for the values?
>>> I think it would be much cleaner in the end than merging two sets of
>>> changes together.
>>
>> I'm not entirely happy about it either, but it works. I did consider a
>> custom sequence type, but I didn't feel like it was the right time to
>> this kind of change in this patchset.
>>
>> Unlike the (DN, dict) -> LDAPEntry
>> transition, this change won't be backward compatible and there is a lot
>> of isinstance(value, list) and entry[attr] = list() kind of things in
>> the framework code.

That's what I was afraid of.

We could live with `isinstance(value, list)`; hopefully we could get rid 
of `type(value) == list` that is the real problem.
With `entry[attr] = list()` we could convert automatically.

But OK, let's settle for a worse solution in the meantime.


To be frank I don't particularly like the LDAPEntryView.
While the dict-like interface is great, there isn't a case for storing a 
Raw view long-term, i.e. you'd always want to do something like
     values = entry.raw[x]
     ...
     entry.raw[x] = new_values
rather than
     raw = entry.raw
     values = raw[x]
     ...
     raw[x] = new_values
The latter is confusing because LDAPEntryView and RawLDAPEntryView are 
two classes that have exactly the same interface, but do something 
different. In a duck-typed world that's a recipe for disaster.
I think it would be better if the view implemented just the dict 
protocol, and not `conn`, `dn`, `nice`, `raw`, etc.
The code would also be much simpler without the elaborate view class 
hierarchy.

If you don't agree then at least don't make `raw` available on raw views 
and `nice` on nice views; the programmer *always* needs to know which 
version they're working with so these aren't necessary.

>>> I think it would also help (in the future?) to make the value lists more
>>> set-like, since the order doesn't matter.
>>
>> +1
>>
>> Honza
>>
>
> Updated patches attached.
>

110.
It can't hurt to have this in for now.

111 - 121 look great!

169.
For reasons I said before I'd prefer if single_value stayed a simple 
function.


-- 
Petr³