More strict parsing of ISO dates #6227

spinscale · 2014-05-19T09:10:02Z

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 · 2014-05-20T09:05:12Z

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 · 2014-05-28T11:05:16Z

@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 · 2014-05-29T18:27:12Z

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 · 2014-05-30T15:32:00Z

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 · 2014-06-17T06:25:07Z

@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 · 2014-06-17T17:41:36Z

LGTM

clintongormley · 2014-07-11T09:42:54Z

@spinscale any news on getting this one merged?

clintongormley · 2014-08-08T09:49:04Z

@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 · 2014-10-20T13:16:17Z

@spinscale giving this one back to you :)

clintongormley · 2014-12-29T13:57:59Z

Also see #5328

spinscale · 2015-05-20T11:49:29Z

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 · 2015-06-08T14:51:23Z

src/test/java/org/elasticsearch/index/mapper/date/SimpleDateMappingTests.java

+                .endObject().endObject().string();
+
+        String indexName = "test-" + randomAsciiOfLength(10).toLowerCase(Locale.ROOT);
+        IndexService index = createIndex(indexName, settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_1_5_2_ID).build());


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

spinscale · 2015-06-25T11:42:04Z

incoporporated your review comments @bleskes

bleskes · 2015-06-29T12:28:41Z

core/src/main/java/org/elasticsearch/common/joda/Joda.java

@@ -171,6 +244,38 @@ public static FormatDateTimeFormatter forPattern(String input, Locale locale) {
        return new FormatDateTimeFormatter(input, formatter.withZone(DateTimeZone.UTC), locale);
    }

+    public static FormatDateTimeFormatter getStrictStandardDateFormater() {


Typo: getStrictStandardDateFormat_t_er

bleskes · 2015-06-29T13:06:49Z

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 · 2015-06-29T19:39:45Z

core/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java

@@ -548,4 +554,13 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
            }
        }
    }
+
+    private static FormatDateTimeFormatter getDateTimeFormatter(Settings indexSettings) {


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

agreed. removed.

rjernst · 2015-06-29T19:44:58Z

I left a couple comments on the mapping portion.

rjernst · 2015-06-30T14:41:28Z

core/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java

            if (Version.indexCreated(context.indexSettings()).before(Version.V_2_0_0)) {
-                boolean includesEpochFormatter = dateTimeFormatter.format().contains("epoch_");
+                if (fieldType().dateTimeFormatter().equals(Defaults.DATE_TIME_FORMATTER)) {


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?

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

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 · 2015-07-02T05:58:41Z

LGTM

spinscale · 2015-07-02T06:29:09Z

@bleskes are you good with the documentation changes?

bleskes · 2015-07-02T10:12:43Z

docs/reference/mapping/date-format.asciidoc

@@ -49,6 +49,13 @@ first millisecond of the rounding scope. The semantics work as follows:
 [[built-in]]
 === Built In Formats

+Most of the above dates have a `strict` companion dates, which means, that


above -> bellow

bleskes · 2015-07-02T10:14:25Z

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

spinscale · 2015-07-06T11:12:30Z

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

bleskes · 2015-07-06T17:47:52Z

LGTM. Thx @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. Same applies for month, day, hour, minute, seconds. Also the new default is `strictDateOptionalTime` for indices created with Elasticsearch 2.0 or newer. In addition a couple of not exposed date formats have been exposed, as they have been mentioned in the documentation. Closes elastic#6158

clintongormley assigned spinscale Jul 11, 2014

clintongormley added the adoptme label Aug 8, 2014

dadoonet unassigned spinscale Aug 8, 2014

brwe added v1.4.0 labels Aug 8, 2014

brwe self-assigned this Aug 8, 2014

clintongormley mentioned this pull request Aug 8, 2014

Add T (on top of :, -, and /) as a way to pre detect of a string is a date for dynamic date formats #1694

Closed

clintongormley added v1.5.0 and removed v1.4.0.Beta1 labels Sep 8, 2014

clintongormley removed the help wanted adoptme label Oct 20, 2014

clintongormley assigned spinscale and unassigned brwe Oct 20, 2014

clintongormley added v2.0.0-beta1 and removed v1.5.0 labels Oct 21, 2014

clintongormley added the :Search/Mapping Index mappings, including merging and defining field types label Nov 11, 2014

drewr force-pushed the master branch from dcc3da0 to 7c20a8a Compare February 20, 2015 16:48

clintongormley mentioned this pull request May 4, 2015

Supporting dots ('.') as separator for dynamic_date_formats #10961

Closed

spinscale mentioned this pull request May 5, 2015

Mapping: Improve date handling #10971

Closed

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from 4c70305 to e852304 Compare May 22, 2015 11:55

bleskes reviewed Jun 8, 2015
View reviewed changes

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from e852304 to ba2054d Compare June 25, 2015 11:41

bleskes reviewed Jun 29, 2015
View reviewed changes

rjernst reviewed Jun 29, 2015
View reviewed changes

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from ba2054d to 392d8bd Compare June 30, 2015 12:19

rjernst reviewed Jun 30, 2015
View reviewed changes

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from 392d8bd to d004f96 Compare July 1, 2015 12:37

bleskes reviewed Jul 2, 2015
View reviewed changes

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from d004f96 to f8faac5 Compare July 6, 2015 11:11

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from f8faac5 to 72f82a0 Compare July 7, 2015 07:34

spinscale force-pushed the issue-6158-reject-wrongly-formatted-years-in-dates branch from 72f82a0 to b612cab Compare July 7, 2015 07:36

spinscale merged commit b612cab into elastic:master Jul 7, 2015

kevinkluge removed the review label Jul 7, 2015

clintongormley added :Search/Mapping Index mappings, including merging and defining field types and removed :Dates labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More strict parsing of ISO dates #6227

More strict parsing of ISO dates #6227

spinscale commented May 19, 2014

spinscale commented May 20, 2014

spinscale commented May 28, 2014

clintongormley commented May 29, 2014

spinscale commented May 30, 2014

spinscale commented Jun 17, 2014

clintongormley commented Jun 17, 2014

clintongormley commented Jul 11, 2014

clintongormley commented Aug 8, 2014

clintongormley commented Oct 20, 2014

clintongormley commented Dec 29, 2014

spinscale commented May 20, 2015

bleskes Jun 8, 2015

spinscale commented Jun 25, 2015

bleskes Jun 29, 2015

bleskes commented Jun 29, 2015

rjernst Jun 29, 2015

spinscale Jun 30, 2015

rjernst commented Jun 29, 2015

rjernst Jun 30, 2015

spinscale Jun 30, 2015

spinscale Jul 1, 2015

rjernst commented Jul 2, 2015

spinscale commented Jul 2, 2015

bleskes Jul 2, 2015

bleskes commented Jul 2, 2015

spinscale commented Jul 6, 2015

bleskes commented Jul 6, 2015

More strict parsing of ISO dates #6227

More strict parsing of ISO dates #6227

Conversation

spinscale commented May 19, 2014

spinscale commented May 20, 2014

spinscale commented May 28, 2014

clintongormley commented May 29, 2014

spinscale commented May 30, 2014

spinscale commented Jun 17, 2014

clintongormley commented Jun 17, 2014

clintongormley commented Jul 11, 2014

clintongormley commented Aug 8, 2014

clintongormley commented Oct 20, 2014

clintongormley commented Dec 29, 2014

spinscale commented May 20, 2015

Choose a reason for hiding this comment

spinscale commented Jun 25, 2015

Choose a reason for hiding this comment

bleskes commented Jun 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jun 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jul 2, 2015

spinscale commented Jul 2, 2015

Choose a reason for hiding this comment

bleskes commented Jul 2, 2015

spinscale commented Jul 6, 2015

bleskes commented Jul 6, 2015