Skip to content

datetime.fromisoformat incorrectly accepts and parses strings without date-time separator #107779

Open
Open
datetime.fromisoformat incorrectly accepts and parses strings without date-time separator#107779
Labels
stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

Description

@joaoe
joaoe
opened on Aug 8, 2023

Bug report

Checklist

  • I am confident this is a bug in CPython, not a bug in a third-party project
  • I have searched the CPython issue tracker, and am confident this bug has not been reported before

A clear and concise description of the bug

This is a regression in Python 3.11 from 3.10

try:   
  print(datetime.datetime.fromisoformat('20230808120000Z'))
  assert 0, "failed"
except ValueError:
  print("PASS")

Since Python 3.11, datetime.fromisoformat invokes _find_isoformat_datetime_separator which tries to find where the separator is between the date and time, e.g. "2023-08-08 12:00:00" which is in position 10.

For strings without a date element separator (the dash) the function just outright returns 8. For the example above, that results in the string being parsed as datetime(2023, 8, 8, 2, 0, 0). This is incorrect.

Suggestion to fix:

  1. First alternative: the _find_isoformat_datetime_separator must always validate that the returned index must not be an ascii digit, that for all cases and branches inside the function.
  2. Second alternative: datetime.fromisoformat must check if separator_location points to an ascii digit or not, and if not complain about invalid date

Your environment

  • CPython versions tested on: 3.11.4
  • Operating system and architecture: Windows

Linked PRs

Activity

  1. joaoe
    added
    type-bugAn unexpected behavior, bug, or error
    on Aug 8, 2023
  2. AlexWaygood
    added this to Date and time issues 🕰️on Aug 8, 2023
  3. joaoe
    added a commit that references this issue on Aug 8, 2023

    pythongh-107779: in datetime.fromisoformat, check the separator betwe…

    ae18b32

  4. pganssle commented on Aug 9, 2023

    @pganssle
    pganssle
    on Aug 9, 2023
    Member

    We cannot forbid the separator from being a digit, we just need to validate that the string is a valid datetime assuming the digit is a separator.

    I remember noticing this a few weeks back and finding the source of the problem, but I forget if I ever documented it. IIRC it was in the C code that parses time strings, in the case where the time strings don't have : separators some error code fails to trigger. I vaguely recall that the code was originally designed to handle (valid ISO8601) formats with fractional hours or minutes, but we decided that adherence to the spec wasn't worth it since no one uses those formats. I seem to recall thinking that if we removed that code we would solve this problem.

  5. joaoe commented on Aug 9, 2023

    @joaoe
    joaoe
    on Aug 9, 2023
    Author

    We cannot forbid the separator from being a digit

    Hi.

    I'm trying to find information that supports that statement or that any character or no character can be used as separators.

    Wikipedia tells

    A single point in time can be represented by concatenating a complete date expression, the letter "T" as a delimiter, and a valid time expression. For example, "2007-04-05T14:30". In ISO 8601:2004 it was permitted to omit the "T" character by mutual agreement as in "200704051430",[36] but this provision was removed in ISO 8601-1:2019. Separating date and time parts with other characters such as space is not allowed in ISO 8601, but allowed in its profile RFC 3339.[37]

    And following the citation links does not add that much info.

    But from my understanding, any deviation from the T character or space (if supporting rfc3339) is an deviation of the spec that needs to be agreed upon by parties exchanging the data. I'm not sure that is supposed to be the use case for datetime.fromisoformat() where it tries to guess if there is a non-standard separator.

    The first patch I uploaded to the PR demanded a non-digit separator but then I saw test cases that expected no separator and even one test case with this data

                ('2025-01-02003:04:05,6+00:00:00.00',
             self.theclass(2025, 1, 2, 3, 4, 5, 600000, tzinfo=UTC)),

    which I found quite remarkable. But those tests all point to that legacy you are describing.
    I'm not entirely sure how to go from here, since the current behavior is asserted by a test.

    PS: I'd be happy if fromisoformat was stricter.

  6. pganssle commented on Aug 9, 2023

    @pganssle
    pganssle
    on Aug 9, 2023
    Member

    The reason is that fromisoformat is primarily intended to be the functional inverse of isoformat, so it needs to be able to parse anything that isoformat can output. dt.isoformat(sep="9") would be an odd choice, but it is a valid one.

    The expansion to cover a larger fraction of the ISO8601 spec is more recent.

    Even if we decided that it's beneficial to restrict this particular subset of valid isoformat outputs, I don't think it's worth breaking backwards compatibility over.

  7. joaoe commented on Aug 16, 2023

    @joaoe
    joaoe
    on Aug 16, 2023
    Author

    @pganssle

    I don't think it's worth breaking backwards compatibility over.

    The current situation of alowing a number as separator is nowhere in the iso spec and is a can of worms. It should be removed, there is no valid use case to keep it, and as seen a source of bugs.

    Might I suggest then to deprecate digit separators, say until python 3.13, and then in 3.14 raise a ValueError ?

  8. miccoli commented on Aug 23, 2023

    @miccoli
    miccoli
    on Aug 23, 2023

    The reason is that fromisoformat is primarily intended to be the functional inverse of isoformat, so it needs to be able to parse anything that isoformat can output. dt.isoformat(sep="9") would be an odd choice, but it is a valid one.

    This argument holds only partially because datetime.datetime.isoformat will only output extended expressions:

    >>> datetime.datetime.fromisoformat('20230808120000Z').isoformat(sep='1')
