From 8ef29041569c14d13320bfa125f2b7127dd697eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 11 Oct 2021 17:33:58 +0200 Subject: [PATCH] Add ability to retrieve _id via fields option Currently we exclude metadata fields from being looked up using the `fields` option in search. However, as issue like #75836 show, they can still be retrieved e.g. via aliases and then fetching their values causes errors. With this change, we enable retrieval of metadata fields (like `_id`) using the fields option when the field is explicitely asked for. We still continue to exclude any metadata field from matching wildcard patterns, but they should be retrievable via an exact name or if there is an alias definition with a path to a metadata field. This change adds support for the `_id` field in particular. --- .../test/search/330_fetch_fields.yml | 55 +++++++++++++++++++ .../index/mapper/IdFieldMapper.java | 2 +- .../index/mapper/StoredValueFetcher.java | 44 +++++++++++++++ .../index/mapper/IdFieldMapperTests.java | 28 ++++++++++ .../fetch/subphase/FieldFetcherTests.java | 2 +- .../index/mapper/MapperServiceTestCase.java | 12 +++- .../index/mapper/MapperTestCase.java | 11 +--- 7 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/330_fetch_fields.yml index 1d156a4eb98fe..39ebbcc76affe 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -1007,3 +1007,58 @@ error for flattened includes whole path: fields: - field: flattened.bar format: "yyyy/MM/dd" + +--- +test fetching metadata fields: + - skip: + version: ' - 7.15.99' + reason: 'fetching metadata via fields introduced in 7.16' + + - do: + indices.create: + index: test + body: + settings: + index.number_of_shards: 1 + mappings: + properties: + field: + type: keyword + idAlias: + type: alias + path: _id + + - do: + index: + index: test + id: 1 + refresh: true + body: + field: foo + + - do: + search: + index: test + body: + fields: [ "*" ] + + - length: { hits.hits.0.fields : 2 } + - match: { hits.hits.0.fields.field.0: "foo" } + - match: { hits.hits.0.fields.idAlias.0: "1" } + + - do: + search: + index: test + body: + fields: [ "_*" ] + + - is_false: hits.hits.0.fields + + - do: + search: + index: test + body: + fields: [ "_id" ] + + - length: { hits.hits.0.fields : 1 } + - match: { hits.hits.0.fields._id.0: "1" } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index d5f634c0a72ed..32c8cef54642e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -109,7 +109,7 @@ public boolean isSearchable() { @Override public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { - throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "]."); + return new StoredValueFetcher(context.lookup(), NAME); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java new file mode 100644 index 0000000000000..77ceae54e6297 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.index.mapper; + +import org.apache.lucene.index.LeafReaderContext; +import org.elasticsearch.search.lookup.LeafSearchLookup; +import org.elasticsearch.search.lookup.SearchLookup; +import org.elasticsearch.search.lookup.SourceLookup; + +import java.io.IOException; +import java.util.List; + +/** + * Value fetcher that loads from stored values. + */ +public final class StoredValueFetcher implements ValueFetcher { + + private final SearchLookup lookup; + private LeafSearchLookup leafSearchLookup; + private final String fieldname; + + public StoredValueFetcher(SearchLookup lookup, String fieldname) { + this.lookup = lookup; + this.fieldname = fieldname; + } + + @Override + public void setNextReader(LeafReaderContext context) { + this.leafSearchLookup = lookup.getLeafSearchLookup(context); + } + + @Override + public List fetchValues(SourceLookup lookup) throws IOException { + leafSearchLookup.setDocument(lookup.docId()); + return leafSearchLookup.fields().get(fieldname).getValues(); + } + +} diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java index eef5af5ed4224..fb05e4dbffc7a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java @@ -10,12 +10,19 @@ import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.IndexableField; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.IndexSearcher; +import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.indices.IndicesService; +import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; +import java.util.Collections; import static org.elasticsearch.index.mapper.IdFieldMapper.ID_FIELD_DATA_DEPRECATION_MESSAGE; import static org.hamcrest.Matchers.containsString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class IdFieldMapperTests extends MapperServiceTestCase { @@ -60,4 +67,25 @@ public void testEnableFieldData() throws IOException { assertWarnings(ID_FIELD_DATA_DEPRECATION_MESSAGE); assertTrue(ft.isAggregatable()); } + + public void testFetchIdFieldValue() throws IOException { + MapperService mapperService = createMapperService( + fieldMapping(b -> b.field("type", "keyword")) + ); + String id = randomAlphaOfLength(12); + withLuceneIndex(mapperService, iw -> { + iw.addDocument(mapperService.documentMapper().parse(source(id, b -> b.field("field", "value"), null)).rootDoc()); + }, iw -> { + SearchLookup lookup = new SearchLookup(mapperService::fieldType, fieldDataLookup()); + SearchExecutionContext searchExecutionContext = mock(SearchExecutionContext.class); + when(searchExecutionContext.lookup()).thenReturn(lookup); + IdFieldMapper.IdFieldType ft = (IdFieldMapper.IdFieldType) mapperService.fieldType("_id"); + ValueFetcher valueFetcher = ft.valueFetcher(searchExecutionContext, null); + IndexSearcher searcher = newSearcher(iw); + LeafReaderContext context = searcher.getIndexReader().leaves().get(0); + lookup.source().setSegmentAndDocument(context, 0); + valueFetcher.setNextReader(context); + assertEquals(Collections.singletonList(id), valueFetcher.fetchValues(lookup.source())); + }); + } } diff --git a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java index 354c1a45ffc87..087bb31a4188c 100644 --- a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java +++ b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java @@ -190,7 +190,7 @@ public void testMetadataFields() throws IOException { assertEquals(100, ((Integer) fields.get("_doc_count").getValue()).intValue()); // several other metadata fields throw exceptions via their value fetchers when trying to get them - for (String fieldname : new String[] { "_id", "_index", "_seq_no", "_routing", "_ignored" }) { + for (String fieldname : new String[] { "_index", "_seq_no", "_routing", "_ignored" }) { expectThrows(UnsupportedOperationException.class, () -> fetchFields(mapperService, source, fieldname)); } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index 01c20ea78209d..51c537f05906c 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -18,8 +18,6 @@ import org.apache.lucene.store.Directory; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetadata; -import org.elasticsearch.core.CheckedConsumer; -import org.elasticsearch.core.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.bytes.BytesArray; @@ -34,6 +32,8 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.core.CheckedConsumer; +import org.elasticsearch.core.Nullable; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.AnalyzerScope; import org.elasticsearch.index.analysis.IndexAnalyzers; @@ -72,8 +72,10 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.BooleanSupplier; import java.util.function.Function; +import java.util.function.Supplier; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; @@ -570,4 +572,10 @@ protected SearchExecutionContext createSearchExecutionContext(MapperService mapp return searchExecutionContext; } + + protected BiFunction, IndexFieldData> fieldDataLookup() { + return (mft, lookupSource) -> mft + .fielddataBuilder("test", lookupSource) + .build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService()); + } } 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 bda170c2d7118..8c7219d524717 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 @@ -19,15 +19,14 @@ import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; import org.apache.lucene.util.SetOnce; -import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.core.Set; -import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldDataCache; import org.elasticsearch.index.fielddata.ScriptDocValues; import org.elasticsearch.index.query.SearchExecutionContext; @@ -43,10 +42,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.function.BiFunction; import java.util.function.Consumer; import java.util.function.Function; -import java.util.function.Supplier; import java.util.stream.Collectors; import static java.util.stream.Collectors.toList; @@ -726,12 +723,6 @@ public final void testIndexTimeStoredFieldsAccess() throws IOException { }); } - private BiFunction, IndexFieldData> fieldDataLookup() { - return (mft, lookupSource) -> mft - .fielddataBuilder("test", lookupSource) - .build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService()); - } - public final void testNullInput() throws Exception { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); if (allowsNullValues()) {