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

Jan Cholasta jcholast at redhat.com
Mon Oct 14 08:59:40 UTC 2013 

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

Updated patches attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-106.4-Make-LDAPEntry-a-wrapper-around-dict-rather-than-a-d.patch
Type: text/x-patch
Size: 8565 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-107.4-Introduce-IPASimpleLDAPObject.decode-method-for-deco.patch
Type: text/x-patch
Size: 3810 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-108.4-Always-use-lists-for-values-in-LDAPEntry-internally.patch
Type: text/x-patch
Size: 3810 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-109.4-Decode-and-encode-attribute-values-in-LDAPEntry-on-d.patch
Type: text/x-patch
Size: 16766 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-110.4-Make-sure-attributeTypes-updates-are-done-before-obj.patch
Type: text/x-patch
Size: 1088 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-111.4-Remove-legacy-toDict-and-origDataDict-methods-of-LDA.patch
Type: text/x-patch
Size: 4878 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-112.4-Store-encoded-attribute-values-from-search-results-d.patch
Type: text/x-patch
Size: 886 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-113.4-Use-encoded-values-from-entry-objects-directly-when-.patch
Type: text/x-patch
Size: 3253 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-121.4-Use-encoded-values-from-entry-objects-directly-when-.patch
Type: text/x-patch
Size: 1478 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-169.4-Turn-LDAPEntry.single_value-into-a-dictionary-like-p.patch
Type: text/x-patch
Size: 49606 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131014/a1b00f99/attachment-0009.bin>

More information about the Freeipa-devel mailing list