Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime.parse should throw an error on invalid date... #11189

Closed
DartBot opened this issue Jun 10, 2013 · 23 comments
Closed

DateTime.parse should throw an error on invalid date... #11189

DartBot opened this issue Jun 10, 2013 · 23 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 library-core P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DartBot
Copy link

DartBot commented Jun 10, 2013

This issue was originally filed by adrian.avil...@gmail.com


What steps will reproduce the problem?

  1. Put this line and run: DateTime pDate = DateTime.parse("20130547");
  2. It will return: 2013-06-16 00:00:00.000
    3.

What is the expected output? What do you see instead?
Since the say is 47 it should throw an exception.

What version of the product are you using? On what operating system?
Dart Editor version 0.5.13_r23552
Dart SDK version 0.5.13.1_r23552
Windows 8 64bits.

@kasperl
Copy link

kasperl commented Jun 11, 2013

Added Area-Library, Triaged labels.

@floitschG
Copy link
Contributor

For the DateTime constructor we allow values outside the valid range.

I'm not yet convinced that we shouldn't do the same for the DateTime parser, but I can get convinced otherwise.

@DartBot
Copy link
Author

DartBot commented Oct 23, 2013

This comment was originally written by adrian.avil...@gmail.com


"For the DateTime constructor we allow values outside the valid range."

Why allow invalid ranges, whay not validate them?

@floitschG
Copy link
Contributor

Turns out that it is actually very convenient. Initially we didn't allow them, but backtracked.

@DartBot
Copy link
Author

DartBot commented Oct 23, 2013

This comment was originally written by adrian.avila.mt...@gmail.com


Convenient? in what way, I was bit by this bug and I didn't see any convenience?

@floitschG
Copy link
Contributor

Issue #12514 has been merged into this issue.

@floitschG
Copy link
Contributor

Issue #2771 was the one where we decided to allow over and underflows.

@lrhn
Copy link
Member

lrhn commented Dec 17, 2013

The convenience is that you can do:
  var today = new DateTime.now();
  var inThirtyDays = new DateTime(today.year, today.month, today.date + 30);

I don't think we should allow it in DateTime.parse.


Added Accepted label.

@lrhn
Copy link
Member

lrhn commented Jan 28, 2014

Removed Priority-Unassigned label.
Added Priority-Low label.

@lrhn
Copy link
Member

lrhn commented Apr 24, 2014

Added Library-Core label.

@DartBot DartBot added Type-Defect P3 A lower priority bug or feature request area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core labels Apr 24, 2014
@lrhn lrhn changed the title DateTime.parse should thorow an error on invalid date... DateTime.parse should throw an error on invalid date... Jun 4, 2015
@kevmoo kevmoo added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed accepted labels Feb 29, 2016
@lrhn lrhn added core-m and removed core-m labels Aug 11, 2017
@floitschG floitschG added core-2 and removed core-m labels Aug 29, 2017
@hoylen
Copy link
Contributor

hoylen commented Jul 3, 2018

+1

Having the DateTime parse method throw an exception for components which are outside the valid range is useful for parsing external input. For example, when reading a value from a file, JSON or user input. In those cases, invalid components are probably a sign of an error in the input.

If DateTime.parse didn't perform such checking, then everyone writing robust code will have to reinvent-the-wheel and write their own parsing code to check the components. It would be much better if the standard parse method implemented the checking (especially for programs which don't bother implementing their own checking and blindly assume input-is-never-wrong). The standard parse method should implement the checking once and implement it correctly. For example, if programmers are expected to write their own date checking code, then most implementations would probably check if the day-of-month is [1-31], but not many would check if 29 is valid for February in that particular year.

Allowing overflows might be "convenient" for internal date manipulations in code you wrote (though I would argue the "correct" way to do that is to use Durations), but it is not safe for parsing input you didn't create.

If you want to keep the overflow behaviour, have an optional named parameter to allow/disallow overflow. The default should be to prohibit them, since that is a safer mode; but unfortunately that would break existing code that expects overflows to work. So maybe the default is to allow overflow.

I would also like to see some optional named parameters to control how much of the DateTime must be in the string for it to be valid. For example, when reading in a timestamp from a log file, you want the parser to fail if only the date (year, month, day) and hour is provided (since a good timestamp should at least specify it down to the minute or second). Also whether the timezone is mandatory or optional, since if omitted it would be ambiguous when reading in a file (especially, one created in a different timezone); and if optional being able to provide a timezone to interpret it in (since the file being read might have been created in a timezone different from the current local timezone).

Without such checking in DateTime.parse, developers are either forced to re-implement their own parsing and checking code; or end up creating fragile programs that could accept unexpected/wrong input.

@lrhn
Copy link
Member

lrhn commented Jul 3, 2018

