[Freeipa-devel] [PATCH 0236] ipaldap: Fallback to string if datetime conversion went wrong

[Petr Viktorin]

On 06/26/2014 09:33 AM, Jan Cholasta wrote:
> On 26.6.2014 09:21, Petr Viktorin wrote:
>> On 06/26/2014 08:30 AM, Jan Cholasta wrote:
>>> On 25.6.2014 18:25, Petr Viktorin wrote:
>>>> On 06/25/2014 05:29 PM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 25.6.2014 17:17, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Our datetime conversion does not support full LDAP Generalized
>>>>>> time syntax. In the unsupported cases, we should fall back
>>>>>> to string representation of the attribute.
>>>>>>
>>>>>> In particular, '0' is used to denote no value of LDAP generalized
>>>>>> time attribute.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4350
>>>>>
>>>>> NACK, this beats the purpose of decoding of the values, because it
>>>>> requires you to check the type of the value before using it.
>>>>>
>>>>> Instead, you should either fix the code that uses the
>>>>> nsds5ReplicaLastUpdate{Start,End} attributes to access their raw value
>>>>> directly, or exclude the attributes from decoding to datetime by
>>>>> overriding their type in IPASimpleLDAPObject._SYNTAX_OVERRIDE.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> I agree that just returning a string when conversion fails is the wrong
>>>> thing to do.
>>>>
>>>> I think that if LDAP generalized can contain the empty/invalid
>>>> value, we
>>>> should convert the '0' to what Python uses for that -- None.
>>>>
>>>
>>> But None already means "empty attribute".
>>
>> I don't think you can get [None] currently, can you? None is the default
>> default for get(), but I don't think that's a problem.
>>
>
> You can't, but you can get it from single_value when the attribute is
> empty or assign it to an attribute to make it empty.
>
> I would very much prefer if we avoided None, because it will only create
> mess and possibly some hard to catch errors.
>

The problem is that "unset" is a valid value here, and None is the 
Python representation for that. If it has to make single_value 
ambiguous, I'd be fine with that -- single_value is just a helper.

But, shouldn't single_value[x] raise KeyError if the attribute is missing?
As for setting, there is a difference between
     entry.single_value[x] = None
and
     del entry[x]
and we should use the correct one for each case.


-- 
Petr³