From a4488edbc59e8f75928c4b33a0f0ddcdd62a9684 Mon Sep 17 00:00:00 2001 From: David Pilato Date: Tue, 23 Dec 2014 14:46:00 +0100 Subject: [PATCH] New timestamp default option changed the default behavior for missing paths PR #7036 changed the behavior for timestamp when provided as a parameter or within the document using `path` attribute. This PR now considers that: * when using timestamp as a parameter, we use a default value of `now`. Which means that if no timestamp is provided, the current time is used when the index operation is performed. * when getting the timestamp from `path`, we use a default value of `null`. Which means that if no value is provided within the document, indexation will fail. If users want to set the default value for `timestamp`, they can explicitly set one or set `"default": "now"`. Closes #8882. --- .../mapping/fields/timestamp-field.asciidoc | 9 +-- .../mapper/internal/TimestampFieldMapper.java | 20 ++++++- .../timestamp/TimestampMappingTests.java | 60 ++++++++++++++++--- 3 files changed, 74 insertions(+), 15 deletions(-) diff --git a/docs/reference/mapping/fields/timestamp-field.asciidoc b/docs/reference/mapping/fields/timestamp-field.asciidoc index 9c12add47dfe3..2f58878f1d072 100644 --- a/docs/reference/mapping/fields/timestamp-field.asciidoc +++ b/docs/reference/mapping/fields/timestamp-field.asciidoc @@ -90,8 +90,10 @@ You can define a default value for when timestamp is not provided within the index request or in the `_source` document. By default, the default value is `now` which means the date the document was processed by the indexing chain. +But if you are using `path`, there is no default value which means that indexing will fail if your document does +not provide the date. -You can disable that default value by setting `default` to `null`. It means that `timestamp` is mandatory: +You force also the default value for path by setting `default` to `now`: [source,js] -------------------------------------------------- @@ -99,14 +101,13 @@ You can disable that default value by setting `default` to `null`. It means that "tweet" : { "_timestamp" : { "enabled" : true, - "default" : null + "path" : "post_date", + "default" : "now" } } } -------------------------------------------------- -If you don't provide any timestamp value, indexation will fail. - You can also set the default value to any date respecting <>: [source,js] diff --git a/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java index aae2104287820..697723109a20d 100644 --- a/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/internal/TimestampFieldMapper.java @@ -74,7 +74,8 @@ public static class Defaults extends DateFieldMapper.Defaults { public static final EnabledAttributeMapper ENABLED = EnabledAttributeMapper.UNSET_DISABLED; public static final String PATH = null; public static final FormatDateTimeFormatter DATE_TIME_FORMATTER = Joda.forPattern(DEFAULT_DATE_TIME_FORMAT); - public static final String DEFAULT_TIMESTAMP = "now"; + public static final String NOW_TIMESTAMP = "now"; + public static final String DEFAULT_TIMESTAMP = NOW_TIMESTAMP; } public static class Builder extends NumberFieldMapper.Builder { @@ -129,6 +130,8 @@ public TimestampFieldMapper build(BuilderContext context) { public static class TypeParser implements Mapper.TypeParser { @Override public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { + boolean pathSet = false; + boolean defaultSet = false; TimestampFieldMapper.Builder builder = timestamp(); parseField(builder, builder.name, node, parserContext); for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { @@ -141,15 +144,23 @@ public Mapper.Builder parse(String name, Map node, ParserContext iterator.remove(); } else if (fieldName.equals("path")) { builder.path(fieldNode.toString()); + pathSet = true; iterator.remove(); } else if (fieldName.equals("format")) { builder.dateTimeFormatter(parseDateTimeFormatter(fieldNode.toString())); iterator.remove(); } else if (fieldName.equals("default")) { builder.defaultTimestamp(fieldNode == null ? null : fieldNode.toString()); + defaultSet = true; iterator.remove(); } } + + // When path is used and no default value is not explicitly set, we force it to null. + if (pathSet && defaultSet == false) { + builder.defaultTimestamp(null); + } + return builder; } } @@ -283,7 +294,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (includeDefaults || !dateTimeFormatter.format().equals(Defaults.DATE_TIME_FORMATTER.format())) { builder.field("format", dateTimeFormatter.format()); } - if (includeDefaults || !Defaults.DEFAULT_TIMESTAMP.equals(defaultTimestamp)) { + // We print `default` if explicitly required or if not using path with default != now + // or if using using path with default != null + if (includeDefaults || (path == Defaults.PATH && !Defaults.DEFAULT_TIMESTAMP.equals(defaultTimestamp)) || + (path != Defaults.PATH && defaultTimestamp != null)) { builder.field("default", defaultTimestamp); } if (customFieldDataSettings != null) { @@ -305,7 +319,7 @@ public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappi this.enabledState = timestampFieldMapperMergeWith.enabledState; } } else { - if (!timestampFieldMapperMergeWith.defaultTimestamp().equals(defaultTimestamp)) { + if (timestampFieldMapperMergeWith.defaultTimestamp() != null && !timestampFieldMapperMergeWith.defaultTimestamp().equals(defaultTimestamp)) { mergeContext.addConflict("Cannot update default in _timestamp value. Value is " + defaultTimestamp.toString() + " now encountering " + timestampFieldMapperMergeWith.defaultTimestamp()); } if (this.path != null) { diff --git a/src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java b/src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java index c15eb8c5cba61..78ea61e573b8d 100644 --- a/src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java +++ b/src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java @@ -168,7 +168,7 @@ public void testThatSerializationWorksCorrectlyForIndexField() throws Exception assertThat(timestampConfiguration.get("index").toString(), is("no")); } - @Test // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] + @Test(expected = TimestampParsingException.class) // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] public void testPathMissingDefaultValue() throws Exception { XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("_timestamp") @@ -188,11 +188,6 @@ public void testPathMissingDefaultValue() throws Exception { IndexRequest request = new IndexRequest("test", "type", "1").source(doc); request.process(metaData, mappingMetaData, true, "test"); - assertThat(request.timestamp(), notNullValue()); - - // We should have less than one minute (probably some ms) - long delay = System.currentTimeMillis() - Long.parseLong(request.timestamp()); - assertThat(delay, lessThanOrEqualTo(60000L)); } @Test // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] @@ -330,7 +325,7 @@ public void testTimestampMissingNowDefaultValue() throws Exception { } @Test(expected = TimestampParsingException.class) // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] - public void testPathMissingShouldFail() throws Exception { + public void testPathMissingWithForcedNullDefaultShouldFail() throws Exception { XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("_timestamp") .field("enabled", "yes") @@ -353,7 +348,29 @@ public void testPathMissingShouldFail() throws Exception { } @Test(expected = TimestampParsingException.class) // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] - public void testTimestampMissingShouldFail() throws Exception { + public void testPathMissingShouldFail() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("_timestamp") + .field("enabled", "yes") + .field("path", "timestamp") + .endObject() + .endObject().endObject(); + XContentBuilder doc = XContentFactory.jsonBuilder() + .startObject() + .field("foo", "bar") + .endObject(); + + MetaData metaData = MetaData.builder().build(); + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping.string()); + + MappingMetaData mappingMetaData = new MappingMetaData(docMapper); + + IndexRequest request = new IndexRequest("test", "type", "1").source(doc); + request.process(metaData, mappingMetaData, true, "test"); + } + + @Test(expected = TimestampParsingException.class) // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] + public void testTimestampMissingWithForcedNullDefaultShouldFail() throws Exception { XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("_timestamp") .field("enabled", "yes") @@ -374,6 +391,33 @@ public void testTimestampMissingShouldFail() throws Exception { request.process(metaData, mappingMetaData, true, "test"); } + @Test // Issue 4718: was throwing a TimestampParsingException: failed to parse timestamp [null] + public void testTimestampMissingShouldNotFail() throws Exception { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("_timestamp") + .field("enabled", "yes") + .endObject() + .endObject().endObject(); + XContentBuilder doc = XContentFactory.jsonBuilder() + .startObject() + .field("foo", "bar") + .endObject(); + + MetaData metaData = MetaData.builder().build(); + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping.string()); + + MappingMetaData mappingMetaData = new MappingMetaData(docMapper); + + IndexRequest request = new IndexRequest("test", "type", "1").source(doc); + request.process(metaData, mappingMetaData, true, "test"); + + assertThat(request.timestamp(), notNullValue()); + + // We should have less than one minute (probably some ms) + long delay = System.currentTimeMillis() - Long.parseLong(request.timestamp()); + assertThat(delay, lessThanOrEqualTo(60000L)); + } + @Test public void testDefaultTimestampStream() throws IOException { // Testing null value for default timestamp