I agree that DateTime.parse should throw on invalid inputs, where invalid inputs are anything that isn't a valid date in a recognized format.
The reasoning is exactly what has been written here: There is no simpler way to validate input than to parse it, so if the parsing doesn't catch invalid input, nobody does.

The other DateTime constructors, then ones taking integers as input, should accept the input they are given, and be generous in the interpretation. That's actually useful, and it's simple to check if the input is valid (create the DateTime, then check that the year, month and date still match the constructor arguments, not something you can easily do with string inputs).

@alan-knight
Copy link
Contributor

There are some interesting edge cases with that kind of validation. e.g. validating February 18, 2018 by passing in integers when in Brazil.

@hoylen
Copy link
Contributor

hoylen commented Jul 4, 2018

alan-knight writes:

interesting edge cases... February 18, 2018 by passing in integers when in Brazil

I assume you are talking about the general issue of dealing with daylight saving changes.

Daylight saving is a separate and complex issue in itself, so let's not try and solve it as a part of this issue - which is only about how DateTime.parse validates strings.

The DateTime.parse method currently does not allow the string to specify whether daylight saving is in use or not. The timezone component of the string (when present) only allows explicit offsets in hours and minutes from UTC, or "Z" - so there is no ambiguity there. That is, 30 minutes before the changeover the BRST time would be "2018-02-17 11:30-02:00" and 30 minutes after the changeover would be the BRT time of "2018-02-17 11:30-03:00" (i.e. the two times are 01:30 UTC and 02:30 UTC). Without the timezone, the string will be parsed differently depending on where/when the program was run (i.e. what the local timezone was at runtime). This is an example of why it is important to have the option of making the parser reject strings which don't have an explicit timezone in it.

lrhn writes:

I agree that DateTime.parse should throw on invalid inputs...

The other DateTime constructors, then ones taking integers as input, should accept the input they are given, and be generous in the interpretation

Good reasoning. Creating a DateTime by parsing a string should behave differently from the constructors from integer components: they are different operations used in different situations.

@lrhn
Copy link
Member

lrhn commented Jul 18, 2019

@woozbooz
Copy link

woozbooz commented Sep 9, 2019

Hello, why this bug still not fixed?

@janjoosse
Copy link

@kevmoo
Copy link
Member

kevmoo commented Mar 17, 2020

Could we entertain adding a strict: true named argument or something here @lrhn ?

@lrhn
Copy link
Member

lrhn commented Mar 23, 2020

I worry about making this strict for local time.

The validation would be simple. Parse numbers, create a date, read numbers back out. If the numbers you read are not the same a the ones you put in, the original wasn't valid.
However, there'll probably be some edge cases around local time and daylight saving that will trip up users sporadically (but obviously never around tests).

So, perhaps we should instead check that the values are in proper ranges:

  • year: -something big to something big.
  • month: 1..12
  • day: 1..end of month
  • hours: 0..23
  • minutes: 0..59
  • seconds: 0..59
  • fractional seconds no limit, we truncate if necessary.
    and then only check that the date was correct. Any correction in hours due to DST will stay inside the same date.

@alan-knight
Copy link
Contributor

You might want to look at the Intl logic which jumps through a bunch of hoops for these kind of edge cases. I particularly note that
Any correction in hours due to DST will stay inside the same date.
is not true. In Brazil the DST transition happens at midnight (facepalm). So if the transition is going backwards, it changes the date.

@alan-knight
Copy link
Contributor

There's a fun document, might be internal to Google, about things that programmers believe about internationalization that aren't true.

@rocaforarnelo
Copy link

Use parseStrict. Can anyone close this?

@kevmoo
Copy link
Member

kevmoo commented Dec 20, 2020

@kevmoo kevmoo closed this as completed Dec 20, 2020
dart-bot pushed a commit that referenced this issue Jun 28, 2021
The [DateTime.parse](https://api.dart.dev/stable/2.10.4/dart-core/DateTime/parse.html) method accepts out-of-range components. For example, parsing the string "2020-01-42" produces "2020-02-11".

This was raised as a bug in #11189, but after more than 7 years that issue was closed with the solution being "use parseStrict from the intl package" instead.

This pull request simply **updates the documentation of DateTime's _parse_ method** so that users know:

1. This is how the method behaves. Otherwise they might be surprised when it unexpectedly happens in their code.
2. This behaviour is a feature and not a bug. Otherwise they might raise another issue for it.
3. The _parseStrict_ method in the intl package exists. Otherwise they might reinvent the wheel.

Closes #44521
#44521

GitOrigin-RevId: a88ada4
Change-Id: Ic45e4db118bd3fad3a612659a35ca7a5d80937b4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176680
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
Commit-Queue: Lasse R.H. Nielsen <lrn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. core-2 library-core P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

10 participants