More strict parsing of ISO dates

[spinscale]

If you are using the default date or the named identifiers of dates,
the current implementation was allowed to read a year with only one
digit. In order to make this more strict, this fixes a year to be at
least 4 digits.

Closes #6158

[spinscale]

talked with @clintongormley - makes sense to not replace those dates, but create new ones with a 'strict' prefix (like strictDateOptionalTime), which forces the year to have at least 4 digits, month 2 and day 2 digits as well. And this will be the default for named dates and automatic recognition

[spinscale]

@clintongormley updated the PR, maybe you can take another look

Changes:

• Have a strict date format for each existing, where it made sense, which always requires you to have two digit months, days, weeks, hours, minutes and seconds and at least 4 digit years.
• Made the strictOptionalDateTime the default
• Added a couple of missing formats, which existed in the docs though (and vice versa)
• Added a lot of tests to make sure the parsing is actually strict

If this is ok, I will need to update documentation, I will add the missing formats and their format (added this info in the test so far), and mention the strict ones. And see, if we can reuse some of the code in order to not duplicate the DateFormat class.

[clintongormley]

hi @spinscale

Almost there, but there are two default formats used for detection. one is now strict ,but the other is not: https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java#L45

This means that this illegal date is still mapped:

PUT /t/t/1
{
  "date": "5/11/10"
}

GET /_mapping

[spinscale]

Updated the rootobjectmapper to have a more strict date. Talked with @clintongormley to possible have strict mapping for self formatted dates as well, like yyyy, from what I read so far, this might be tricky, but I need to dig deeper into joda for this.

[spinscale]

@clintongormley ok, so having a parsing format like yyyy does not seem possible at the moment... are still good to go with the rest? If so, I would add documentation and maybe try to cut out some code if possible

[clintongormley]

LGTM

[clintongormley]

@spinscale any news on getting this one merged?

[clintongormley]

@brwe When this PR is in, then we can probably implement #1694 as well, as 1T will no longer be accepted as a date.

[clintongormley]

@spinscale giving this one back to you :)

[clintongormley]

Also see #5328

[spinscale]

I dug a bit around in joda time and still do not see any other pattern than to copy the whole class and use the fixed date builders. Another approach would have been to take supplied date string and compare it with the date being created by the date formatter - if it doesnt match, then the formatting has been different. However this flaw has the problem, that we need to parse the date twice and it becomes really complex, when we use the || for allowing several dates in one field, as each format then needs to be tried out. This also has a performance penalty during index time.

[bleskes]

it sucks we can do this, but it helps here :)

[spinscale]

incoporporated your review comments @bleskes

[bleskes]

Typo: getStrictStandardDateFormat_t_er

[bleskes]

I went through and left some comments. Looks nice. I think we also need to document this on the 2.0 migration doc. Also would love someone who is more at home with the mapping code to have a look /cc @rjernst

[rjernst]

I think this is cleaner to have inline? it is very short and not used anywhere else.

[spinscale]

agreed. removed.

[rjernst]

I left a couple comments on the mapping portion.

[rjernst]

Doesn't this have the same problem? If the user upgrades to 2.0, and tries to set their older index to use the new defaults, they will be forced back to non strict?

[spinscale]

yes, you are right. A bug in the code didnt let my test fail accordingly... will rethink.
Thx for pointing out!

[spinscale]

added a new commit with a test fixing this. I just check if the the parser actually parsed a new format and only in case if not, the defaults are applied

[rjernst]

LGTM

[spinscale]

@bleskes are you good with the documentation changes?

[bleskes]

above -> bellow

[bleskes]

left one minor comment. I think we also need an entry in migrate_2_0.asciidoc ?

[spinscale]

fixed the typo and added a small paragraph to the upgrade asciidoc

[bleskes]

LGTM. Thx @spinscale