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

Tue Apr 22 11:32:21 UTC 2014

On 03/05/2014 01:08 PM, Jan Cholasta wrote:
> 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.
>

Thank you for the suggestions. Updated, rebased patch is attached.

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0137-5-ipalib-Add-DateTime-parameter.patch
Type: text/x-patch
Size: 10643 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140422/583a1dad/attachment.bin>