From 684e6986279ddbacdacd5470e27eddc25207427e Mon Sep 17 00:00:00 2001 From: Boaz Leskes Date: Wed, 9 Jul 2014 08:25:53 +0200 Subject: [PATCH] [Code] Control whether MapperService docMapper iterator should contain DEFAULT_MAPPING At the moment one can iterate the MapperService to go through all document mappers. This includes the document mapper of DEFAULT_MAPPING, which may be surprising and lead to unintended results. This commit removes the Iterable implementation and add a docMappers method that asks the caller to make an explicit choice Closes #6793 --- .../metadata/MetaDataCreateIndexService.java | 2 +- .../plain/ParentChildIndexFieldData.java | 2 +- .../index/mapper/MapperService.java | 33 ++++++++++++++++--- .../mapper/internal/ParentFieldMapper.java | 4 +-- .../index/query/HasParentFilterParser.java | 2 +- .../index/query/HasParentQueryParser.java | 2 +- .../cluster/IndicesClusterStateService.java | 2 +- .../indices/recovery/RecoverySource.java | 10 ++---- .../elasticsearch/search/SearchService.java | 11 ++++--- .../indices/leaks/IndicesLeaksTests.java | 4 +-- 10 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 9558bb6fcf1..02809c9f93d 100644 --- a/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -372,7 +372,7 @@ public class MetaDataCreateIndexService extends AbstractComponent { // now, update the mappings with the actual source Map mappingsMetaData = Maps.newHashMap(); - for (DocumentMapper mapper : mapperService) { + for (DocumentMapper mapper : mapperService.docMappers(true)) { MappingMetaData mappingMd = new MappingMetaData(mapper); mappingsMetaData.put(mapper.type(), mappingMd); } diff --git a/src/main/java/org/elasticsearch/index/fielddata/plain/ParentChildIndexFieldData.java b/src/main/java/org/elasticsearch/index/fielddata/plain/ParentChildIndexFieldData.java index c606a2bc676..3ee946f7143 100644 --- a/src/main/java/org/elasticsearch/index/fielddata/plain/ParentChildIndexFieldData.java +++ b/src/main/java/org/elasticsearch/index/fielddata/plain/ParentChildIndexFieldData.java @@ -73,7 +73,7 @@ public class ParentChildIndexFieldData extends AbstractIndexFieldData(BytesRef.getUTF8SortedAsUnicodeComparator()); this.breakerService = breakerService; this.globalOrdinalsBuilder = globalOrdinalsBuilder; - for (DocumentMapper documentMapper : mapperService) { + for (DocumentMapper documentMapper : mapperService.docMappers(false)) { beforeCreate(documentMapper); } mapperService.addTypeListener(this); diff --git a/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 9e7acfcaff6..48e195623da 100755 --- a/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -21,9 +21,9 @@ package org.elasticsearch.index.mapper; import com.carrotsearch.hppc.ObjectOpenHashSet; import com.google.common.base.Charsets; +import com.google.common.base.Predicate; import com.google.common.collect.*; import org.apache.lucene.analysis.Analyzer; -import org.apache.lucene.analysis.AnalyzerWrapper; import org.apache.lucene.analysis.SimpleAnalyzerWrapper; import org.apache.lucene.index.Term; import org.apache.lucene.queries.FilterClause; @@ -75,7 +75,7 @@ import static org.elasticsearch.index.mapper.DocumentMapper.MergeFlags.mergeFlag /** * */ -public class MapperService extends AbstractIndexComponent implements Iterable { +public class MapperService extends AbstractIndexComponent { public static final String DEFAULT_MAPPING = "_default_"; private static ObjectOpenHashSet META_FIELDS = ObjectOpenHashSet.from( @@ -226,11 +226,34 @@ public class MapperService extends AbstractIndexComponent implements Iterable iterator() { - return Iterators.unmodifiableIterator(mappers.values().iterator()); + /** + * returns an immutable iterator over current document mappers. + * + * @param includingDefaultMapping indicates whether the iterator should contain the {@link #DEFAULT_MAPPING} document mapper. + * As is this not really an active type, you would typically set this to false + */ + public Iterable docMappers(final boolean includingDefaultMapping) { + return new Iterable() { + @Override + public Iterator iterator() { + final Iterator iterator; + if (includingDefaultMapping) { + iterator = mappers.values().iterator(); + } else { + iterator = Iterators.filter(mappers.values().iterator(), NOT_A_DEFAULT_DOC_MAPPER); + } + return Iterators.unmodifiableIterator(iterator); + } + }; } + private static final Predicate NOT_A_DEFAULT_DOC_MAPPER = new Predicate() { + @Override + public boolean apply(DocumentMapper input) { + return !DEFAULT_MAPPING.equals(input.type()); + } + }; + public AnalysisService analysisService() { return this.analysisService; } diff --git a/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java index 8ba333e84ed..10cf726256c 100644 --- a/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/internal/ParentFieldMapper.java @@ -263,7 +263,7 @@ public class ParentFieldMapper extends AbstractFieldMapper implements Inter } List types = new ArrayList<>(context.mapperService().types().size()); - for (DocumentMapper documentMapper : context.mapperService()) { + for (DocumentMapper documentMapper : context.mapperService().docMappers(false)) { if (!documentMapper.parentFieldMapper().active()) { types.add(documentMapper.type()); } @@ -294,7 +294,7 @@ public class ParentFieldMapper extends AbstractFieldMapper implements Inter } List types = new ArrayList<>(context.mapperService().types().size()); - for (DocumentMapper documentMapper : context.mapperService()) { + for (DocumentMapper documentMapper : context.mapperService().docMappers(false)) { if (!documentMapper.parentFieldMapper().active()) { types.add(documentMapper.type()); } diff --git a/src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java b/src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java index 8ae63441be2..463f2481bae 100644 --- a/src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java @@ -132,7 +132,7 @@ public class HasParentFilterParser implements FilterParser { Set parentTypes = new HashSet<>(5); parentTypes.add(parentType); ParentChildIndexFieldData parentChildIndexFieldData = null; - for (DocumentMapper documentMapper : parseContext.mapperService()) { + for (DocumentMapper documentMapper : parseContext.mapperService().docMappers(false)) { ParentFieldMapper parentFieldMapper = documentMapper.parentFieldMapper(); if (parentFieldMapper.active()) { DocumentMapper parentTypeDocumentMapper = parseContext.mapperService().documentMapper(parentFieldMapper.type()); diff --git a/src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java b/src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java index 4f3f2c080bf..2609b640c0e 100644 --- a/src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java +++ b/src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java @@ -135,7 +135,7 @@ public class HasParentQueryParser implements QueryParser { ParentChildIndexFieldData parentChildIndexFieldData = null; Set parentTypes = new HashSet<>(5); parentTypes.add(parentType); - for (DocumentMapper documentMapper : parseContext.mapperService()) { + for (DocumentMapper documentMapper : parseContext.mapperService().docMappers(false)) { ParentFieldMapper parentFieldMapper = documentMapper.parentFieldMapper(); if (parentFieldMapper.active()) { parentChildIndexFieldData = parseContext.getForField(parentFieldMapper); diff --git a/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java b/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java index 83397be6dc2..7c1e1646f40 100644 --- a/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java +++ b/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java @@ -374,7 +374,7 @@ public class IndicesClusterStateService extends AbstractLifecycleComponent(index, documentMapper.type())) && !indexMetaData.mappings().containsKey(documentMapper.type())) { // we have it in our mappings, but not in the metadata, and we have seen it in the cluster state, remove it mapperService.remove(documentMapper.type()); diff --git a/src/main/java/org/elasticsearch/indices/recovery/RecoverySource.java b/src/main/java/org/elasticsearch/indices/recovery/RecoverySource.java index 2515af8da62..b0754f0abac 100644 --- a/src/main/java/org/elasticsearch/indices/recovery/RecoverySource.java +++ b/src/main/java/org/elasticsearch/indices/recovery/RecoverySource.java @@ -43,7 +43,6 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.index.deletionpolicy.SnapshotIndexCommit; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.mapper.DocumentMapper; -import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.service.IndexService; import org.elasticsearch.index.shard.IllegalIndexShardStateException; import org.elasticsearch.index.shard.IndexShardClosedException; @@ -289,12 +288,9 @@ public class RecoverySource extends AbstractComponent { metaDataMappings = indexMetaData.getMappings(); } List documentMappersToUpdate = Lists.newArrayList(); - for (DocumentMapper documentMapper : indexService.mapperService()) { - // default mapping should not be sent back, it can only be updated by put mapping API, and its - // a full in place replace, we don't want to override a potential update coming it - if (documentMapper.type().equals(MapperService.DEFAULT_MAPPING)) { - continue; - } + // default mapping should not be sent back, it can only be updated by put mapping API, and its + // a full in place replace, we don't want to override a potential update coming it + for (DocumentMapper documentMapper : indexService.mapperService().docMappers(false)) { MappingMetaData mappingMetaData = metaDataMappings == null ? null : metaDataMappings.get(documentMapper.type()); if (mappingMetaData == null || !documentMapper.refreshSource().equals(mappingMetaData.source())) { diff --git a/src/main/java/org/elasticsearch/search/SearchService.java b/src/main/java/org/elasticsearch/search/SearchService.java index 7b17827b2a6..b4704325d60 100644 --- a/src/main/java/org/elasticsearch/search/SearchService.java +++ b/src/main/java/org/elasticsearch/search/SearchService.java @@ -69,8 +69,11 @@ import org.elasticsearch.search.dfs.CachedDfSource; import org.elasticsearch.search.dfs.DfsPhase; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.*; -import org.elasticsearch.search.internal.*; +import org.elasticsearch.search.internal.DefaultSearchContext; +import org.elasticsearch.search.internal.InternalScrollSearchRequest; +import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.internal.SearchContext.Lifetime; +import org.elasticsearch.search.internal.ShardSearchRequest; import org.elasticsearch.search.query.*; import org.elasticsearch.search.warmer.IndexWarmersMetaData; import org.elasticsearch.threadpool.ThreadPool; @@ -721,7 +724,7 @@ public class SearchService extends AbstractLifecycleComponent { final Loading defaultLoading = Loading.parse(indexMetaData.settings().get(NORMS_LOADING_KEY), Loading.LAZY); final MapperService mapperService = indexShard.mapperService(); final ObjectSet warmUp = new ObjectOpenHashSet<>(); - for (DocumentMapper docMapper : mapperService) { + for (DocumentMapper docMapper : mapperService.docMappers(false)) { for (FieldMapper fieldMapper : docMapper.mappers().mappers()) { final String indexName = fieldMapper.names().indexName(); if (fieldMapper.fieldType().indexed() && !fieldMapper.fieldType().omitNorms() && fieldMapper.normsLoading(defaultLoading) == Loading.EAGER) { @@ -772,7 +775,7 @@ public class SearchService extends AbstractLifecycleComponent { public TerminationHandle warm(final IndexShard indexShard, IndexMetaData indexMetaData, final WarmerContext context, ThreadPool threadPool) { final MapperService mapperService = indexShard.mapperService(); final Map> warmUp = new HashMap<>(); - for (DocumentMapper docMapper : mapperService) { + for (DocumentMapper docMapper : mapperService.docMappers(false)) { for (FieldMapper fieldMapper : docMapper.mappers().mappers()) { final FieldDataType fieldDataType = fieldMapper.fieldDataType(); if (fieldDataType == null) { @@ -826,7 +829,7 @@ public class SearchService extends AbstractLifecycleComponent { public TerminationHandle warmTop(final IndexShard indexShard, IndexMetaData indexMetaData, final WarmerContext context, ThreadPool threadPool) { final MapperService mapperService = indexShard.mapperService(); final Map> warmUpGlobalOrdinals = new HashMap<>(); - for (DocumentMapper docMapper : mapperService) { + for (DocumentMapper docMapper : mapperService.docMappers(false)) { for (FieldMapper fieldMapper : docMapper.mappers().mappers()) { final FieldDataType fieldDataType = fieldMapper.fieldDataType(); if (fieldDataType == null) { diff --git a/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java b/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java index 64b0a4b4e60..be9f92d3549 100644 --- a/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java +++ b/src/test/java/org/elasticsearch/indices/leaks/IndicesLeaksTests.java @@ -34,7 +34,7 @@ import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.List; -import static org.elasticsearch.test.ElasticsearchIntegrationTest.*; +import static org.elasticsearch.test.ElasticsearchIntegrationTest.Scope; import static org.hamcrest.Matchers.nullValue; /** @@ -72,7 +72,7 @@ public class IndicesLeaksTests extends ElasticsearchIntegrationTest { indexReferences.add(new WeakReference(indexService)); indexReferences.add(new WeakReference(indexInjector)); indexReferences.add(new WeakReference(indexService.mapperService())); - for (DocumentMapper documentMapper : indexService.mapperService()) { + for (DocumentMapper documentMapper : indexService.mapperService().docMappers(true)) { indexReferences.add(new WeakReference(documentMapper)); } indexReferences.add(new WeakReference(indexService.aliasesService()));