From 5ea6c77dad00b90ed73779a4d936cc61c6fa0a3f Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Thu, 30 May 2013 10:24:34 +0200 Subject: [PATCH] Highlighting shouldn't fail when the field to highlight is absent. PlainHighlighter fails with a NPE when the field to highlight is marked as stored in the mapping but doesn't exist in a hit. This patch makes FieldsVisitor.fields less error-prone by returning an empty list instead of null when no matching stored field was found. Closes #3109 --- .../index/fieldvisitor/FieldsVisitor.java | 14 ++++----- .../index/get/ShardGetService.java | 3 +- .../search/fetch/FetchPhase.java | 2 +- .../search/highlight/PlainHighlighter.java | 11 +++++-- .../highlight/HighlighterSearchTests.java | 30 +++++++++++++++++++ 5 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/fieldvisitor/FieldsVisitor.java b/src/main/java/org/elasticsearch/index/fieldvisitor/FieldsVisitor.java index 271813968be..17872633aec 100644 --- a/src/main/java/org/elasticsearch/index/fieldvisitor/FieldsVisitor.java +++ b/src/main/java/org/elasticsearch/index/fieldvisitor/FieldsVisitor.java @@ -19,6 +19,7 @@ package org.elasticsearch.index.fieldvisitor; +import com.google.common.collect.ImmutableMap; import org.apache.lucene.index.FieldInfo; import org.apache.lucene.index.StoredFieldVisitor; import org.apache.lucene.util.BytesRef; @@ -44,9 +45,6 @@ public abstract class FieldsVisitor extends StoredFieldVisitor { protected Map> fieldsValues; public void postProcess(MapperService mapperService) { - if (fieldsValues == null || fieldsValues.isEmpty()) { - return; - } if (uid != null) { DocumentMapper documentMapper = mapperService.documentMapper(uid.type()); if (documentMapper != null) { @@ -56,7 +54,7 @@ public abstract class FieldsVisitor extends StoredFieldVisitor { } } // can't derive exact mapping type - for (Map.Entry> entry : fieldsValues.entrySet()) { + for (Map.Entry> entry : fields().entrySet()) { FieldMappers fieldMappers = mapperService.indexName(entry.getKey()); if (fieldMappers == null) { continue; @@ -69,8 +67,8 @@ public abstract class FieldsVisitor extends StoredFieldVisitor { } public void postProcess(DocumentMapper documentMapper) { - for (Map.Entry> entry : fieldsValues.entrySet()) { - FieldMapper fieldMapper = documentMapper.mappers().indexName(entry.getKey()).mapper(); + for (Map.Entry> entry : fields().entrySet()) { + FieldMapper fieldMapper = documentMapper.mappers().indexName(entry.getKey()).mapper(); if (fieldMapper == null) { continue; } @@ -128,7 +126,9 @@ public abstract class FieldsVisitor extends StoredFieldVisitor { } public Map> fields() { - return fieldsValues; + return fieldsValues != null + ? fieldsValues + : ImmutableMap.>of(); } public void reset() { diff --git a/src/main/java/org/elasticsearch/index/get/ShardGetService.java b/src/main/java/org/elasticsearch/index/get/ShardGetService.java index ae325a0d834..c3e8415a225 100644 --- a/src/main/java/org/elasticsearch/index/get/ShardGetService.java +++ b/src/main/java/org/elasticsearch/index/get/ShardGetService.java @@ -52,7 +52,6 @@ import org.elasticsearch.search.lookup.SearchLookup; import org.elasticsearch.search.lookup.SourceLookup; import java.io.IOException; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -322,7 +321,7 @@ public class ShardGetService extends AbstractIndexShardComponent { } source = fieldVisitor.source(); - if (fieldVisitor.fields() != null) { + if (!fieldVisitor.fields().isEmpty()) { fieldVisitor.postProcess(docMapper); fields = new HashMap(fieldVisitor.fields().size()); for (Map.Entry> entry : fieldVisitor.fields().entrySet()) { diff --git a/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java b/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java index 845fb0031c4..24091919c33 100644 --- a/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java +++ b/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java @@ -145,7 +145,7 @@ public class FetchPhase implements SearchPhase { fieldsVisitor.postProcess(context.mapperService()); Map searchFields = null; - if (fieldsVisitor.fields() != null) { + if (!fieldsVisitor.fields().isEmpty()) { searchFields = new HashMap(fieldsVisitor.fields().size()); for (Map.Entry> entry : fieldsVisitor.fields().entrySet()) { searchFields.put(entry.getKey(), new InternalSearchHitField(entry.getKey(), entry.getValue())); diff --git a/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java b/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java index 36c35bb80fd..b7c2e4b445a 100644 --- a/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java +++ b/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java @@ -18,6 +18,8 @@ */ package org.elasticsearch.search.highlight; +import com.google.common.collect.ImmutableList; + import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import org.apache.lucene.analysis.Analyzer; @@ -58,10 +60,10 @@ public class PlainHighlighter implements Highlighter { Encoder encoder = field.encoder().equals("html") ? Encoders.HTML : Encoders.DEFAULT; if (!hitContext.cache().containsKey(CACHE_KEY)) { - Map mappers = Maps.newHashMap(); + Map, org.apache.lucene.search.highlight.Highlighter> mappers = Maps.newHashMap(); hitContext.cache().put(CACHE_KEY, mappers); } - Map cache = (Map) hitContext.cache().get(CACHE_KEY); + Map, org.apache.lucene.search.highlight.Highlighter> cache = (Map, org.apache.lucene.search.highlight.Highlighter>) hitContext.cache().get(CACHE_KEY); org.apache.lucene.search.highlight.Highlighter entry = cache.get(mapper); if (entry == null) { @@ -98,6 +100,10 @@ public class PlainHighlighter implements Highlighter { CustomFieldsVisitor fieldVisitor = new CustomFieldsVisitor(ImmutableSet.of(mapper.names().indexName()), false); hitContext.reader().document(hitContext.docId(), fieldVisitor); textsToHighlight = fieldVisitor.fields().get(mapper.names().indexName()); + if (textsToHighlight == null) { + // Can happen if the document doesn't have the field to highlight + textsToHighlight = ImmutableList.of(); + } } catch (Exception e) { throw new FetchPhaseExecutionException(context, "Failed to highlight field [" + highlighterContext.fieldName + "]", e); } @@ -107,6 +113,7 @@ public class PlainHighlighter implements Highlighter { lookup.setNextDocId(hitContext.docId()); textsToHighlight = lookup.source().extractRawValues(mapper.names().sourcePath()); } + assert textsToHighlight != null; // a HACK to make highlighter do highlighting, even though its using the single frag list builder int numberOfFragments = field.numberOfFragments() == 0 ? 1 : field.numberOfFragments(); diff --git a/src/test/java/org/elasticsearch/test/integration/search/highlight/HighlighterSearchTests.java b/src/test/java/org/elasticsearch/test/integration/search/highlight/HighlighterSearchTests.java index 4c24c2240af..cdbad7b54c4 100644 --- a/src/test/java/org/elasticsearch/test/integration/search/highlight/HighlighterSearchTests.java +++ b/src/test/java/org/elasticsearch/test/integration/search/highlight/HighlighterSearchTests.java @@ -1503,4 +1503,34 @@ public class HighlighterSearchTests extends AbstractNodesTests { } } + @Test + public void testMissingStoredField() throws Exception { + try { + client.admin().indices().prepareDelete("test").execute().actionGet(); + } catch (Exception e) { + // ignore + } + + client.admin().indices().prepareCreate("test").setSettings(ImmutableSettings.settingsBuilder() + .put("index.number_of_shards", 1).put("index.number_of_replicas", 0)) + .addMapping("type1", jsonBuilder().startObject().startObject("type1").startObject("properties") + .startObject("highlight_field").field("type", "string").field("store", "yes").endObject() + .endObject().endObject().endObject()) + .execute().actionGet(); + + client.prepareIndex("test", "type1", "1") + .setSource(jsonBuilder().startObject() + .field("field", "highlight") + .endObject()) + .setRefresh(true).execute().actionGet(); + + // This query used to fail when the field to highlight was absent + SearchResponse response = client.prepareSearch("test") + .setQuery(QueryBuilders.matchQuery("field", "highlight").type(MatchQueryBuilder.Type.BOOLEAN)) + .addHighlightedField(new HighlightBuilder.Field("highlight_field") + .fragmentSize(-1).numOfFragments(1).fragmenter("simple")) + .execute().actionGet(); + assertThat(response.getHits().hits()[0].highlightFields().isEmpty(), equalTo(true)); + } + }