From d621ab9958aa3fd313f2a5e03ad89a8268b33f6f Mon Sep 17 00:00:00 2001 From: David Pilato Date: Tue, 28 Jan 2014 16:52:09 +0100 Subject: [PATCH] Fix potential NPE when no source and no body In recent changes, we added missing support for `source` parameter in some REST APIs: * #4892 : mget * #4900 : mpercolate * #4901 : msearch * #4902 : mtermvectors * #4903 : percolate ```java BytesReference content = null; if (request.hasContent()) { content = request.content(); } else { String source = request.param("source"); if (source != null) { content = new BytesArray(source); } } ``` It's definitely better to have: ```java BytesReference content = request.content(); if (!request.hasContent()) { String source = request.param("source"); if (source != null) { content = new BytesArray(source); } } ``` That said, it could be nice to have a single method to manage it for various REST actions. Closes #4924. --- .../rest/action/get/RestMultiGetAction.java | 15 ++----------- .../percolate/RestMultiPercolateAction.java | 15 ++----------- .../action/percolate/RestPercolateAction.java | 15 +------------ .../action/search/RestMultiSearchAction.java | 15 ++----------- .../rest/action/support/RestActions.java | 21 +++++++++++++++++++ .../RestMultiTermVectorsAction.java | 13 ++---------- 6 files changed, 30 insertions(+), 64 deletions(-) diff --git a/src/main/java/org/elasticsearch/rest/action/get/RestMultiGetAction.java b/src/main/java/org/elasticsearch/rest/action/get/RestMultiGetAction.java index a2c46d6c046..a89385fbe03 100644 --- a/src/main/java/org/elasticsearch/rest/action/get/RestMultiGetAction.java +++ b/src/main/java/org/elasticsearch/rest/action/get/RestMultiGetAction.java @@ -24,12 +24,11 @@ import org.elasticsearch.action.get.MultiGetRequest; import org.elasticsearch.action.get.MultiGetResponse; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.*; +import org.elasticsearch.rest.action.support.RestActions; import org.elasticsearch.search.fetch.source.FetchSourceContext; import java.io.IOException; @@ -73,18 +72,8 @@ public class RestMultiGetAction extends BaseRestHandler { FetchSourceContext defaultFetchSource = FetchSourceContext.parseFromRestRequest(request); - BytesReference content = null; - if (request.hasContent()) { - content = request.content(); - } else { - String source = request.param("source"); - if (source != null) { - content = new BytesArray(source); - } - } - try { - multiGetRequest.add(request.param("index"), request.param("type"), sFields, defaultFetchSource, request.param("routing"), content, allowExplicitIndex); + multiGetRequest.add(request.param("index"), request.param("type"), sFields, defaultFetchSource, request.param("routing"), RestActions.getRestContent(request), allowExplicitIndex); } catch (Exception e) { try { XContentBuilder builder = restContentBuilder(request); diff --git a/src/main/java/org/elasticsearch/rest/action/percolate/RestMultiPercolateAction.java b/src/main/java/org/elasticsearch/rest/action/percolate/RestMultiPercolateAction.java index c11d6431cc2..b8064594bff 100644 --- a/src/main/java/org/elasticsearch/rest/action/percolate/RestMultiPercolateAction.java +++ b/src/main/java/org/elasticsearch/rest/action/percolate/RestMultiPercolateAction.java @@ -24,12 +24,11 @@ import org.elasticsearch.action.percolate.MultiPercolateResponse; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.*; +import org.elasticsearch.rest.action.support.RestActions; import java.io.IOException; @@ -66,18 +65,8 @@ public class RestMultiPercolateAction extends BaseRestHandler { multiPercolateRequest.indices(Strings.splitStringByCommaToArray(restRequest.param("index"))); multiPercolateRequest.documentType(restRequest.param("type")); - BytesReference content = null; - if (restRequest.hasContent()) { - content = restRequest.content(); - } else { - String source = restRequest.param("source"); - if (source != null) { - content = new BytesArray(source); - } - } - try { - multiPercolateRequest.add(content, restRequest.contentUnsafe(), allowExplicitIndex); + multiPercolateRequest.add(RestActions.getRestContent(restRequest), restRequest.contentUnsafe(), allowExplicitIndex); } catch (Exception e) { try { restChannel.sendResponse(new XContentThrowableRestResponse(restRequest, e)); diff --git a/src/main/java/org/elasticsearch/rest/action/percolate/RestPercolateAction.java b/src/main/java/org/elasticsearch/rest/action/percolate/RestPercolateAction.java index b83bf7d8dba..0b2f71a3bd8 100644 --- a/src/main/java/org/elasticsearch/rest/action/percolate/RestPercolateAction.java +++ b/src/main/java/org/elasticsearch/rest/action/percolate/RestPercolateAction.java @@ -26,8 +26,6 @@ import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.broadcast.BroadcastOperationThreading; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -71,18 +69,7 @@ public class RestPercolateAction extends BaseRestHandler { percolateRequest.documentType(restRequest.param("type")); percolateRequest.routing(restRequest.param("routing")); percolateRequest.preference(restRequest.param("preference")); - - BytesReference content = null; - if (restRequest.hasContent()) { - content = restRequest.content(); - } else { - String source = restRequest.param("source"); - if (source != null) { - content = new BytesArray(source); - } - } - - percolateRequest.source(content, restRequest.contentUnsafe()); + percolateRequest.source(RestActions.getRestContent(restRequest), restRequest.contentUnsafe()); percolateRequest.indicesOptions(IndicesOptions.fromRequest(restRequest, percolateRequest.indicesOptions())); executePercolate(percolateRequest, restRequest, restChannel); diff --git a/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java b/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java index ca6137eb93c..10a1ec041ab 100644 --- a/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java +++ b/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java @@ -25,12 +25,11 @@ import org.elasticsearch.action.search.MultiSearchResponse; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.*; +import org.elasticsearch.rest.action.support.RestActions; import java.io.IOException; @@ -69,18 +68,8 @@ public class RestMultiSearchAction extends BaseRestHandler { String[] types = Strings.splitStringByCommaToArray(request.param("type")); IndicesOptions indicesOptions = IndicesOptions.fromRequest(request, multiSearchRequest.indicesOptions()); - BytesReference content = null; - if (request.hasContent()) { - content = request.content(); - } else { - String source = request.param("source"); - if (source != null) { - content = new BytesArray(source); - } - } - try { - multiSearchRequest.add(content, request.contentUnsafe(), indices, types, request.param("search_type"), request.param("routing"), indicesOptions, allowExplicitIndex); + multiSearchRequest.add(RestActions.getRestContent(request), request.contentUnsafe(), indices, types, request.param("search_type"), request.param("routing"), indicesOptions, allowExplicitIndex); } catch (Exception e) { try { XContentBuilder builder = restContentBuilder(request); diff --git a/src/main/java/org/elasticsearch/rest/action/support/RestActions.java b/src/main/java/org/elasticsearch/rest/action/support/RestActions.java index 82c4f595188..8cc900284eb 100644 --- a/src/main/java/org/elasticsearch/rest/action/support/RestActions.java +++ b/src/main/java/org/elasticsearch/rest/action/support/RestActions.java @@ -23,6 +23,8 @@ import org.elasticsearch.ElasticsearchIllegalArgumentException; import org.elasticsearch.action.ShardOperationFailedException; import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.action.support.broadcast.BroadcastOperationResponse; +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentBuilderString; import org.elasticsearch.index.query.QueryBuilders; @@ -101,4 +103,23 @@ public class RestActions { } return new QuerySourceBuilder().setQuery(queryBuilder); } + + /** + * Get Rest content from either payload or source parameter + * @param request Rest request + * @return rest content + */ + public static BytesReference getRestContent(RestRequest request) { + assert request != null; + + BytesReference content = request.content(); + if (!request.hasContent()) { + String source = request.param("source"); + if (source != null) { + content = new BytesArray(source); + } + } + + return content; + } } diff --git a/src/main/java/org/elasticsearch/rest/action/termvector/RestMultiTermVectorsAction.java b/src/main/java/org/elasticsearch/rest/action/termvector/RestMultiTermVectorsAction.java index 7d3d5e4d94d..e1b0b6f184b 100644 --- a/src/main/java/org/elasticsearch/rest/action/termvector/RestMultiTermVectorsAction.java +++ b/src/main/java/org/elasticsearch/rest/action/termvector/RestMultiTermVectorsAction.java @@ -25,12 +25,11 @@ import org.elasticsearch.action.termvector.MultiTermVectorsResponse; import org.elasticsearch.action.termvector.TermVectorRequest; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesArray; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.rest.*; +import org.elasticsearch.rest.action.support.RestActions; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; @@ -61,16 +60,8 @@ public class RestMultiTermVectorsAction extends BaseRestHandler { RestTermVectorAction.readURIParameters(template, request); multiTermVectorsRequest.ids(Strings.commaDelimitedListToStringArray(request.param("ids"))); - BytesReference content = request.content(); - if (!request.hasContent()) { - String source = request.param("source"); - if (source != null) { - content = new BytesArray(source); - } - } - try { - multiTermVectorsRequest.add(template, content); + multiTermVectorsRequest.add(template, RestActions.getRestContent(request)); } catch (Throwable t) { try { channel.sendResponse(new XContentThrowableRestResponse(request, t));