Skip to content

Commit

Permalink
Refactor how to determine if a field is metafield (#57378) (#57771)
Browse files Browse the repository at this point in the history
Before to determine if a field is meta-field, a static method of MapperService
isMetadataField was used. This method was using an outdated static list
of meta-fields.

This PR instead changes this method to the instance method that
is also aware of meta-fields in all registered plugins.

Related #38373, #41656
Closes #24422
  • Loading branch information
mayya-sharipova committed Jun 8, 2020
1 parent 663702e commit 70e63a3
Show file tree
Hide file tree
Showing 32 changed files with 232 additions and 295 deletions.
Expand Up @@ -20,8 +20,10 @@
package org.elasticsearch.index.mapper;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESSingleNodeTestCase;
Expand All @@ -44,7 +46,7 @@ public void setup() {
protected Collection<Class<? extends Plugin>> getPlugins() {
return pluginList(MapperExtrasPlugin.class);
}

public void testBasics() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties").startObject("field").field("type", "rank_feature").endObject().endObject()
Expand All @@ -55,4 +57,18 @@ public void testBasics() throws Exception {
assertEquals(mapping, mapper.mappingSource().toString());
assertNotNull(mapper.metadataMapper(RankFeatureMetaFieldMapper.class));
}

/**
* Check that meta-fields are picked from plugins (in this case MapperExtrasPlugin),
* and parsing of a document fails if the document contains these meta-fields.
*/
public void testDocumentParsingFailsOnMetaField() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("_doc").endObject().endObject());
DocumentMapper mapper = parser.parse("_doc", new CompressedXContent(mapping));
String rfMetaField = RankFeatureMetaFieldMapper.CONTENT_TYPE;
BytesReference bytes = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().field(rfMetaField, 0).endObject());
MapperParsingException e = expectThrows(MapperParsingException.class, () ->
mapper.parse(new SourceToParse("test", "_doc", "1", bytes, XContentType.JSON)));
assertTrue(e.getMessage().contains("Field ["+ rfMetaField + "] is a metadata field and cannot be added inside a document."));
}
}
Expand Up @@ -39,9 +39,7 @@

import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

Expand Down Expand Up @@ -104,13 +102,9 @@ static void innerHitsExecute(Query mainQuery, IndexSearcher indexSearcher, Searc
continue;
}

Map<String, DocumentField> fields = hit.fieldsOrNull();
if (fields == null) {
fields = new HashMap<>();
hit.fields(fields);
}
IntStream slots = convertTopDocsToSlots(topDocs, rootDocsBySlot);
hit.setField(fieldName, new DocumentField(fieldName, slots.boxed().collect(Collectors.toList())));
// _percolator_document_slot fields are document fields and should be under "fields" section in a hit
hit.setDocumentField(fieldName, new DocumentField(fieldName, slots.boxed().collect(Collectors.toList())));
}
}
}
Expand Down
Expand Up @@ -34,7 +34,6 @@
import java.util.OptionalInt;

import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;

