diff --git a/docs/changelog/108746.yaml b/docs/changelog/108746.yaml new file mode 100644 index 000000000000..93ed917f3b56 --- /dev/null +++ b/docs/changelog/108746.yaml @@ -0,0 +1,5 @@ +pr: 108746 +summary: Support synthetic source for `aggregate_metric_double` when ignore_malf… +area: Mapping +type: feature +issues: [] diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index b08e0d221cf7..2227c5487135 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -748,4 +748,24 @@ public class XContentHelper { throw new ElasticsearchGenerationException("Failed to generate [" + source + "]", e); } } + + /** + * Drains all data available via this parser into a provided builder. + * Provided parser is closed as a result. + * @param parser + * @param destination + */ + public static void drainAndClose(XContentParser parser, XContentBuilder destination) throws IOException { + if (parser.isClosed()) { + throw new IllegalStateException("Can't drain a parser that is closed"); + } + + XContentParser.Token token; + do { + destination.copyCurrentStructure(parser); + token = parser.nextToken(); + } while (token != null); + + parser.close(); + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/XContentDataHelper.java b/server/src/main/java/org/elasticsearch/index/mapper/XContentDataHelper.java index a527a2df2049..6b5b2537e5e1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/XContentDataHelper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/XContentDataHelper.java @@ -30,7 +30,7 @@ import java.util.Arrays; /** * Helper class for processing field data of any type, as provided by the {@link XContentParser}. */ -final class XContentDataHelper { +public final class XContentDataHelper { /** * Build a {@link StoredField} for the value on which the parser is * currently positioned. @@ -57,7 +57,7 @@ final class XContentDataHelper { * Build a {@link BytesRef} wrapping a byte array containing an encoded form * of the passed XContentBuilder contents. */ - static BytesRef encodeXContentBuilder(XContentBuilder builder) throws IOException { + public static BytesRef encodeXContentBuilder(XContentBuilder builder) throws IOException { return new BytesRef(TypeUtils.encode(builder)); } diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java index 5b50eb63e148..a3e11c0645e3 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.common.xcontent.support; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; @@ -420,4 +421,25 @@ public class XContentHelperTests extends ESTestCase { assertThat(names, equalTo(Set.of("a", "c"))); } + + public void testDrainAndClose() throws IOException { + String json = """ + { "a": "b", "c": "d", "e": {"f": "g"}, "h": ["i", "j", {"k": "l"}]}"""; + var parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json); + var content = XContentBuilder.builder(XContentType.JSON.xContent()); + XContentHelper.drainAndClose(parser, content); + + assertEquals(json.replace(" ", ""), Strings.toString(content)); + assertTrue(parser.isClosed()); + } + + public void testDrainAndCloseAlreadyClosed() throws IOException { + var parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, "{}"); + parser.close(); + + assertThrows( + IllegalStateException.class, + () -> XContentHelper.drainAndClose(parser, XContentBuilder.builder(XContentType.JSON.xContent())) + ); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index f9af0d27f3e6..3b9195266b06 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1115,7 +1115,7 @@ public abstract class MapperTestCase extends MapperServiceTestCase { assertSyntheticSource(syntheticSourceSupport(ignoreMalformed).example(5)); } - public final void testSyntheticSourceIgnoreMalformedExamples() throws IOException { + public void testSyntheticSourceIgnoreMalformedExamples() throws IOException { assumeTrue("type doesn't support ignore_malformed", supportsIgnoreMalformed()); CheckedConsumer mapping = syntheticSourceSupport(true).example(1).mapping(); for (ExampleMalformedValue v : exampleMalformedValues()) { diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java index ad54c0c7746b..81abe3dc5c08 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java @@ -19,6 +19,7 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateMathParser; import org.elasticsearch.common.util.BigArrays; +import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.index.IndexMode; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.fielddata.FieldDataContext; @@ -29,6 +30,7 @@ import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.index.mapper.DocumentParserContext; import org.elasticsearch.index.mapper.FieldMapper; +import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperBuilderContext; @@ -41,6 +43,7 @@ import org.elasticsearch.index.mapper.TextSearchInfo; import org.elasticsearch.index.mapper.TimeSeriesParams; import org.elasticsearch.index.mapper.TimeSeriesParams.MetricType; import org.elasticsearch.index.mapper.ValueFetcher; +import org.elasticsearch.index.mapper.XContentDataHelper; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.script.ScriptCompiler; @@ -587,6 +590,12 @@ public class AggregateDoubleMetricFieldMapper extends FieldMapper { XContentParser.Token token; XContentSubParser subParser = null; EnumMap metricsParsed = new EnumMap<>(Metric.class); + // Preserves the content of the field in order to be able to construct synthetic source + // if field value is malformed. + XContentBuilder malformedContentForSyntheticSource = context.mappingLookup().isSourceSynthetic() && ignoreMalformed + ? XContentBuilder.builder(context.parser().contentType().xContent()) + : null; + try { token = context.parser().currentToken(); if (token == XContentParser.Token.VALUE_NULL) { @@ -596,6 +605,9 @@ public class AggregateDoubleMetricFieldMapper extends FieldMapper { ensureExpectedToken(XContentParser.Token.START_OBJECT, token, context.parser()); subParser = new XContentSubParser(context.parser()); token = subParser.nextToken(); + if (malformedContentForSyntheticSource != null) { + malformedContentForSyntheticSource.startObject(); + } while (token != XContentParser.Token.END_OBJECT) { // should be an object sub-field with name a metric name ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, subParser); @@ -609,13 +621,20 @@ public class AggregateDoubleMetricFieldMapper extends FieldMapper { } token = subParser.nextToken(); + if (malformedContentForSyntheticSource != null) { + malformedContentForSyntheticSource.field(fieldName); + } // Make sure that the value is a number. Probably this will change when // new aggregate metric types are added (histogram, cardinality etc) ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, subParser); NumberFieldMapper delegateFieldMapper = metricFieldMappers.get(metric); // Delegate parsing the field to a numeric field mapper try { - metricsParsed.put(metric, delegateFieldMapper.value(context.parser())); + Number metricValue = delegateFieldMapper.value(context.parser()); + metricsParsed.put(metric, metricValue); + if (malformedContentForSyntheticSource != null) { + malformedContentForSyntheticSource.value(metricValue); + } } catch (IllegalArgumentException e) { throw new IllegalArgumentException("failed to parse [" + metric.name() + "] sub field: " + e.getMessage(), e); } @@ -658,10 +677,26 @@ public class AggregateDoubleMetricFieldMapper extends FieldMapper { } } catch (Exception e) { if (ignoreMalformed) { - if (subParser != null) { - // close the subParser so we advance to the end of the object + if (malformedContentForSyntheticSource != null) { + if (subParser != null) { + // Remaining data in parser needs to be stored as is in order to provide it in synthetic source. + XContentHelper.drainAndClose(subParser, malformedContentForSyntheticSource); + } else { + // We don't use DrainingXContentParser since we don't want to go beyond current field + malformedContentForSyntheticSource.copyCurrentStructure(context.parser()); + } + ; + var nameValue = IgnoredSourceFieldMapper.NameValue.fromContext( + context, + name(), + XContentDataHelper.encodeXContentBuilder(malformedContentForSyntheticSource) + ); + context.addIgnoredField(nameValue); + } else if (subParser != null) { + // close the subParser, so we advance to the end of the object subParser.close(); } + context.addIgnoredField(name()); context.path().remove(); return; @@ -689,11 +724,7 @@ public class AggregateDoubleMetricFieldMapper extends FieldMapper { @Override public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { - if (ignoreMalformed) { - throw new IllegalArgumentException( - "field [" + name() + "] of type [" + typeName() + "] doesn't support synthetic source because it ignores malformed numbers" - ); - } + // Note that malformed values are handled via `IgnoredSourceFieldMapper` infrastructure return new AggregateMetricSyntheticFieldLoader(name(), simpleName(), metrics); } diff --git a/x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java b/x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java index 6646db4f2abf..83e701486d93 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/test/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java @@ -18,6 +18,7 @@ import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.MapperTestCase; import org.elasticsearch.index.mapper.ParsedDocument; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xpack.aggregatemetric.AggregateMetricMapperPlugin; @@ -33,11 +34,12 @@ import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.function.Supplier; +import static org.elasticsearch.xpack.aggregatemetric.mapper.AggregateDoubleMetricFieldMapper.Names.IGNORE_MALFORMED; import static org.elasticsearch.xpack.aggregatemetric.mapper.AggregateDoubleMetricFieldMapper.Names.METRICS; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.matchesPattern; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.core.IsInstanceOf.instanceOf; @@ -467,8 +469,12 @@ public class AggregateDoubleMetricFieldMapperTests extends MapperTestCase { @Override protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) { - assumeFalse("synthetic _source support for aggregate_double_metric doesn't support ignore_malformed", ignoreMalformed); - return new AggregateDoubleMetricSyntheticSourceSupport(); + return new AggregateDoubleMetricSyntheticSourceSupport(ignoreMalformed); + } + + @Override + public void testSyntheticSourceIgnoreMalformedExamples() { + assumeTrue("Scenarios are covered in scope of syntheticSourceSupport", false); } @Override @@ -478,16 +484,94 @@ public class AggregateDoubleMetricFieldMapperTests extends MapperTestCase { protected final class AggregateDoubleMetricSyntheticSourceSupport implements SyntheticSourceSupport { - private final EnumSet storedMetrics = EnumSet.copyOf(randomNonEmptySubsetOf(Arrays.asList(Metric.values()))); + private final boolean malformedExample; + private final EnumSet storedMetrics; + + public AggregateDoubleMetricSyntheticSourceSupport(boolean malformedExample) { + this.malformedExample = malformedExample; + this.storedMetrics = EnumSet.copyOf(randomNonEmptySubsetOf(Arrays.asList(Metric.values()))); + } @Override public SyntheticSourceExample example(int maxVals) { // aggregate_metric_double field does not support arrays - Map value = randomAggregateMetric(); + Object value = randomAggregateMetric(); return new SyntheticSourceExample(value, value, this::mapping); } - private Map randomAggregateMetric() { + private Object randomAggregateMetric() { + if (malformedExample && randomBoolean()) { + return malformedValue(); + } + + return validMetrics(); + } + + private Object malformedValue() { + List> choices = List.of( + () -> randomAlphaOfLength(3), + ESTestCase::randomInt, + ESTestCase::randomLong, + ESTestCase::randomFloat, + ESTestCase::randomDouble, + ESTestCase::randomBoolean, + // no metrics + Map::of, + // unmapped metric + () -> { + var metrics = validMetrics(); + metrics.put("hello", "world"); + return metrics; + }, + // missing metric + () -> { + var metrics = validMetrics(); + metrics.remove(storedMetrics.stream().findFirst().get().name()); + return metrics; + }, + // invalid metric value + () -> { + var metrics = validMetrics(); + metrics.put(storedMetrics.stream().findFirst().get().name(), "boom"); + return metrics; + }, + // metric is an object + () -> { + var metrics = validMetrics(); + metrics.put(storedMetrics.stream().findFirst().get().name(), Map.of("hello", "world")); + return metrics; + }, + // invalid metric value with additional data + () -> { + var metrics = validMetrics(); + metrics.put(storedMetrics.stream().findFirst().get().name(), "boom"); + metrics.put("hello", "world"); + metrics.put("object", Map.of("hello", "world")); + metrics.put("list", List.of("hello", "world")); + return metrics; + }, + // negative value count + () -> { + var metrics = validMetrics(); + if (storedMetrics.contains(Metric.value_count.name())) { + metrics.put(Metric.value_count.name(), -100); + } + return metrics; + }, + // value count with decimal digits (whole numbers formatted as doubles are permitted, but non-whole numbers are not) + () -> { + var metrics = validMetrics(); + if (storedMetrics.contains(Metric.value_count.name())) { + metrics.put(Metric.value_count.name(), 10.5); + } + return metrics; + } + ); + + return randomFrom(choices).get(); + } + + private Map validMetrics() { Map value = new LinkedHashMap<>(storedMetrics.size()); for (Metric m : storedMetrics) { if (Metric.value_count == m) { @@ -506,19 +590,14 @@ public class AggregateDoubleMetricFieldMapperTests extends MapperTestCase { private void mapping(XContentBuilder b) throws IOException { String[] metrics = storedMetrics.stream().map(Metric::toString).toArray(String[]::new); b.field("type", CONTENT_TYPE).array(METRICS_FIELD, metrics).field(DEFAULT_METRIC, metrics[0]); + if (malformedExample) { + b.field(IGNORE_MALFORMED, true); + } } @Override public List invalidExample() throws IOException { - return List.of( - new SyntheticSourceInvalidExample( - matchesPattern("field \\[field] of type \\[.+] doesn't support synthetic source because it ignores malformed numbers"), - b -> { - mapping(b); - b.field("ignore_malformed", true); - } - ) - ); + return List.of(); } } diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/aggregate-metrics/100_synthetic_source.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/aggregate-metrics/100_synthetic_source.yml index 45ec59a419e1..b846dbe858f6 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/aggregate-metrics/100_synthetic_source.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/aggregate-metrics/100_synthetic_source.yml @@ -1,4 +1,4 @@ -constant_keyword: +aggregate_metric_double: - requires: cluster_features: ["gte_v8.5.0"] reason: synthetic source support added in 8.5.0 @@ -51,3 +51,54 @@ constant_keyword: min: 18.2 max: 100.0 value_count: 50 + +--- +aggregate_metric_double with ignore_malformed: + - requires: + cluster_features: ["mapper.track_ignored_source"] + reason: requires tracking ignored source + + - do: + indices.create: + index: test + body: + mappings: + _source: + mode: synthetic + properties: + metric: + type: aggregate_metric_double + metrics: [min, max, value_count] + default_metric: max + ignore_malformed: true + + - do: + index: + index: test + id: "1" + refresh: true + body: + metric: + min: 18.2 + max: 100 + field: "field" + sub: + array: [1, 2, 3] + field: "field" + value_count: 50 + + - do: + search: + index: test + + - match: + hits.hits.0._source: + metric: + min: 18.2 + max: 100 + field: "field" + sub: + array: [1, 2, 3] + field: "field" + value_count: 50 +