From 1d88fe639ba926ee26f728c6cbed25948cbcb90b Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Fri, 18 Jun 2021 14:12:43 +0200 Subject: [PATCH] Dynamic runtime to not dynamically create objects (#74234) When we introduced dynamic:runtime (#65489) we decided to have it create objects dynamically under properties, as the runtime section did not (and still does not) support object fields. That proved to be a poor choice, because the runtime section is flat, supports dots in field names, and does not really need objects. Also, these end up causing unnecessary mapping conflicts. With this commit we adapt dynamic:runtime to not dynamically create objects. Closes #70268 --- .../mapping/dynamic/field-mapping.asciidoc | 4 +- .../index/mapper/DynamicMappingIT.java | 82 ++++++++++++++++++- .../index/mapper/DocumentParser.java | 32 ++++++-- .../index/mapper/DynamicFieldsBuilder.java | 10 +-- .../index/mapper/DocumentParserTests.java | 3 +- .../index/mapper/DynamicMappingTests.java | 11 +-- .../index/mapper/DynamicRuntimeTests.java | 23 ++++-- 7 files changed, 134 insertions(+), 31 deletions(-) diff --git a/docs/reference/mapping/dynamic/field-mapping.asciidoc b/docs/reference/mapping/dynamic/field-mapping.asciidoc index c3687c9d7e2c2..69b43dd66eb58 100644 --- a/docs/reference/mapping/dynamic/field-mapping.asciidoc +++ b/docs/reference/mapping/dynamic/field-mapping.asciidoc @@ -22,12 +22,12 @@ h| JSON data type h| `"dynamic":"true"` h| `"dynamic":"runtime"` |`true` or `false` 2*| `boolean` |`double` | `float` | `double` |`integer` 2*| `long` - |`object`^1^ 2*| `object` + |`object` | `object` | No field added |`array` 2*| Depends on the first non-`null` value in the array |`string` that passes <> 2*| `date` |`string` that passes <> | `float` or `long` | `double` or `long` |`string` that doesn't pass `date` detection or `numeric` detection | `text` with a `.keyword` sub-field | `keyword` -3+| ^1^Objects are always mapped as part of the `properties` section, even when the `dynamic` parameter is set to `runtime`. | | +3+| |=== // end::dynamic-field-mapping-types-tag[] diff --git a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java index 29671140b87fe..ed2c9979d4e3b 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java @@ -20,13 +20,17 @@ import org.elasticsearch.cluster.metadata.MappingMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Randomness; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.core.TimeValue; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.support.XContentMapValues; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.query.GeoBoundingBoxQueryBuilder; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.InternalSettingsPlugin; import org.hamcrest.Matchers; @@ -44,6 +48,8 @@ import static org.elasticsearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -327,4 +333,78 @@ public void testBulkRequestWithNotFoundDynamicTemplate() throws Exception { assertThat(bulkItemResponses.getItems()[1].getFailureMessage(), containsString("Can't find dynamic template for dynamic template name [bar_foo] of field [address.location]")); } + + public void testDynamicRuntimeNoConflicts() { + assertAcked(client().admin().indices().prepareCreate("test").setMapping("{\"_doc\":{\"dynamic\":\"runtime\"}}").get()); + + List docs = new ArrayList<>(); + docs.add(new IndexRequest("test").source("one.two.three", new int[]{1, 2, 3})); + docs.add(new IndexRequest("test").source("one.two", 1.2)); + docs.add(new IndexRequest("test").source("one", "one")); + docs.add(new IndexRequest("test").source("{\"one\":{\"two\": { \"three\": \"three\"}}}", XContentType.JSON)); + Collections.shuffle(docs, random()); + BulkRequest bulkRequest = new BulkRequest(); + for (IndexRequest doc : docs) { + bulkRequest.add(doc); + } + BulkResponse bulkItemResponses = client().bulk(bulkRequest).actionGet(); + assertFalse(bulkItemResponses.buildFailureMessage(), bulkItemResponses.hasFailures()); + + GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings("test").get(); + Map sourceAsMap = getMappingsResponse.getMappings().get("test").sourceAsMap(); + assertFalse(sourceAsMap.containsKey("properties")); + @SuppressWarnings("unchecked") + Map runtime = (Map)sourceAsMap.get("runtime"); + //depending on the order of the documents field types may differ, but there are no mapping conflicts + assertThat(runtime.keySet(), containsInAnyOrder("one", "one.two", "one.two.three")); + } + + public void testDynamicRuntimeObjectFields() { + assertAcked(client().admin().indices().prepareCreate("test").setMapping("{\"_doc\":{\"properties\":{" + + "\"obj\":{\"properties\":{\"runtime\":{\"type\":\"object\",\"dynamic\":\"runtime\"}}}}}}").get()); + + List docs = new ArrayList<>(); + docs.add(new IndexRequest("test").source("obj.one", 1)); + docs.add(new IndexRequest("test").source("anything", 1)); + docs.add(new IndexRequest("test").source("obj.runtime.one.two", "test")); + docs.add(new IndexRequest("test").source("obj.runtime.one", "one")); + docs.add(new IndexRequest("test").source("{\"obj\":{\"runtime\":{\"one\":{\"two\": \"test\"}}}}", XContentType.JSON)); + Collections.shuffle(docs, random()); + BulkRequest bulkRequest = new BulkRequest(); + for (IndexRequest doc : docs) { + bulkRequest.add(doc); + } + BulkResponse bulkItemResponses = client().bulk(bulkRequest).actionGet(); + assertFalse(bulkItemResponses.buildFailureMessage(), bulkItemResponses.hasFailures()); + + MapperParsingException exception = expectThrows(MapperParsingException.class, + () -> client().prepareIndex("test").setSource("obj.runtime", "value").get()); + assertEquals("object mapping for [obj.runtime] tried to parse field [obj.runtime] as object, but found a concrete value", + exception.getMessage()); + + assertEquals("{\"test\":{\"mappings\":" + + "{\"runtime\":{\"obj.runtime.one\":{\"type\":\"keyword\"},\"obj.runtime.one.two\":{\"type\":\"keyword\"}}," + + "\"properties\":{\"anything\":{\"type\":\"long\"}," + + "\"obj\":{\"properties\":{\"one\":{\"type\":\"long\"}," + + "\"runtime\":{\"type\":\"object\",\"dynamic\":\"runtime\"}}}}}}}", + Strings.toString(client().admin().indices().prepareGetMappings("test").get())); + + assertAcked(client().admin().indices().preparePutMapping("test").setSource("{\"_doc\":{\"properties\":{\"obj\":{\"properties\":" + + "{\"runtime\":{\"properties\":{\"dynamic\":{\"type\":\"object\", \"dynamic\":true}}}}}}}}", XContentType.JSON)); + + assertEquals(RestStatus.CREATED, client().prepareIndex("test").setSource("obj.runtime.dynamic.leaf", 1).get().status()); + GetMappingsResponse getMappingsResponse = client().admin().indices().prepareGetMappings("test").get(); + Map sourceAsMap = getMappingsResponse.getMappings().get("test").sourceAsMap(); + assertThat( + XContentMapValues.extractRawValues("properties.obj.properties.runtime.properties.dynamic.properties.leaf.type", sourceAsMap), + contains("long")); + } + + private static Map getMappedField(Map sourceAsMap, String name) { + @SuppressWarnings("unchecked") + Map properties = (Map)sourceAsMap.get("properties"); + @SuppressWarnings("unchecked") + Map mappedField = (Map)properties.get(name); + return mappedField; + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 97708c04892db..02e4e54386e2f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -13,8 +13,8 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.search.Query; import org.elasticsearch.Version; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Strings; -import org.elasticsearch.core.Tuple; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -22,6 +22,7 @@ import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.core.Tuple; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.fielddata.IndexFieldDataCache; @@ -568,12 +569,19 @@ private static void parseObject(final ParseContext context, ObjectMapper mapper, ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentMapper, context); if (dynamic == ObjectMapper.Dynamic.STRICT) { throw new StrictDynamicMappingException(mapper.fullPath(), currentFieldName); - } else if ( dynamic == ObjectMapper.Dynamic.FALSE) { + } else if (dynamic == ObjectMapper.Dynamic.FALSE) { // not dynamic, read everything up to end object context.parser().skipChildren(); } else { - Mapper dynamicObjectMapper = dynamic.getDynamicFieldsBuilder().createDynamicObjectMapper(context, currentFieldName); - context.addDynamicMapper(dynamicObjectMapper); + Mapper dynamicObjectMapper; + if (dynamic == ObjectMapper.Dynamic.RUNTIME) { + //with dynamic:runtime all leaf fields will be runtime fields unless explicitly mapped, + //hence we don't dynamically create empty objects under properties, but rather carry around an artificial object mapper + dynamicObjectMapper = new NoOpObjectMapper(currentFieldName, context.path().pathAsText(currentFieldName)); + } else { + dynamicObjectMapper = dynamic.getDynamicFieldsBuilder().createDynamicObjectMapper(context, currentFieldName); + context.addDynamicMapper(dynamicObjectMapper); + } context.path().add(currentFieldName); parseObjectOrField(context, dynamicObjectMapper); context.path().remove(); @@ -759,7 +767,8 @@ private static Tuple getDynamicParentMapper(ParseContext int pathsAdded = 0; ObjectMapper parent = mapper; for (int i = 0; i < paths.length-1; i++) { - String currentPath = context.path().pathAsText(paths[i]); + String name = paths[i]; + String currentPath = context.path().pathAsText(name); Mapper existingFieldMapper = context.mappingLookup().getMapper(currentPath); if (existingFieldMapper != null) { throw new MapperParsingException( @@ -771,13 +780,14 @@ private static Tuple getDynamicParentMapper(ParseContext // One mapping is missing, check if we are allowed to create a dynamic one. ObjectMapper.Dynamic dynamic = dynamicOrDefault(parent, context); if (dynamic == ObjectMapper.Dynamic.STRICT) { - throw new StrictDynamicMappingException(parent.fullPath(), paths[i]); + throw new StrictDynamicMappingException(parent.fullPath(), name); } else if (dynamic == ObjectMapper.Dynamic.FALSE) { // Should not dynamically create any more mappers so return the last mapper return new Tuple<>(pathsAdded, parent); + } else if (dynamic == ObjectMapper.Dynamic.RUNTIME) { + mapper = new NoOpObjectMapper(name, currentPath); } else { - //objects are created under properties even with dynamic: runtime, as the runtime section only holds leaf fields - final Mapper fieldMapper = dynamic.getDynamicFieldsBuilder().createDynamicObjectMapper(context, paths[i]); + final Mapper fieldMapper = dynamic.getDynamicFieldsBuilder().createDynamicObjectMapper(context, name); if (fieldMapper instanceof ObjectMapper == false) { assert context.sourceToParse().dynamicTemplates().containsKey(currentPath) : "dynamic templates [" + context.sourceToParse().dynamicTemplates() + "]"; @@ -957,4 +967,10 @@ protected String contentType() { throw new UnsupportedOperationException(); } } + + private static class NoOpObjectMapper extends ObjectMapper { + NoOpObjectMapper(String name, String fullPath) { + super(name, fullPath, new Explicit<>(true, false), Nested.NO, Dynamic.RUNTIME, Collections.emptyMap(), Version.CURRENT); + } + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java index 3d3ef06141c77..8d96423e2a3b8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java @@ -10,10 +10,10 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.CheckedBiConsumer; -import org.elasticsearch.core.CheckedRunnable; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.core.CheckedRunnable; import org.elasticsearch.index.mapper.ObjectMapper.Dynamic; import org.elasticsearch.script.ScriptCompiler; @@ -23,8 +23,9 @@ /** * Encapsulates the logic for dynamically creating fields as part of document parsing. - * Objects are always created the same, but leaf fields can be mapped under properties, as concrete fields that get indexed, + * Fields can be mapped under properties, as concrete fields that get indexed, * or as runtime fields that are evaluated at search-time and have no indexing overhead. + * Objects get dynamically mapped only under dynamic:true. */ final class DynamicFieldsBuilder { private static final Concrete CONCRETE = new Concrete(DocumentParser::parseObjectOrField); @@ -121,10 +122,8 @@ void createDynamicFieldFromValue(final ParseContext context, /** * Returns a dynamically created object mapper, eventually based on a matching dynamic template. - * Note that objects are always mapped under properties. */ Mapper createDynamicObjectMapper(ParseContext context, String name) { - //dynamic:runtime maps objects under properties, exactly like dynamic:true Mapper mapper = createObjectMapperFromTemplate(context, name); return mapper != null ? mapper : new ObjectMapper.Builder(name, context.indexSettings().getIndexVersionCreated()).enabled(true).build(context.path()); @@ -132,7 +131,6 @@ Mapper createDynamicObjectMapper(ParseContext context, String name) { /** * Returns a dynamically created object mapper, based exclusively on a matching dynamic template, null otherwise. - * Note that objects are always mapped under properties. */ Mapper createObjectMapperFromTemplate(ParseContext context, String name) { Mapper.Builder templateBuilder = findTemplateBuilderForObject(context, name); @@ -311,7 +309,7 @@ void newDynamicBinaryField(ParseContext context, String name) throws IOException /** * Dynamically creates runtime fields, in the runtime section. - * Used for leaf fields, when their parent object is mapped as dynamic:runtime. + * Used for sub-fields of objects that are mapped as dynamic:runtime. * @see Dynamic */ private static final class Runtime implements Strategy { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index 787e1c095a6ce..a232961faa380 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -556,8 +556,7 @@ public void testPropagateDynamicRuntimeWithDynamicMapper() throws Exception { })); assertNull(doc.rootDoc().getField("foo.bar.baz")); assertEquals("{\"_doc\":{\"dynamic\":\"false\"," + - "\"runtime\":{\"foo.bar.baz\":{\"type\":\"keyword\"},\"foo.baz\":{\"type\":\"keyword\"}}," + - "\"properties\":{\"foo\":{\"dynamic\":\"runtime\",\"properties\":{\"bar\":{\"type\":\"object\"}}}}}}", + "\"runtime\":{\"foo.bar.baz\":{\"type\":\"keyword\"},\"foo.baz\":{\"type\":\"keyword\"}}}}", Strings.toString(doc.dynamicMappingsUpdate())); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index 7960cb73289d6..aa2781634a050 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -7,11 +7,11 @@ */ package org.elasticsearch.index.mapper; -import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.core.CheckedConsumer; import java.io.IOException; import java.time.Instant; @@ -302,8 +302,7 @@ public void testDynamicRuntimeFieldWithinObject() throws Exception { })); assertEquals("{\"_doc\":{\"dynamic\":\"runtime\"," + - "\"runtime\":{\"foo.bar.baz\":{\"type\":\"long\"}}," + - "\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"type\":\"object\"}}}}}}", + "\"runtime\":{\"foo.bar.baz\":{\"type\":\"long\"}}}}", Strings.toString(doc.dynamicMappingsUpdate())); } @@ -326,8 +325,7 @@ public void testDynamicRuntimeMappingDynamicObject() throws Exception { assertEquals("{\"_doc\":{\"dynamic\":\"runtime\"," + "\"runtime\":{\"object.foo.bar.baz\":{\"type\":\"long\"}}," + "\"properties\":{\"dynamic_object\":{\"dynamic\":\"true\"," + - "\"properties\":{\"foo\":{" + "\"properties\":{\"bar\":{" + "\"properties\":{\"baz\":" + "{\"type\":\"long\"}}}}}}}," + - "\"object\":{\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"type\":\"object\"}}}}}}}}", + "\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"properties\":{\"baz\":{\"type\":\"long\"}}}}}}}}}}", Strings.toString(doc.dynamicMappingsUpdate())); } @@ -350,8 +348,7 @@ public void testDynamicMappingDynamicRuntimeObject() throws Exception { assertEquals("{\"_doc\":{\"dynamic\":\"true\",\"" + "runtime\":{\"runtime_object.foo.bar.baz\":{\"type\":\"keyword\"}}," + "\"properties\":{\"object\":{\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"properties\":{" + - "\"baz\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\",\"ignore_above\":256}}}}}}}}}," + - "\"runtime_object\":{\"dynamic\":\"runtime\",\"properties\":{\"foo\":{\"properties\":{\"bar\":{\"type\":\"object\"}}}}}}}}", + "\"baz\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\",\"ignore_above\":256}}}}}}}}}}}}", Strings.toString(doc.dynamicMappingsUpdate())); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicRuntimeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicRuntimeTests.java index 31ecd33dada0a..6f337f8774cb3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicRuntimeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicRuntimeTests.java @@ -9,10 +9,6 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.Strings; -import org.elasticsearch.index.mapper.DocumentMapper; -import org.elasticsearch.index.mapper.MapperServiceTestCase; -import org.elasticsearch.index.mapper.ObjectMapper; -import org.elasticsearch.index.mapper.ParsedDocument; import java.io.IOException; @@ -72,7 +68,7 @@ public void testWithObjects() throws IOException { "{\"_doc\":{\"dynamic\":\"false\"," + "\"runtime\":{\"dynamic_runtime.child.field4\":{\"type\":\"keyword\"}," + "\"dynamic_runtime.field3\":{\"type\":\"keyword\"}}," - + "\"properties\":{\"dynamic_runtime\":{\"dynamic\":\"runtime\",\"properties\":{\"child\":{\"type\":\"object\"}}}," + + "\"properties\":{" + "\"dynamic_true\":{\"dynamic\":\"true\",\"properties\":{\"child\":{\"properties\":{" + "\"field2\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\",\"ignore_above\":256}}}}}," + "\"field1\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\",\"ignore_above\":256}}}}}}}}", @@ -109,4 +105,21 @@ public void testWithDynamicTemplate() throws IOException { Strings.toString(parsedDoc.dynamicMappingsUpdate()) ); } + + public void testDotsInFieldNames() throws IOException { + DocumentMapper documentMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", ObjectMapper.Dynamic.RUNTIME))); + ParsedDocument doc = documentMapper.parse(source(b -> { + b.field("one.two.three.four", "1234"); + b.field("one.two.three", 123); + b.array("one.two", 1.2, 1.2, 1.2); + b.field("one", "one"); + })); + assertEquals("{\"_doc\":{\"dynamic\":\"runtime\",\"runtime\":{" + + "\"one\":{\"type\":\"keyword\"}," + + "\"one.two\":{\"type\":\"double\"}," + + "\"one.two.three\":{\"type\":\"long\"}," + + "\"one.two.three.four\":{\"type\":\"keyword\"}}}}", + Strings.toString(doc.dynamicMappingsUpdate()) + ); + } }