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

[Jan Cholasta]

On 26.6.2014 09:40, Petr Viktorin wrote:
> 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,

It is not, according to Generalized Time syntax (RFC 4517 section 
3.3.13) "0" is an invalid value and we should treat it the same way as 
any other invalid value (hence my original suggestion).

> 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?

It does. When attribute is empty/None, it is unset, not missing. There 
is old code which depends on this.

> 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.
>
>

Yes, but until the old code is fixed, None has a special meaning and 
can't be used for anything else.

-- 
Jan Cholasta