Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor how to determine if a field is metafield #57378

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,14 @@ public void testBasics() throws Exception {
assertEquals(mapping, mapper.mappingSource().toString());
assertNotNull(mapper.metadataMapper(RankFeatureMetaFieldMapper.class));
}

public void testDocumentParsingFailsOnMetaField() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great way to test!
nit: could you add a very brief comment explaining why this proves the metadata field from the plugin was correctly registered? Makes it easier to later track why tests are there and what they check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbuescher Thank you for the review.
The last comment addressed in ac6a4bc

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