[Freeipa-devel] Re: [PATCH] Add conversion of attribute name synonyms when generating modlists.

[Rob Crittenden]

Pavel Zuna wrote:
> Rob Crittenden wrote:
>> Pavel Zůna wrote:
>>> Rob Crittenden wrote:
>>>> Pavel Zůna wrote:
>>>>> Rob Crittenden wrote:
>>>>>> Pavel Zuna wrote:
>>>>>>> Patch 0003: Add conversion of attribute name synonyms when 
>>>>>>> generating modlists.
>>>>>>>
>>>>>>> Fixes the bug with 'localityname' in host plugins and prevents 
>>>>>>> similar bugs from happening.
>>>>>>>
>>>>>>> Pavel
>>>>>>
>>>>>> I'm not sure I understand this patch. What is the purpose of 
>>>>>> preferred_names? It looks that if this is set the the translation 
>>>>>> doesn't actually happen (and this is always set).
>>>>>>
>>>>>> rob
>>>>>
>>>>> If preferred_names evaluate to True (if not None and not and empty 
>>>>> list), then all attribute names equivalent to those in preferred 
>>>>> names are converted to them. Otherwise all attributes are converted 
>>>>> to their preferred synonym according to the schema.
>>>>>
>>>>> For example:
>>>>>
>>>>> a = {'locality': 'Brno'}
>>>>> ldap.convert_attr_synonyms(a, ['localityname'])
>>>>> # a == {'localityname': 'Brno'}
>>>>> ldap.convert_attr_synonyms(a)
>>>>> # a == {'l': 'Brno'}
>>>>>
>>>>> I did it this way, so when generating a modlist, we can convert 
>>>>> attributes as returned by python-ldap to what the plugin uses.
>>>>>
>>>>> Pavel
>>>>>
>>>>
>>>> Ok, this is where I got confused. You don't actually make that 
>>>> second call (without the preferred_names) anywhere in the patch.
>>>>
>>>> I'm not sure we need to do the preferred name though, we'll still 
>>>> end up with extra MOD operations this way. Take an example:
>>>>
>>>> I add a host:
>>>>
>>>> % ipa host-add --locality=Brno h1.example.com
>>>>
>>>> When the record is added 389 is going to change the attribute to 'l'.
>>>>
>>>>   dn: fqdn=h1.example.com,cn=computers,cn=accounts,dc=example,dc=com
>>>>   cn: h1.example.com
>>>>   fqdn: h1.example.com
>>>>   l: Brno
>>>>   <snip>
>>>>
>>>> The machine moves so I modify the entry:
>>>>
>>>> % ipa host-mod --locality=Berlin h1.example.com
>>>>
>>>> Your first call will leave the attribute as localityname and it will 
>>>> try to compare it to 'l', right? So it is still going to add a new 
>>>> value.
>>>>
>>>> rob
>>>>
>>>
>>> If converting entry attributes received from *-mod commands, then 
>>> yes. But I took another approach to it. When generating a modlist, I 
>>> first retrieve the original entry attributes and convert their names 
>>> this way:
>>>
>>> ldap.convert_attr_synonyms(original_entry_attrs, new_entry_attrs.keys())
>>>
>>> It doesn't matter if I convert the original entry attributes or the 
>>> new attributes before making the comparison, but the later is a bit 
>>> faster, because there's always more attributes in the original entry 
>>> (resulting in more iterations when performing the conversion).
>>>
>>> Oh, and it only works for plugins2, because it all happens inside ldap2.
>>>
>>> Pavel
>>
>> Ok, that makes sense but you didn't include this call in the patch. Do 
>> you need to make an updated patch?
>>
>> rob
>>
> I think I did, it's hidden in comments. ;-)
> 
>  47 @@ -477,6 +503,8 @@ class ldap2(CrudBackend, Encoder):
>  48          # we could call search_s directly, but this saves a lot of 
> code at
>  49          # the expense of a little bit of performace
>  50          entry_attrs_old = self.encode(entry_attrs_old)
>  51 +        # we also need to make sure that attribute names match
>  52 +        self.convert_attr_synonyms(entry_attrs_old, 
> entry_attrs.keys())
>  53          # generate modlist, we don't want any MOD_REPLACE operations
>  54          # to handle simultaneous updates better
>  55          modlist = []
> 
> Pavel

No, I saw that but didn't quite grok what it was doing, I was assuming 
you'd do the reverse, change the incoming data to match the server. As 
you say, it doesn't really matter which one we choose.

Can you beef up the comment to say what you said above, that you are 
converting the existing entry attribute names to match the current names 
for the purposes of generating a modlist?

ack and pushed to master.

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090615/f6d8c03e/attachment.bin>