public class RatedSearchHitTests extends ESTestCase {

Expand Down Expand Up @@ -76,8 +75,7 @@ public void testXContentRoundtrip() throws IOException {
RatedSearchHit testItem = randomRatedSearchHit();
XContentType xContentType = randomFrom(XContentType.values());
BytesReference originalBytes = toShuffledXContent(testItem, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
BytesReference withRandomFields = insertRandomFields(xContentType, originalBytes, null, random());
try (XContentParser parser = createParser(xContentType.xContent(), withRandomFields)) {
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
RatedSearchHit parsedItem = RatedSearchHit.parse(parser);
assertNotSame(testItem, parsedItem);
assertEquals(testItem, parsedItem);
Expand Down
Expand Up @@ -596,14 +596,12 @@ public void testGetFieldsComplexField() throws Exception {
String field = "field1.field2.field3.field4";
GetResponse getResponse = client().prepareGet("my-index", "my-type", "1").setStoredFields(field).get();
assertThat(getResponse.isExists(), equalTo(true));
assertThat(getResponse.getField(field).isMetadataField(), equalTo(false));
assertThat(getResponse.getField(field).getValues().size(), equalTo(2));
assertThat(getResponse.getField(field).getValues().get(0).toString(), equalTo("value1"));
assertThat(getResponse.getField(field).getValues().get(1).toString(), equalTo("value2"));

getResponse = client().prepareGet("my-index", "my-type", "1").setStoredFields(field).get();
assertThat(getResponse.isExists(), equalTo(true));
assertThat(getResponse.getField(field).isMetadataField(), equalTo(false));
assertThat(getResponse.getField(field).getValues().size(), equalTo(2));
assertThat(getResponse.getField(field).getValues().get(0).toString(), equalTo("value1"));
assertThat(getResponse.getField(field).getValues().get(1).toString(), equalTo("value2"));
Expand Down Expand Up @@ -631,7 +629,6 @@ public void testGetFieldsComplexField() throws Exception {

getResponse = client().prepareGet("my-index", "my-type", "1").setStoredFields(field).get();
assertThat(getResponse.isExists(), equalTo(true));
assertThat(getResponse.getField(field).isMetadataField(), equalTo(false));
assertThat(getResponse.getField(field).getValues().size(), equalTo(2));
assertThat(getResponse.getField(field).getValues().get(0).toString(), equalTo("value1"));
assertThat(getResponse.getField(field).getValues().get(1).toString(), equalTo("value2"));
Expand Down
Expand Up @@ -63,9 +63,7 @@ public void testGetFieldsMetadataWithRouting() throws Exception {
.setStoredFields("field1")
.get();
assertThat(getResponse.isExists(), equalTo(true));
assertThat(getResponse.getField("field1").isMetadataField(), equalTo(false));
assertThat(getResponse.getField("field1").getValue().toString(), equalTo("value"));
assertThat(getResponse.getField("_routing").isMetadataField(), equalTo(true));
assertThat(getResponse.getField("_routing").getValue().toString(), equalTo("1"));
}

Expand All @@ -78,9 +76,7 @@ public void testGetFieldsMetadataWithRouting() throws Exception {
.setRouting("1")
.get();
assertThat(getResponse.isExists(), equalTo(true));
assertThat(getResponse.getField("field1").isMetadataField(), equalTo(false));
assertThat(getResponse.getField("field1").getValue().toString(), equalTo("value"));
assertThat(getResponse.getField("_routing").isMetadataField(), equalTo(true));
assertThat(getResponse.getField("_routing").getValue().toString(), equalTo("1"));
}
}
Expand Down
Expand Up @@ -125,13 +125,10 @@ public void hitExecute(SearchContext context, HitContext hitContext) {
return;
}
String field = fetchSubPhaseBuilder.getField();
if (hitContext.hit().fieldsOrNull() == null) {
hitContext.hit().fields(new HashMap<>());
}
DocumentField hitField = hitContext.hit().getFields().get(NAME);
if (hitField == null) {
hitField = new DocumentField(NAME, new ArrayList<>(1));
hitContext.hit().setField(NAME, hitField);
hitContext.hit().setDocumentField(NAME, hitField);
}
TermVectorsRequest termVectorsRequest = new TermVectorsRequest(context.indexShard().shardId().getIndex().getName(),
hitContext.hit().getType(), hitContext.hit().getId());
Expand Down
Expand Up @@ -660,7 +660,6 @@ public void testSearchFieldsMetadata() throws Exception {

assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));
assertThat(searchResponse.getHits().getAt(0).field("field1"), nullValue());
assertThat(searchResponse.getHits().getAt(0).field("_routing").isMetadataField(), equalTo(true));
assertThat(searchResponse.getHits().getAt(0).field("_routing").getValue().toString(), equalTo("1"));
}

Expand Down Expand Up @@ -736,7 +735,6 @@ public void testGetFieldsComplexField() throws Exception {

SearchResponse searchResponse = client().prepareSearch("my-index").addStoredField(field).get();
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));
assertThat(searchResponse.getHits().getAt(0).field(field).isMetadataField(), equalTo(false));
assertThat(searchResponse.getHits().getAt(0).field(field).getValues().size(), equalTo(2));
assertThat(searchResponse.getHits().getAt(0).field(field).getValues().get(0).toString(), equalTo("value1"));
assertThat(searchResponse.getHits().getAt(0).field(field).getValues().get(1).toString(), equalTo("value2"));
Expand Down Expand Up @@ -1223,7 +1221,6 @@ public void testLoadMetadata() throws Exception {
Map<String, DocumentField> fields = response.getHits().getAt(0).getFields();

assertThat(fields.get("field1"), nullValue());
assertThat(fields.get("_routing").isMetadataField(), equalTo(true));
assertThat(fields.get("_routing").getValue().toString(), equalTo("1"));
}
}
Expand Up @@ -26,7 +26,6 @@
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.get.GetResult;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.search.SearchHit;

import java.io.IOException;
Expand Down Expand Up @@ -87,13 +86,6 @@ public List<Object> getValues() {
return values;
}

/**
* @return The field is a metadata field
*/
public boolean isMetadataField() {
return MapperService.isMetadataField(name);
}

@Override
public Iterator<Object> iterator() {
return values.iterator();
Expand Down
28 changes: 8 additions & 20 deletions server/src/main/java/org/elasticsearch/index/get/GetResult.java
Expand Up @@ -33,6 +33,7 @@
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.IgnoredFieldMapper;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.SourceFieldMapper;
import org.elasticsearch.search.lookup.SourceLookup;

Expand Down Expand Up @@ -97,7 +98,8 @@ public GetResult(StreamInput in) throws IOException {
Map<String, DocumentField> fields = readFields(in);
documentFields = new HashMap<>();
metaFields = new HashMap<>();
splitFieldsByMetadata(fields, documentFields, metaFields);
fields.forEach((fieldName, docField) ->
(MapperService.isMetadataFieldStatic(fieldName) ? metaFields : documentFields).put(fieldName, docField));
}
} else {
metaFields = Collections.emptyMap();
Expand Down Expand Up @@ -363,7 +365,7 @@ public static GetResult fromXContentEmbedded(XContentParser parser, String index
} else if (FOUND.equals(currentFieldName)) {
found = parser.booleanValue();
} else {
metaFields.put(currentFieldName, new DocumentField(currentFieldName,
metaFields.put(currentFieldName, new DocumentField(currentFieldName,
Collections.singletonList(parser.objectText())));
}
} else if (token == XContentParser.Token.START_OBJECT) {
Expand Down Expand Up @@ -401,10 +403,10 @@ public static GetResult fromXContent(XContentParser parser) throws IOException {
}

private Map<String, DocumentField> readFields(StreamInput in) throws IOException {
Map<String, DocumentField> fields = null;
Map<String, DocumentField> fields;
int size = in.readVInt();
if (size == 0) {
fields = new HashMap<>();
fields = emptyMap();
} else {
fields = new HashMap<>(size);
for (int i = 0; i < size; i++) {
Expand All @@ -414,20 +416,6 @@ private Map<String, DocumentField> readFields(StreamInput in) throws IOException
}
return fields;
}

static void splitFieldsByMetadata(Map<String, DocumentField> fields, Map<String, DocumentField> outOther,
Map<String, DocumentField> outMetadata) {
if (fields == null) {
return;
}
for (Map.Entry<String, DocumentField> fieldEntry: fields.entrySet()) {
if (fieldEntry.getValue().isMetadataField()) {
outMetadata.put(fieldEntry.getKey(), fieldEntry.getValue());
} else {
outOther.put(fieldEntry.getKey(), fieldEntry.getValue());
}
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
Expand All @@ -446,11 +434,11 @@ public void writeTo(StreamOutput out) throws IOException {
writeFields(out, documentFields);
writeFields(out, metaFields);
} else {
writeFields(out, this.getFields());
writeFields(out, this.getFields());
}
}
}

private void writeFields(StreamOutput out, Map<String, DocumentField> fields) throws IOException {
if (fields == null) {
out.writeVInt(0);
Expand Down
Expand Up @@ -285,7 +285,7 @@ private GetResult innerGetLoadFromStoredFields(String type, String id, String[]
documentFields = new HashMap<>();
metadataFields = new HashMap<>();
for (Map.Entry<String, List<Object>> entry : fieldVisitor.fields().entrySet()) {
if (MapperService.isMetadataField(entry.getKey())) {
if (mapperService.isMetadataField(entry.getKey())) {
metadataFields.put(entry.getKey(), new DocumentField(entry.getKey(), entry.getValue()));
} else {
documentFields.put(entry.getKey(), new DocumentField(entry.getKey(), entry.getValue()));
Expand Down
Expand Up @@ -404,7 +404,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper,
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
paths = splitAndValidatePath(currentFieldName);
if (MapperService.isMetadataField(context.path().pathAsText(currentFieldName))) {
if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) {
throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside"
+ " a document. Use the index API request parameters.");
} else if (containsDisabledObjectMapper(mapper, paths)) {
Expand Down
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.index.mapper;

import com.carrotsearch.hppc.ObjectHashSet;
import com.carrotsearch.hppc.cursors.ObjectCursor;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.message.ParameterizedMessage;
Expand Down Expand Up @@ -56,14 +55,14 @@
import org.elasticsearch.index.mapper.Mapper.BuilderContext;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.similarity.SimilarityService;
import org.elasticsearch.indices.IndicesModule;
import org.elasticsearch.indices.InvalidTypeNameException;
import org.elasticsearch.indices.mapper.MapperRegistry;
import org.elasticsearch.search.suggest.completion.context.ContextMapping;

import java.io.Closeable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -121,14 +120,6 @@ public enum MergeReason {
Setting.boolSetting("index.mapper.dynamic", INDEX_MAPPER_DYNAMIC_DEFAULT,
Property.Dynamic, Property.IndexScope, Property.Deprecated);

//TODO this needs to be cleaned up: _timestamp and _ttl are not supported anymore, _field_names, _seq_no, _version and _source are
//also missing, not sure if on purpose. See IndicesModule#getMetadataMappers
private static final String[] SORTED_META_FIELDS = new String[]{
"_id", IgnoredFieldMapper.NAME, "_index", "_routing", "_size", "_timestamp", "_ttl", "_type"
};

private static final ObjectHashSet<String> META_FIELDS = ObjectHashSet.from(SORTED_META_FIELDS);

private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(MapperService.class));
static final String DEFAULT_MAPPING_ERROR_MESSAGE = "[_default_] mappings are not allowed on new indices and should no " +
"longer be used. See [https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html" +
Expand All @@ -146,6 +137,7 @@ public enum MergeReason {
private boolean hasNested = false; // updated dynamically to true when a nested object is added

private final DocumentMapperParser documentParser;
private final Version indexVersionCreated;

private final MapperAnalyzerWrapper indexAnalyzer;
private final MapperAnalyzerWrapper searchAnalyzer;
Expand All @@ -161,6 +153,7 @@ public MapperService(IndexSettings indexSettings, IndexAnalyzers indexAnalyzers,
SimilarityService similarityService, MapperRegistry mapperRegistry,
Supplier<QueryShardContext> queryShardContextSupplier, BooleanSupplier idFieldDataEnabled) {
super(indexSettings);
this.indexVersionCreated = indexSettings.getIndexVersionCreated();
this.indexAnalyzers = indexAnalyzers;
this.fieldTypes = new FieldTypeLookup();
this.documentParser = new DocumentMapperParser(indexSettings, this, xContentRegistry, similarityService, mapperRegistry,
Expand Down Expand Up @@ -812,14 +805,27 @@ public void close() throws IOException {
}

/**
* @return Whether a field is a metadata field.
* @return Whether a field is a metadata field
* Deserialization of SearchHit objects sent from pre 7.8 nodes and GetResults objects sent from pre 7.3 nodes,
* uses this method to divide fields into meta and document fields.
* TODO: remove in v 9.0
* @deprecated Use an instance method isMetadataField instead
*/
public static boolean isMetadataField(String fieldName) {
return META_FIELDS.contains(fieldName);
@Deprecated
public static boolean isMetadataFieldStatic(String fieldName) {
if (IndicesModule.getBuiltInMetadataFields().contains(fieldName)) {
return true;
}
// if a node had Size Plugin installed, _size field should also be considered a meta-field
return fieldName.equals("_size");
}

public static String[] getAllMetaFields() {
return Arrays.copyOf(SORTED_META_FIELDS, SORTED_META_FIELDS.length);
/**
* @return Whether a field is a metadata field.
* this method considers all mapper plugins
*/
public boolean isMetadataField(String field) {
return mapperRegistry.isMetadataField(indexVersionCreated, field);
}

/**
Expand Down

0 comments on commit 70e63a3

Please sign in to comment.