From 2325b02b644bffc7c58c17ff3bfd2b87e2acd8b8 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Sat, 4 May 2024 07:38:42 -0400 Subject: [PATCH] DATETIME_FORMATTER_CACHING_SETTING experimental feature should not default to 'true' (#13532) (#13538) (cherry picked from commit 91067134e8e0a6dbfb05317afe4752194a8a69d2) Signed-off-by: Andriy Redko Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] --- CHANGELOG.md | 1 + .../opensearch/common/util/FeatureFlags.java | 2 +- .../common/util/FeatureFlagTests.java | 12 ++--- .../index/mapper/DateFieldMapperTests.java | 18 ++++++++ .../index/mapper/RangeFieldMapperTests.java | 26 +++++++++++ .../index/mapper/RangeFieldTypeTests.java | 45 +++++++++++++++++++ 6 files changed, 94 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8d219c547166..c6c4eab918de3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Improve the error messages for _stats with closed indices ([#13012](https://github.com/opensearch-project/OpenSearch/pull/13012)) - Ignore BaseRestHandler unconsumed content check as it's always consumed. ([#13290](https://github.com/opensearch-project/OpenSearch/pull/13290)) - Fix mapper_parsing_exception when using flat_object fields with names longer than 11 characters ([#13259](https://github.com/opensearch-project/OpenSearch/pull/13259)) +- DATETIME_FORMATTER_CACHING_SETTING experimental feature should not default to 'true' ([#13532](https://github.com/opensearch-project/OpenSearch/pull/13532)) ### Security diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index bdfce72d106d3..a72583607ede0 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -81,7 +81,7 @@ public class FeatureFlags { public static final Setting DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting( DATETIME_FORMATTER_CACHING, - true, + false, Property.NodeScope ); diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java index 88cb3782252b7..6b6c7b96266d3 100644 --- a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java +++ b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java @@ -39,12 +39,6 @@ public void testNonBooleanFeatureFlag() { assertFalse(FeatureFlags.isEnabled(javaVersionProperty)); } - public void testBooleanFeatureFlagWithDefaultSetToTrue() { - final String testFlag = DATETIME_FORMATTER_CACHING; - assertNotNull(testFlag); - assertTrue(FeatureFlags.isEnabled(testFlag)); - } - public void testBooleanFeatureFlagWithDefaultSetToFalse() { final String testFlag = IDENTITY; FeatureFlags.initializeFeatureFlags(Settings.EMPTY); @@ -52,17 +46,17 @@ public void testBooleanFeatureFlagWithDefaultSetToFalse() { assertFalse(FeatureFlags.isEnabled(testFlag)); } - public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToTrue() { + public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToFalse() { final String testFlag = DATETIME_FORMATTER_CACHING; FeatureFlags.initializeFeatureFlags(Settings.EMPTY); assertNotNull(testFlag); - assertTrue(FeatureFlags.isEnabled(testFlag)); + assertFalse(FeatureFlags.isEnabled(testFlag)); } public void testInitializeFeatureFlagsWithExperimentalSettings() { FeatureFlags.initializeFeatureFlags(Settings.builder().put(IDENTITY, true).build()); assertTrue(FeatureFlags.isEnabled(IDENTITY)); - assertTrue(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING)); + assertFalse(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING)); assertFalse(FeatureFlags.isEnabled(EXTENSIONS)); // reset FeatureFlags to defaults FeatureFlags.initializeFeatureFlags(Settings.EMPTY); diff --git a/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java index 054d3956596af..2aa310ae959d9 100644 --- a/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java @@ -35,6 +35,7 @@ import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexableField; import org.opensearch.common.time.DateFormatter; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.termvectors.TermVectorsService; import org.opensearch.search.DocValueFormat; @@ -45,8 +46,10 @@ import java.time.ZonedDateTime; import java.util.List; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assume.assumeThat; public class DateFieldMapperTests extends MapperTestCase { @@ -146,7 +149,22 @@ public void testStore() throws Exception { assertEquals(1457654400000L, storedField.numericValue().longValue()); } + public void testIgnoreMalformedLegacy() throws IOException { + assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false)); + testIgnoreMalformedForValue( + "2016-03-99", + "failed to parse date field [2016-03-99] with format [strict_date_optional_time||epoch_millis]" + ); + testIgnoreMalformedForValue("-2147483648", "Invalid value for Year (valid values -999999999 - 999999999): -2147483648"); + testIgnoreMalformedForValue("-522000000", "long overflow"); + } + public void testIgnoreMalformed() throws IOException { + assumeThat( + "Using experimental datetime format as default", + FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), + is(true) + ); testIgnoreMalformedForValue( "2016-03-99", "failed to parse date field [2016-03-99] with format [strict_date_time_no_millis||strict_date_optional_time||epoch_millis]" diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java index 331bfb7b2ddf4..91eab942c499a 100644 --- a/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java @@ -37,6 +37,7 @@ import org.apache.lucene.index.IndexableField; import org.opensearch.common.CheckedConsumer; import org.opensearch.common.network.InetAddresses; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; @@ -51,8 +52,10 @@ import static org.opensearch.index.query.RangeQueryBuilder.GT_FIELD; import static org.opensearch.index.query.RangeQueryBuilder.LTE_FIELD; import static org.opensearch.index.query.RangeQueryBuilder.LT_FIELD; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.containsString; +import static org.junit.Assume.assumeThat; public class RangeFieldMapperTests extends AbstractNumericFieldMapperTestCase { private static final String FROM_DATE = "2016-10-31"; @@ -351,7 +354,30 @@ public void testIllegalArguments() throws Exception { assertThat(e.getMessage(), containsString("should not define a dateTimeFormatter")); } + public void testSerializeDefaultsLegacy() throws Exception { + assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false)); + + for (String type : types()) { + DocumentMapper docMapper = createDocumentMapper(fieldMapping(b -> b.field("type", type))); + RangeFieldMapper mapper = (RangeFieldMapper) docMapper.root().getMapper("field"); + XContentBuilder builder = XContentFactory.jsonBuilder().startObject(); + mapper.doXContentBody(builder, true, ToXContent.EMPTY_PARAMS); + String got = builder.endObject().toString(); + + // if type is date_range we check that the mapper contains the default format and locale + // otherwise it should not contain a locale or format + assertTrue(got, got.contains("\"format\":\"strict_date_optional_time||epoch_millis\"") == type.equals("date_range")); + assertTrue(got, got.contains("\"locale\":" + "\"" + Locale.ROOT + "\"") == type.equals("date_range")); + } + } + public void testSerializeDefaults() throws Exception { + assumeThat( + "Using experimental datetime format as default", + FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), + is(true) + ); + for (String type : types()) { DocumentMapper docMapper = createDocumentMapper(fieldMapping(b -> b.field("type", type))); RangeFieldMapper mapper = (RangeFieldMapper) docMapper.root().getMapper("field"); diff --git a/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java index 00b48240d0567..49bf227e5073c 100644 --- a/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java @@ -51,6 +51,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.time.DateFormatter; import org.opensearch.common.util.BigArrays; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexSettings; import org.opensearch.index.mapper.DateFieldMapper.DateFieldType; import org.opensearch.index.mapper.RangeFieldMapper.RangeFieldType; @@ -65,8 +66,10 @@ import java.util.Collections; import java.util.Map; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assume.assumeThat; public class RangeFieldTypeTests extends FieldTypeTestCase { RangeType type; @@ -249,7 +252,49 @@ private QueryShardContext createContext() { ); } + public void testDateRangeQueryUsingMappingFormatLegacy() { + assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false)); + + QueryShardContext context = createContext(); + RangeFieldType strict = new RangeFieldType("field", RangeFieldMapper.Defaults.DATE_FORMATTER); + // don't use DISJOINT here because it doesn't work on date fields which we want to compare bounds with + ShapeRelation relation = randomValueOtherThan(ShapeRelation.DISJOINT, () -> randomFrom(ShapeRelation.values())); + + // dates will break the default format, month/day of month is turned around in the format + final String from = "2016-15-06T15:29:50+08:00"; + final String to = "2016-16-06T15:29:50+08:00"; + + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> strict.rangeQuery(from, to, true, true, relation, null, null, context) + ); + assertThat( + ex.getMessage(), + containsString("failed to parse date field [2016-15-06T15:29:50+08:00] with format [strict_date_optional_time||epoch_millis]") + ); + + // setting mapping format which is compatible with those dates + final DateFormatter formatter = DateFormatter.forPattern("yyyy-dd-MM'T'HH:mm:ssZZZZZ"); + assertEquals(1465975790000L, formatter.parseMillis(from)); + assertEquals(1466062190000L, formatter.parseMillis(to)); + + RangeFieldType fieldType = new RangeFieldType("field", formatter); + final Query query = fieldType.rangeQuery(from, to, true, true, relation, null, fieldType.dateMathParser(), context); + assertEquals("field:", ((IndexOrDocValuesQuery) query).getIndexQuery().toString()); + + // compare lower and upper bounds with what we would get on a `date` field + DateFieldType dateFieldType = new DateFieldType("field", DateFieldMapper.Resolution.MILLISECONDS, formatter); + final Query queryOnDateField = dateFieldType.rangeQuery(from, to, true, true, relation, null, fieldType.dateMathParser(), context); + assertEquals("field:[1465975790000 TO 1466062190999]", ((IndexOrDocValuesQuery) queryOnDateField).getIndexQuery().toString()); + } + public void testDateRangeQueryUsingMappingFormat() { + assumeThat( + "Using experimental datetime format as default", + FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), + is(true) + ); + QueryShardContext context = createContext(); RangeFieldType strict = new RangeFieldType("field", RangeFieldMapper.Defaults.DATE_FORMATTER); // don't use DISJOINT here because it doesn't work on date fields which we want to compare bounds with