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

[Pavel Zuna]

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