[Freeipa-devel] [PATCH 0137] ipalib: Add DateTime parameter

[Jan Cholasta]

On 25.2.2014 11:15, Tomas Babej wrote:
>
> On 01/14/2014 10:19 AM, Petr Viktorin wrote:
>> On 01/14/2014 09:27 AM, Jan Cholasta wrote:
>>> On 13.1.2014 14:57, Petr Vobornik wrote:
>>>> On 13.1.2014 13:41, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 10.1.2014 21:21, Nathaniel McCallum wrote:
>>>>>> On Thu, 2014-01-09 at 16:30 +0100, Tomas Babej wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Adds a parameter that represents a DateTime format using
>>>>>>> datetime.datetime
>>>>>>> object from python's native datetime library.
>>>>>>>
>>>>>>> In the CLI, accepts one of the following formats:
>>>>>>> Accepts subset of values defined by ISO 8601:
>>>>>>> %Y-%m-%dT%H:%M:%S
>>>>>>> %Y-%m-%dT%H:%M
>>>>>>> '%Y%m%dT%H:%M:%S'
>>>>>>> '%Y%m%dT%H:%M'
>>>>>>>
>>>>>>> Also accepts LDAP Generalized time in the following format:
>>>>>>> '%Y%m%d%H%M%SZ'
>>>>>>>
>>>>>>> As a simplification, it does not deal with timezone info and ISO
>>>>>>> 8601
>>>>>>> values with timezone info (+-hhmm) are rejected. Values are expected
>>>>>>> to be in the UTC timezone.
>>>>>>>
>>>>>>> Values are saved to LDAP as LDAP Generalized time values in the
>>>>>>> format
>>>>>>> '%Y%m%d%H%SZ' (no time fractions and UTC timezone is assumed). To
>>>>>>> avoid
>>>>>>> confusion, in addition to subset of ISO 8601 values, the LDAP
>>>>>>> generalized
>>>>>>> time in the format '%Y%m%d%H%M%SZ' is also accepted as an input (as
>>>>>>> this
>>>>>>> is the
>>>>>>> format user will see on the output).
>>>>>>>
>>>>>>> Part of: https://fedorahosted.org/freeipa/ticket/3306
>>>>>>
>>>>>> The date/time syntax formats are not compliant with ISO 8601. You
>>>>>> stated
>>>>>> they are expected to be in UTC timezone, but no 'Z' is expected in
>>>>>> most
>>>>>> of them. This is not only non-standard, but would prevent you from
>>>>>> every
>>>>>> supporting local time in the future.
>>>>>
>>>>> +1, please require the trailing "Z".
>>>>>
>>>>>
>>>>> I think generalized time format should be the preferred one, as it is
>>>>> stored in LDAP and thus returned by commands. Also, do we really need
>>>>> all of these other formats?
>>>>
>>>> +1 That API should accept and always return generalized time.
>>>>
>>>> But UIs(CLI, Web) should display something more human readable. Like:
>>>> %Y-%m-%dT%H:%M:%SZ or IMHO better (but not ISO 8601): %Y-%m-%d
>>>> %H:%M:%SZ
>>>> ie. 2014-03-08 14:16:55Z compared to 2014-03-08T14:16:55Z or
>>>> 20140308141655Z
>>>>
>>>> Both should accept input value in the same format. Usage of different
>>>> output and input formats is confusing. Thus I agree with supporting
>>>> more
>>>> input formats. Decision whether it should be done on API level or
>>>> client
>>>> level is another matter.
>>>
>>> If you want human readable output, you have to do the conversion from
>>> generalized time on clients. If you want to support local time and
>>> timezones, you have to do some processing on the client as well. I would
>>> be perfectly happy if we supported only generalized time over the wire
>>> and did conversion from/to human readable on clients.
>>>
>>>>
>>>> I has been implementing the Web UI part and I think we should also
>>>> support a formats without time component. My initial impl. supports
>>>> combinations of:
>>>>
>>>>      var dates = [
>>>>          ['YYYY-MM-DD', /^(\d{4})-(\d{2})-(\d{2})$/],
>>>>          ['YYYYMMDD',/^(\d{4})(\d{2})(\d{2})$/]
>>>>      ];
>>>>
>>>>      var times = [
>>>>          ['HH:mm:ss', /^(\d\d):(\d\d):(\d\d)Z$/],
>>>>          ['HH:mm', /^(\d\d):(\d\d)Z$/],
>>>>          ['HH', /^(\d\d)Z$/]
>>>>      ];
>>>> + generalized time
>>>> ['YYYYMMDDHHmmssZ',/^(\d{4})(\d{2})(\d{2})(\d{2})(\d{2})(\d{2})Z$/]
>>>>
>>>> with /( |T)/ as divider and time optional.
>>>>
>>>> Both UIs should also tell the user what is the expected format (at
>>>> least
>>>> one of them). Getting incorrect format error without knowing it is
>>>> annoying.
>>>
>>> The current code raises a ValidationError including the list of formats.
>>
>> On yesterday's meeting, Simo said that on the wire and for CLI output,
>> we should use generalized time.
>> I disagree with using it for CLI ouptut, as I find generalized time
>> unreadable, but for the API it's the obvious choice.
>>
>> I believe we should require the "Z" postfix in all formats, so that 1)
>> users are forced to be aware that it's UTC, and 2) we can
>> theoretically support local time in the future.
>>
>> I think the supported input formats should be:
>>
>> %Y%m%d%H%M%SZ      (generalized time)
>> %Y-%m-%dT%H:%M:%SZ (ISO 8601, with seconds)
>> %Y-%m-%dT%H:%MZ    (ISO 8601, without seconds)
>> %Y-%m-%dZ          (ISO 8601, date only)
>>
>> I also think we should accept a space instead of the "T", as Petr²
>> said above.
>>
>
> Thanks for review, everybody.
>
> * All accepted formats now require explicit Z
> * Accept values with ' ' instead of T
> * LDAP generalized time used on the wire with JSON
> * Minor implementation fixes
>
> Updated patch should address all the issues.

The first if statement is not necessary here, datetime values are 
correctly handled in the super class:

+    def _convert_scalar(self, value, index=None):
+        if isinstance(value, datetime.datetime):
+            return value
+        elif isinstance(value, basestring):


Please use ', ' instead of just ',' as the separator to make the error 
message more readable here:

+            error = (_("does not match any of accepted formats: ") +
+                      (','.join(self.accepted_formats)))


This if statement is not present on old clients, so the assert below 
will fail on them when they receive DateTime:

+    if isinstance(value, DateTime):
+        return datetime.datetime.strptime(str(value), "%Y%m%dT%H:%M:%S")
      assert type(value) in (unicode, int, float, bool, NoneType)

But, they will never receive DateTime from us, because LDAP generalized 
time is decoded to unicode in ipaldap.

What I think we should do is decode LDAP generalized time to datetime 
objects in ipaldap, add a new capability (e.g. "datetime_values") and 
use DateTime only for clients with that capability, otherwise use the 
generalized time string. Also, something similar should be done for 
JSON, so that clients can infer that a value is datetime and not just a 
generic string (e.g. wrap it in a dict with '__datetime__', similar to 
how binary values are wrapped with '__base64__'), again only for clients 
with the capability.

-- 
Jan Cholasta