'2023-08-08120:00:00+00:00'

    there is no way to output the basic expression '20230808120000Z' In other words: currently the codomain of datetime.datetime.isoformat is a proper subset of the domain of datetime.datetime.fromisoformat.

    So what should be done with expressions like '20230808120000Z'?

    ISO 8601 reference

    I've checked the ISO 8601‐1:2019+A1:2022 docs, which say at clause 5.4.1:

    A time can be expressed by combining a date expression — a calendar date (5.2.2), ordinal date (5.2.3) or week dates (5.2.4) — with a time of day expression.
    The date is followed (without space) by [“T”] followed (without space) by the time, optionally including the time shift designator. The date part of a date and time expression shall be complete. The time may be incomplete (see 5.3.1.3).

    Note that in the ISO documents

    • datetime is refererred to as a time
    • date is a date
    • time is time of day

    So there is no doubt that with strict adherence to ISO 8601‐1:2019 both '20230808120000Z' and '2023-08-08120:00:00+00:00' are invalid time expressions.

    Please not that according to ISO 8601 the letter "T" is not a separator but the time designator:

    “T” the time designator, which indicates

    • the start of the representation of local time of day to designate local time of day expressions as such;
    • the start of the representation of the time of day in date and time of day expressions;
    • the start of the repr

    Some thoughts, which I think most will accept.

    • A strict ISO 8601‐1:2019 parser should error out on both '20230808120000Z' and '2023-08-08120:00:00+00:00'
    • With 99.99% ceratainity, by '20230808120000Z' the user/programmer meant '2023-08-08T12:00:00Z' and not '2023-08-08T20:00:00Z' using '1' as the time designator.
    • Strict ISO is ugly: '2023-08-08T12:00:00+00:00'
    • Using space or underscore as time designator is more legible: '2023-08-08 12:00:00+00:00', '2023-08-08_12:00:00+00:00'
    • Using arbitrary chars is
      • crazy '2023-08-08⏳12:00:00+00:00' or
      • confusing '2023-08-08112:00:00+00:00'

    If I got it right, the 3.11 datetime.datetime.fromiso implementation

    • extends the ISO standard allowing arbitrary unicode code points as time designator
    • is only a partial implementation, since some valid ISO time expression (say '1985102T232030+04') are not parsed.

    My opinion is that

    • allowing for arbitrary code points in the sep argument is way to broad, but
    • fuzzing the parser, allowing for widly used but not well defined pseudo ISO expressions, or pre ISO 8601‐1:2019 semantics is higly undesiderable.

    Finally my suggestion is to simply keep the status quo.

    However, in order to reduce the surprise factor when parsing strings like '20230808120000Z', a deprecation cycle could be started to eventually have

    • a mandatory time designator from an extended but small set: {T, , _}
    • explicilty disallow a missing time designator.

  9. joaoe commented on Aug 25, 2023

    @joaoe
    joaoe
    on Aug 25, 2023
    Author

    Hi @miccoli , thank you for your thorough analysis

    a deprecation cycle could be started to eventually have ...

    I completly agree. This generic separator feature should be removed.

  10. iritkatriel
    added
    stdlibStandard Library Python modules in the Lib/ directory
    on Nov 26, 2023

  11. cfbolz commented on Nov 28, 2023

    @cfbolz
    cfbolz
    on Nov 28, 2023
    Contributor

    An additional problem is that the Python implementation and the C implementation of datetime also disagree about this issue. Here's the behaviour if I block the import of _datetime on 3.11:

    >>> print(datetime.datetime.fromisoformat('20230808120000Z'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/cfbolz/projects/cpython/Lib/datetime.py", line 1846, in fromisoformat
    raise ValueError(
ValueError: Invalid isoformat string: '20230808120000Z'

    PyPy just got a (related?) bug report about using 0 as the time separator (we are using the pure python datetime code from CPython):

    https://foss.heptapod.net/pypy/pypy/-/issues/4039

    The discrepancy there is that this works in the pure python code, but fails with C _datetime:

    >>> import datetime
>>> datetime.datetime.fromisoformat("2023-01-300")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    datetime.datetime.fromisoformat("2023-01-300")
ValueError: Invalid isoformat string: '2023-01-300'
>>> import _pydatetime as datetime
>>> datetime.datetime.fromisoformat("2023-01-300")
datetime.datetime(2023, 1, 30, 0, 0)

    (this is on git head, but the same difference exists in 3.11)

  12. StanFromIreland commented on Mar 15, 2025

    @StanFromIreland
    StanFromIreland
    on Mar 15, 2025
    Member

    Just a minor reminder that the [T] separator is not strictly required in ISO 8601.

    By mutual agreement of the partners in information interchange, the character [T] may be omitted in applications where there is no risk of confusing a date and time of day representation with others defined in this International Standard

  13. miccoli commented on Mar 15, 2025

    @miccoli
    miccoli
    on Mar 15, 2025

    Just a minor reminder that the [T] separator is not strictly required in ISO 8601.

    By mutual agreement of the partners in information interchange, the character [T] may be omitted in applications where there is no risk of confusing a date and time of day representation with others defined in this International Standard

    This provision was present in ISO 8601:2004 but has been dropped in ISO 8601:2019.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions