support only string format for date, root object & date range #28117

nikoncode · 2018-01-06T15:28:22Z

Limit date format attribute to String values only (to avoid serialization issues)
Closes #23650

elasticmachine · 2018-01-06T15:28:23Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-01-06T15:28:23Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-04-24T08:53:44Z

Pinging @elastic/es-search-aggs

cbuescher · 2018-07-24T19:24:23Z

@nikoncode sorry for the delay on this, I just found this in unanswered PR while scanning issues. I will update the branch by merging in master if you don't mind and will take a first look then.

Conflicts: core/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java core/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java

cbuescher

@nikoncode thanks again for opening this, I think the PR looks quite good already but I left a suggestion for a change. Would you be interested in making this change? I understand that after waiting for quite some time you might not be interested in working on this any longer. Let me know if you want to pick it up or would like somebody else to work on it. I think its a good change to get in.

cbuescher · 2018-07-24T20:13:26Z

server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java

@@ -265,7 +265,10 @@ private static IndexOptions nodeIndexOptionValue(final Object propNode) {
    }

    public static FormatDateTimeFormatter parseDateTimeFormatter(Object node) {
-        return Joda.forPattern(node.toString());
+        if (node instanceof String) {


Instead of instance testing here I would suggest changing the method to only take a String argument (the format) and change all call sites to convert to String instead. I should be save to do so since we to it here anyway currently.
That way we also don't need to throw an exception for bad formats, Jodas "forPattern" will do so already.

nikoncode · 2018-07-28T18:33:46Z

@cbuescher I'll handle it and mention you again to review.

This reverts commit 4d41cf8.

nikoncode · 2018-07-29T15:10:11Z

@cbuescher I don't think that your suggestion is relevant.

Problem of the original issue is using String.toString() method on List to convert input into string value. As you know, this method produce comma separated values wrapped in square brackets (for example [el1]) and Jodas parser successfully parse this format and threat square brackets as part of user input. We need to differentiate cases when we have List separately from String cases. To do it I have added additional validation condition. Just to implement validation in single place.

If I will implement your recommendation:

Instead of generic validation (1 place) I will need to put the same condition to filter out this case before any usage (from 1 to 3 places).
I have two option how to convert parameter value:
- String.toString() - just doesn't solve the problem;
- (String) format - solves problem but throws ClassCastException instead of meaningful message, we can add 3 more catches to produce good message (see point one);
parseDateTimeFormatter method is part of TypeParsers class - the responsibility of this class is to extract useful objects from untyped values (and validate if required). Just look on any other method in this class, all of them get Object as input parameter and then going through validation + conversion chain returns useful value as the result.

cbuescher · 2018-08-06T20:40:04Z

Problem of the original issue is using String.toString() method on List to convert input into string value.

I see. I was trying to avoid adding the instanceof check and thought that simply using "toString()" at the call site would work. But I agree this is a bit awkward on Lists, although they would still be rejected by Joda then due to the contained square brackets, for example. But the exception messages are not very descriptive in that case, which is why you convinced me that your solution is better. I will run CI now and merge once that has passed sucessfully.

cbuescher · 2018-08-06T20:41:23Z

@elasticmachine test this please

cbuescher · 2018-08-06T20:45:49Z

@nikoncode would you mind adding a bit more detail to the first comment, since this is what the final commit message will be based on? Otherwise I can also write a few lines, but I think its better if the commit author adds a few words.

cbuescher · 2018-08-06T22:00:47Z

@nikoncode I took the liberty to merge in the current state of the master branch because CI was failing without it. Hope you don't mind.
@elasticmachine test this please

nikoncode · 2018-08-12T17:02:02Z

@cbuescher I have changed my message.
Does it specific enough now?

cbuescher

Still looking good. I will merge this in shortly

Limit date `format` attribute to String values only. Closes #23650

* master: Adjust BWC version on mapping version Token API supports the client_credentials grant (#33106) Build: forked compiler max memory matches jvmArgs (#33138) Introduce mapping version to index metadata (#33147) SQL: Enable aggregations to create a separate bucket for missing values (#32832) Fix grammar in contributing docs SECURITY: Fix Compile Error in ReservedRealmTests (#33166) APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Fix forbiddenapis on java 11 (#33116) Apply publishing to genreate pom (#33094) Have circuit breaker succeed on unknown mem usage Do not lose default mapper on metadata updates (#33153) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

support only string format for date, root object & date range

902be2b

colings86 added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types labels Apr 24, 2018

cbuescher self-assigned this Jul 24, 2018

cbuescher requested changes Jul 24, 2018

View reviewed changes

Mikita Karaliou added 2 commits July 29, 2018 17:42

change proto of parseDateTimeFormatter to support only Strings

4d41cf8

Revert "change proto of parseDateTimeFormatter to support only Strings"

cac0999

This reverts commit 4d41cf8.

cbuescher approved these changes Aug 6, 2018

View reviewed changes

Merge branch 'master' into issue-23650

d8960b1

javanna requested a review from cbuescher August 16, 2018 14:03

cbuescher approved these changes Aug 27, 2018

View reviewed changes

cbuescher merged commit f1f6d4e into elastic:master Aug 27, 2018

cbuescher pushed a commit that referenced this pull request Aug 27, 2018

Support only string format in date, root object & date range (#28117)

a54ebd5

Limit date `format` attribute to String values only. Closes #23650

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support only string format for date, root object & date range #28117

support only string format for date, root object & date range #28117

nikoncode commented Jan 6, 2018 •

edited

Loading

elasticmachine commented Jan 6, 2018

elasticmachine commented Jan 6, 2018

elasticmachine commented Apr 24, 2018

cbuescher commented Jul 24, 2018

cbuescher left a comment

cbuescher Jul 24, 2018

nikoncode commented Jul 28, 2018

nikoncode commented Jul 29, 2018 •

edited

Loading

cbuescher commented Aug 6, 2018 •

edited

Loading

cbuescher commented Aug 6, 2018

cbuescher commented Aug 6, 2018

cbuescher commented Aug 6, 2018

nikoncode commented Aug 12, 2018 •

edited

Loading

cbuescher left a comment

support only string format for date, root object & date range #28117

support only string format for date, root object & date range #28117

Conversation

nikoncode commented Jan 6, 2018 • edited Loading

elasticmachine commented Jan 6, 2018

elasticmachine commented Jan 6, 2018

elasticmachine commented Apr 24, 2018

cbuescher commented Jul 24, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher Jul 24, 2018

Choose a reason for hiding this comment

nikoncode commented Jul 28, 2018

nikoncode commented Jul 29, 2018 • edited Loading

cbuescher commented Aug 6, 2018 • edited Loading

cbuescher commented Aug 6, 2018

cbuescher commented Aug 6, 2018

cbuescher commented Aug 6, 2018

nikoncode commented Aug 12, 2018 • edited Loading

cbuescher left a comment

Choose a reason for hiding this comment

nikoncode commented Jan 6, 2018 •

edited

Loading

nikoncode commented Jul 29, 2018 •

edited

Loading

cbuescher commented Aug 6, 2018 •

edited

Loading

nikoncode commented Aug 12, 2018 •

edited

Loading