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

[Jan Cholasta]

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.
>
> ipapython/ipaldap.py:847:
>   warning: 1 line adds whitespace errors.
>
>           if isinstance(_obj, LDAPEntry):
> +            data = dict(_obj._data)
>               orig = _obj._orig
>
> Is this necessary? `self.update(_obj)` is done later.

Probably not. But it's removed in patch 109.

>
>
>       def __contains__(self, name):
> -        return self._names.has_key(self._attr_name(name))
> +        return self._names.has_key(name)
>
> has_key() is deprecated for dict, it would make sense to prefer `name in
> self._names` for CIDict too.

Sure, this line is from before CIDict got __contains__.

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

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

>
>
> 107. Introduce IPASimpleLDAPObject.decode method for decoding LDAP values.
>
> Can you put in a docstring?

OK.

>
>
>
> 108. Always use lists for values in LDAPEntry internally.
>
> @@ -698,6 +701,7 @@ class LDAPEntry(collections.MutableMapping):
>
>           result._names = deepcopy(self._names)
>           result._data = deepcopy(self._data)
> +        result._not_list = deepcopy(self._not_list)
>           if self._orig is not self:
>               result._orig = self._orig.clone()
>
> It's better to use set() than deepcopy() for a set of strings.

Right.

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

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

-- 
Jan Cholasta