Skip to content

Commit

Permalink
Refactor how to determine if a field is metafield (#57378)
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 authored Jun 5, 2020
1 parent 9d5409a commit b68bd78
Show file tree
Hide file tree
Showing 31 changed files with 227 additions and 290 deletions.
Original file line number Diff line number Diff line change
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", "1", bytes, XContentType.JSON)));
assertTrue(e.getMessage().contains("Field ["+ rfMetaField + "] is a metadata field and cannot be added inside a document."));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,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 @@ -101,13 +99,9 @@ static void innerHitsExecute(Query mainQuery,
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,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 @@ -74,8 +73,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
Original file line number Diff line number Diff line change
Expand Up @@ -594,14 +594,12 @@ public void testGetFieldsComplexField() throws Exception {
String field = "field1.field2.field3.field4";
GetResponse getResponse = client().prepareGet("my-index", "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", "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 @@ -629,7 +627,6 @@ public void testGetFieldsComplexField() throws Exception {

getResponse = client().prepareGet("my-index", "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
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,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().getId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,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 @@ -735,7 +734,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 @@ -1187,7 +1185,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"));
}
}
Original file line number Diff line number Diff line change
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 @@ -88,13 +87,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
21 changes: 4 additions & 17 deletions server/src/main/java/org/elasticsearch/index/get/GetResult.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,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 @@ -386,10 +387,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 @@ -400,20 +401,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 {
out.writeString(index);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ private GetResult innerGetLoadFromStoredFields(String id, String[] storedFields,
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
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,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
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@

package org.elasticsearch.index.mapper;

import com.carrotsearch.hppc.ObjectHashSet;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.DelegatingAnalyzerWrapper;
import org.elasticsearch.Assertions;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.MappingMetadata;
import org.elasticsearch.common.Strings;
Expand All @@ -50,14 +50,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 @@ -107,14 +107,6 @@ public enum MergeReason {
public static final Setting<Long> INDEX_MAPPING_FIELD_NAME_LENGTH_LIMIT_SETTING =
Setting.longSetting("index.mapping.field_name_length.limit", Long.MAX_VALUE, 1L, Property.Dynamic, Property.IndexScope);

//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", "_nested_path", "_routing", "_size", "_timestamp", "_ttl", "_type"
};

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

private final IndexAnalyzers indexAnalyzers;

private volatile DocumentMapper mapper;
Expand All @@ -124,6 +116,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 @@ -139,6 +132,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 @@ -640,14 +634,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);
}

/** An analyzer wrapper that can lookup fields within the index mappings */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ public static Map<String, Mapper.TypeParser> getMappers(List<MapperPlugin> mappe

private static final Map<String, MetadataFieldMapper.TypeParser> builtInMetadataMappers = initBuiltInMetadataMappers();

private static Set<String> builtInMetadataFields = Collections.unmodifiableSet(builtInMetadataMappers.keySet());

private static Map<String, MetadataFieldMapper.TypeParser> initBuiltInMetadataMappers() {
Map<String, MetadataFieldMapper.TypeParser> builtInMetadataMappers;
// Use a LinkedHashMap for metadataMappers because iteration order matters
Expand Down Expand Up @@ -198,7 +200,7 @@ public static Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers(Lis
* Returns a set containing all of the builtin metadata fields
*/
public static Set<String> getBuiltInMetadataFields() {
return builtInMetadataMappers.keySet();
return builtInMetadataFields;
}

private static Function<String, Predicate<String>> getFieldFilter(List<MapperPlugin> mapperPlugins) {
Expand Down
Loading

0 comments on commit b68bd78

Please sign in to comment.