[Freeipa-devel] [PATCH 0081] Skip referrals when converting LDAP result to LDAPEntry

[Martin Kosek]

On 07/26/2013 01:19 PM, Tomas Babej wrote:
> On Friday 26 of July 2013 13:03:25 Martin Kosek wrote:
>> On 07/26/2013 12:23 PM, Tomas Babej wrote:
>>> On Friday 26 of July 2013 12:16:42 Jan Cholasta wrote:
>>>> On 26.7.2013 11:29, Tomas Babej wrote:
>>>>> After some investigation I decided the correct approach here is to
>>>>> scream at the debug level only, when referral is being ignored.
>>>>>
>>>>> We cannot guide ourselves by the ldap.OPT_REFFERALS option of the underlying
>>>>> connection simply because even if referral chasing is turned on (and therefore
>>>>> we should not get any referrals from python-ldap, since they should have been
>>>>> resolved), queries for AD can return referrals (AD returns them often as a way to
>>>>> provide additional information AFAIU). This can also happen if we are not able
>>>>> to authenticate to the referred server, or resolve the LDAP uri.
>>>>>
>>>>> In case ignoring referrals ever breaks something, we can find the information
>>>>> in the log at the debug level. Doing otherwise would be unnecessarily spamming
>>>>> the log now.
>>>>>
>>>>> Updated patch attached.
>>>>
>>>> Nitpick: I would prefer a shorter message without unnecessary 
>>>> implementation details - something like "Ignoring referral entry {ref}". 
>>>> Also use str(original_attrs) as ref.
>>>>
>>>> Honza
>>>>
>>>> -- 
>>>> Jan Cholasta
>>>
>>> Agreed, fixed.
>>>
>>> Tomas
>>>
>>
>> Looks OK. Another nitpick: why do you use formatting with .format()? It makes
>> the formatting inconsistent with rest of the code base.
>>
> 
> Simply because python documentation recommends using .format():
> 
> http://docs.python.org/3.4/library/stdtypes.html#printf-style-string-formatting
> 
> At some point it was even considered that % would be removed:
> 
> http://docs.python.org/release/3.1.5/library/stdtypes.html#old-string-formatting-operations
> 
>> I would prefer:
>>
>> +                log_msg = 'Referral entry ignored: %s' % str(original_attrs)
>>
>>> Martin
> 
> I have no strong opinions about this. Do we want to impose a restriction on using the new
> string formatters available in Python since 2.5?
> 
> Tomas
> 

We do not need to impose a restriction. I was just thinking about this - I have
no strong opinions about it either. Also discussed with Petr3, he is fine with
using format function.

So ACK. Pushed to master.

Martin