Fix NPEs due to disabled source

This commit fixes several NPEs caused by implicitly performing a get request for a document that exists with its _source disabled and then trying to access the source. Instead of causing an NPE the following queries will throw an exception with a "source disabled" message (similar behavior as if the document does not exist).:
- GeoShape query for pre-indexed shape (throws IllegalArgumentException)
- Percolate query for an existing document (throws IllegalArgumentException)

A Terms query with a lookup will ignore the document if the source does not exist (same as if the document does not exist).

GET and HEAD requests for the document _source will return a 404 if the source is disabled (even if the document exists).
This commit is contained in:
Alex Benusovich 2016-06-18 20:43:33 -07:00
parent 7c87d39f0c
commit 3ca909dfea
11 changed files with 159 additions and 12 deletions

View File

@ -238,7 +238,8 @@ public class NetworkModule extends AbstractModule {
RestIndexAction.class,
RestGetAction.class,
RestGetSourceAction.class,
RestHeadAction.class,
RestHeadAction.Document.class,
RestHeadAction.Source.class,
RestMultiGetAction.class,
RestDeleteAction.class,
org.elasticsearch.rest.action.count.RestCountAction.class,

View File

@ -378,6 +378,9 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder<GeoShapeQueryBuil
if (!response.isExists()) {
throw new IllegalArgumentException("Shape with ID [" + getRequest.id() + "] in type [" + getRequest.type() + "] not found");
}
if (response.isSourceEmpty()) {
throw new IllegalArgumentException("Shape with ID [" + getRequest.id() + "] in type [" + getRequest.type() + "] source disabled");
}
String[] pathElements = path.split("\\.");
int currentPathSlot = 0;

View File

@ -328,7 +328,7 @@ public class TermsQueryBuilder extends AbstractQueryBuilder<TermsQueryBuilder> {
GetRequest getRequest = new GetRequest(termsLookup.index(), termsLookup.type(), termsLookup.id())
.preference("_local").routing(termsLookup.routing());
final GetResponse getResponse = client.get(getRequest).actionGet();
if (getResponse.isExists()) {
if (getResponse.isSourceEmpty() == false) { // extract terms only if the doc source exists
List<Object> extractedValues = XContentMapValues.extractRawValues(termsLookup.path(), getResponse.getSourceAsMap());
terms.addAll(extractedValues);
}

View File

@ -78,7 +78,7 @@ public class RestGetSourceAction extends BaseRestHandler {
@Override
public RestResponse buildResponse(GetResponse response) throws Exception {
XContentBuilder builder = channel.newBuilder(response.getSourceInternal(), false);
if (!response.isExists()) {
if (response.isSourceEmpty()) { // check if doc source (or doc itself) is missing
return new BytesRestResponse(NOT_FOUND, builder);
} else {
builder.rawValue(response.getSourceInternal());

View File

@ -39,15 +39,47 @@ import static org.elasticsearch.rest.RestStatus.NOT_FOUND;
import static org.elasticsearch.rest.RestStatus.OK;
/**
*
* Base class for {@code HEAD} request handlers for a single document.
*/
public class RestHeadAction extends BaseRestHandler {
public abstract class RestHeadAction extends BaseRestHandler {
@Inject
public RestHeadAction(Settings settings, RestController controller, Client client) {
/**
* Handler to check for document existence.
*/
public static class Document extends RestHeadAction {
@Inject
public Document(Settings settings, RestController controller, Client client) {
super(settings, client, false);
controller.registerHandler(HEAD, "/{index}/{type}/{id}", this);
}
}
/**
* Handler to check for document source existence (may be disabled in the mapping).
*/
public static class Source extends RestHeadAction {
@Inject
public Source(Settings settings, RestController controller, Client client) {
super(settings, client, true);
controller.registerHandler(HEAD, "/{index}/{type}/{id}/_source", this);
}
}
private final boolean source;
/**
* All subclasses must be registered in {@link org.elasticsearch.common.network.NetworkModule}.
*
* @param settings injected settings
* @param client injected client
* @param source {@code false} to check for {@link GetResponse#isExists()}.
* {@code true} to also check for {@link GetResponse#isSourceEmpty()}.
*/
public RestHeadAction(Settings settings, Client client, boolean source) {
super(settings, client);
controller.registerHandler(HEAD, "/{index}/{type}/{id}", this);
controller.registerHandler(HEAD, "/{index}/{type}/{id}/_source", this);
this.source = source;
}
@Override
@ -68,6 +100,8 @@ public class RestHeadAction extends BaseRestHandler {
public RestResponse buildResponse(GetResponse response) {
if (!response.isExists()) {
return new BytesRestResponse(NOT_FOUND, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY);
} else if (source && response.isSourceEmpty()) { // doc exists, but source might not (disabled in the mapping)
return new BytesRestResponse(NOT_FOUND, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY);
} else {
return new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY);
}

View File

@ -19,6 +19,8 @@
package org.elasticsearch.search.geo;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.settings.Settings;
import org.locationtech.spatial4j.shape.Rectangle;
import com.vividsolutions.jts.geom.Coordinate;
@ -54,6 +56,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSear
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.nullValue;
public class GeoShapeQueryTests extends ESSingleNodeTestCase {
@ -197,6 +200,30 @@ public class GeoShapeQueryTests extends ESSingleNodeTestCase {
assertThat(searchResponse.getHits().getAt(0).id(), equalTo("1"));
}
public void testIndexedShapeReferenceSourceDisabled() throws Exception {
XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
.startObject("properties")
.startObject("location")
.field("type", "geo_shape")
.field("tree", "quadtree")
.endObject()
.endObject()
.endObject();
client().admin().indices().prepareCreate("test").addMapping("type1", mapping).get();
createIndex("shapes", Settings.EMPTY, "shape_type", "_source", "enabled=false");
ensureGreen();
ShapeBuilder shape = ShapeBuilders.newEnvelope(new Coordinate(-45, 45), new Coordinate(45, -45));
client().prepareIndex("shapes", "shape_type", "Big_Rectangle").setSource(jsonBuilder().startObject()
.field("shape", shape).endObject()).setRefreshPolicy(IMMEDIATE).get();
ElasticsearchException e = expectThrows(ElasticsearchException.class, () -> client().prepareSearch("test").setTypes("type1")
.setQuery(geoIntersectionQuery("location", "Big_Rectangle", "shape_type")).get());
assertThat(e.getRootCause(), instanceOf(IllegalArgumentException.class));
assertThat(e.getRootCause().getMessage(), containsString("source disabled"));
}
public void testReusableBuilder() throws IOException {
ShapeBuilder polygon = ShapeBuilders.newPolygon(new CoordinatesBuilder()
.coordinate(170, -10).coordinate(190, -10).coordinate(190, 10).coordinate(170, 10).close())

View File

@ -1148,6 +1148,7 @@ public class SearchQueryIT extends ESIntegTestCase {
jsonBuilder().startObject().startObject("type").startObject("properties")
.startObject("arr").startObject("properties").startObject("term").field("type", "text")
.endObject().endObject().endObject().endObject().endObject().endObject()));
assertAcked(prepareCreate("lookup3").addMapping("type", "_source", "enabled=false", "terms","type=text"));
assertAcked(prepareCreate("test").addMapping("type", "term", "type=text"));
indexRandom(true,
@ -1172,6 +1173,7 @@ public class SearchQueryIT extends ESIntegTestCase {
.startObject().field("term", "4").endObject()
.endArray()
.endObject()),
client().prepareIndex("lookup3", "type", "1").setSource("terms", new String[]{"1", "3"}),
client().prepareIndex("test", "type", "1").setSource("term", "1"),
client().prepareIndex("test", "type", "2").setSource("term", "2"),
client().prepareIndex("test", "type", "3").setSource("term", "3"),
@ -1227,6 +1229,16 @@ public class SearchQueryIT extends ESIntegTestCase {
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "type", "3", "arr.term"))).get();
assertHitCount(searchResponse, 0L);
// index "lookup" type "type" id "missing" document does not exist: ignore the lookup terms
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term" , new TermsLookup("lookup", "type", "missing", "terms"))).get();
assertHitCount(searchResponse, 0L);
// index "lookup3" type "type" has the source disabled: ignore the lookup terms
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term" , new TermsLookup("lookup3", "type", "1", "terms"))).get();
assertHitCount(searchResponse, 0L);
}
public void testBasicQueryById() throws Exception {

View File

@ -146,7 +146,8 @@ You can also use the same source filtering parameters to control which parts of
curl -XGET 'http://localhost:9200/twitter/tweet/1/_source?_source_include=*.id&_source_exclude=entities'
--------------------------------------------------
Note, there is also a HEAD variant for the _source endpoint to efficiently test for document existence.
Note, there is also a HEAD variant for the _source endpoint to efficiently test for document _source existence.
An existing document will not have a _source if it is disabled in the <<mapping-source-field,mapping>>.
Curl example:
[source,js]

View File

@ -359,6 +359,11 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
"indexed document [{}/{}/{}] couldn't be found", indexedDocumentIndex, indexedDocumentType, indexedDocumentId
);
}
if(getResponse.isSourceEmpty()) {
throw new IllegalArgumentException(
"indexed document [" + indexedDocumentIndex + "/" + indexedDocumentType + "/" + indexedDocumentId + "] source disabled"
);
}
return new PercolateQueryBuilder(field, documentType, getResponse.getSourceAsBytesRef());
}
@ -369,7 +374,7 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBu
}
if (document == null) {
throw new IllegalStateException("nothing to percolator");
throw new IllegalStateException("no document to percolate");
}
MapperService mapperService = context.getMapperService();

View File

@ -19,6 +19,7 @@
package org.elasticsearch.percolator;
import org.apache.lucene.search.join.ScoreMode;
import org.elasticsearch.action.search.SearchPhaseExecutionException;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
@ -37,7 +38,6 @@ import org.elasticsearch.test.ESSingleNodeTestCase;
import java.util.Collection;
import java.util.Collections;
import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
import static org.elasticsearch.index.query.QueryBuilders.commonTermsQuery;
@ -49,6 +49,7 @@ import static org.elasticsearch.index.query.QueryBuilders.spanNotQuery;
import static org.elasticsearch.index.query.QueryBuilders.spanTermQuery;
import static org.elasticsearch.index.query.QueryBuilders.termQuery;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
@ -158,6 +159,29 @@ public class PercolatorQuerySearchIT extends ESSingleNodeTestCase {
assertThat(response.getHits().getAt(2).getId(), equalTo("3"));
}
public void testPercolatorQueryExistingDocumentSourceDisabled() throws Exception {
createIndex("test", client().admin().indices().prepareCreate("test")
.addMapping("type", "_source", "enabled=false", "field1", "type=keyword")
.addMapping("queries", "query", "type=percolator")
);
client().prepareIndex("test", "queries", "1")
.setSource(jsonBuilder().startObject().field("query", matchAllQuery()).endObject())
.get();
client().prepareIndex("test", "type", "1").setSource("{}").get();
client().admin().indices().prepareRefresh().get();
logger.info("percolating empty doc with source disabled");
Throwable e = expectThrows(SearchPhaseExecutionException.class, () -> {
client().prepareSearch()
.setQuery(new PercolateQueryBuilder("query", "type", "test", "type", "1", null, null, null))
.get();
}).getRootCause();
assertThat(e, instanceOf(IllegalArgumentException.class));
assertThat(e.getMessage(), containsString("source disabled"));
}
public void testPercolatorSpecificQueries() throws Exception {
createIndex("test", client().admin().indices().prepareCreate("test")
.addMapping("type", "field1", "type=text", "field2", "type=text")

View File

@ -0,0 +1,40 @@
---
setup:
- do:
indices.create:
index: test_1
body:
mappings:
test:
_source: { enabled: false }
- do:
cluster.health:
wait_for_status: yellow
- do:
index:
index: test_1
type: test
id: 1
body: { foo: bar }
---
"Missing document source with catch":
- do:
catch: missing
get_source:
index: test_1
type: test
id: 1
---
"Missing document source with ignore":
- do:
get_source:
index: test_1
type: test
id: 1
ignore: 404