[Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names

[Jan Cholasta]

On 4.2.2013 15:49, Petr Viktorin wrote:
> On 02/04/2013 02:25 PM, Jan Cholasta wrote:
>> On 1.2.2013 12:12, Petr Viktorin wrote:
>>> On 01/31/2013 04:18 PM, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> these patches implement attribute name case preservation in LDAPEntry.
>>>> Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
>>>> part 5).
>>>>
>>>> Honza
>>>
>>> Patches 99 & 101 need some tests to make sure the _names work correctly.
>>
>> Added.
>
> I see one of the changes is using has_key instead of `in` for a CIDict.
> Given that dict.has_key() is deprecated, I think a better solution would
> be to add __contains__ to CIDict. Is there a reason against that?

I think a separate patch with this and other changes to make CIDict more 
like dict would be better.

>
>>> Since you can call LDAPEntry.__init__ in different ways which don't
>>> always correspond to the argument names, it would be nice to explain
>>> them in a docstring.
>>
>> Done.
>>
>>>
>>> A few nitpicks below.
>>>
>>>> freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
>>>>
>>>> diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
>>>> index 20c11b4..6268ac0 100644
>>>> @@ -650,14 +637,48 @@ class LDAPEntry(dict):
>>>>           self._orig = self
>>>>           self._orig = deepcopy(self)
>>>>
>>>> +    def _attr_name(self, name):
>>>> +        if not isinstance(name, (unicode, str)):
>>>
>>> Use basestring instead of (unicode, str).
>>
>> Is there any compelling reason to do so? I don't want to support
>> arbitrary basestring subclasses in this code, just unicode and str.
>
> Using basestring is the standard idiom. Since you're supporting
> arbitrary str and unicode subclasses anyway, I don't see your point.
> Besides, basestring can't even be instantiated. Someone who'd subclass
> basestring directly would really have to know what they're doing.

OK. I just don't like basestring I guess. Fixed.

>
>>>> freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
>>>>
>>>> diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
>>>> index 6268ac0..01fa0c1 100644
>>>> @@ -595,8 +600,10 @@ class LDAPEntry(dict):
>>>>                   _obj = {}
>>>>               orig = None
>>>>
>>>> +        assert isinstance(_conn, IPASimpleLDAPObject)
>>>>           assert isinstance(_dn, DN)
>>>>
>>>> +        self._conn = lambda: _conn  # do not deepcopy me!
>>>
>>> This would be better done by overriding __deepcopy__, or just using a
>>> custom method for the copying.
>>>
>>
>> I have tried multiple different solutions and none is as elegant as
>> this. Yes, it is a hack, but since it is an internal implementation
>> detail of LDAPEntry, I don't see any harm in doing it.
>>
>> On further thought, custom method is probably better suited for this job
>> than than deepcopy. Added.
>>
>> Updated patches attached.
>
> Could you also add a docstring to commit()? The function is not clear
> from the name alone.

Done.

>
> Other than that, the patches look good.
>

Updated patches attached.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-98.2-Use-the-dn-attribute-of-LDAPEntry-to-set-get-DNs-of-.patch
Type: text/x-patch
Size: 17429 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130205/8c8496a4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-99.2-Preserve-case-of-attribute-names-in-LDAPEntry.patch
Type: text/x-patch
Size: 9683 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130205/8c8496a4/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-100.2-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
Type: text/x-patch
Size: 5561 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130205/8c8496a4/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-101.2-Support-attributes-with-multiple-names-in-LDAPEntry.patch
Type: text/x-patch
Size: 3335 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130205/8c8496a4/attachment-0003.bin>