Fix DeleteRequest / GetRequest / UpdateRequest / ExplainRequest validation for null and/or empty id/type (elastic#35314)

vladimirdolzhenko · web-flow · commit a467a816bc14 · 2018-11-07T12:52:11.000+01:00
Closes elastic#35297
diff --git a/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java b/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.action.DocWriteRequest;
 import org.elasticsearch.action.support.replication.ReplicatedWriteRequest;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.lucene.uid.Versions;
@@ -83,13 +84,13 @@ public DeleteRequest(String index, String type, String id) {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = super.validate();
-        if (type == null) {
+        if (Strings.isEmpty(type)) {
             validationException = addValidationError("type is missing", validationException);
         }
-        if (id == null) {
+        if (Strings.isEmpty(id)) {
             validationException = addValidationError("id is missing", validationException);
         }
-        if (!versionType.validateVersionForWrites(version)) {
+        if (versionType.validateVersionForWrites(version) == false) {
             validationException = addValidationError("illegal version value [" + version + "] for version type ["
                 + versionType.name() + "]", validationException);
         }
diff --git a/server/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java b/server/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java
@@ -34,6 +34,8 @@
 
 import java.io.IOException;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 /**
  * Explain request encapsulating the explain query and document identifier to get an explanation for.
  */
@@ -152,11 +154,11 @@ public ExplainRequest filteringAlias(AliasFilter filteringAlias) {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = super.validateNonNullIndex();
-        if (type == null) {
-            validationException = ValidateActions.addValidationError("type is missing", validationException);
+        if (Strings.isEmpty(type)) {
+            validationException = addValidationError("type is missing", validationException);
         }
-        if (id == null) {
-            validationException = ValidateActions.addValidationError("id is missing", validationException);
+        if (Strings.isEmpty(id)) {
+            validationException = addValidationError("id is missing", validationException);
         }
         if (query == null) {
             validationException = ValidateActions.addValidationError("query is missing", validationException);
diff --git a/server/src/main/java/org/elasticsearch/action/get/GetRequest.java b/server/src/main/java/org/elasticsearch/action/get/GetRequest.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.action.ValidateActions;
 import org.elasticsearch.action.support.single.shard.SingleShardRequest;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.lucene.uid.Versions;
@@ -33,6 +34,8 @@
 
 import java.io.IOException;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 /**
  * A request to get a document (its source) from an index based on its type (optional) and id. Best created using
  * {@link org.elasticsearch.client.Requests#getRequest(String)}.
@@ -91,13 +94,13 @@ public GetRequest(String index, String type, String id) {
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = super.validateNonNullIndex();
-        if (type == null) {
-            validationException = ValidateActions.addValidationError("type is missing", validationException);
+        if (Strings.isEmpty(type)) {
+            validationException = addValidationError("type is missing", validationException);
         }
-        if (id == null) {
-            validationException = ValidateActions.addValidationError("id is missing", validationException);
+        if (Strings.isEmpty(id)) {
+            validationException = addValidationError("id is missing", validationException);
         }
-        if (!versionType.validateVersionForReads(version)) {
+        if (versionType.validateVersionForReads(version) == false) {
             validationException = ValidateActions.addValidationError("illegal version value [" + version + "] for version type ["
                     + versionType.name() + "]", validationException);
         }
diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java
@@ -136,10 +136,10 @@ public ActionRequestValidationException validate() {
         if(upsertRequest != null && upsertRequest.version() != Versions.MATCH_ANY) {
             validationException = addValidationError("can't provide version in upsert request", validationException);
         }
-        if (type == null) {
+        if (Strings.isEmpty(type)) {
             validationException = addValidationError("type is missing", validationException);
         }
-        if (id == null) {
+        if (Strings.isEmpty(id)) {
             validationException = addValidationError("id is missing", validationException);
         }
 
diff --git a/server/src/test/java/org/elasticsearch/action/delete/DeleteRequestTests.java b/server/src/test/java/org/elasticsearch/action/delete/DeleteRequestTests.java
@@ -0,0 +1,46 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.action.delete;
+
+import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.CoreMatchers.hasItems;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
+
+public class DeleteRequestTests extends ESTestCase {
+
+    public void testValidation() {
+        {
+            final DeleteRequest request = new DeleteRequest("index4", "_doc", "0");
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, nullValue());
+        }
+
+        {
+            final DeleteRequest request = new DeleteRequest("index4", randomBoolean() ? "" : null, randomBoolean() ? "" : null);
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, not(nullValue()));
+            assertThat(validate.validationErrors(), hasItems("type is missing", "id is missing"));
+        }
+    }
+}
diff --git a/server/src/test/java/org/elasticsearch/action/explain/ExplainRequestTests.java b/server/src/test/java/org/elasticsearch/action/explain/ExplainRequestTests.java
@@ -18,6 +18,7 @@
  */
 package org.elasticsearch.action.explain;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
@@ -35,6 +36,10 @@
 import java.util.Collections;
 import java.util.List;
 
+import static org.hamcrest.CoreMatchers.hasItems;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
+
 public class ExplainRequestTests extends ESTestCase {
     private NamedWriteableRegistry namedWriteableRegistry;
 
@@ -70,4 +75,24 @@ public void testSerialize() throws IOException {
             }
         }
     }
+
+    public void testValidation() {
+        {
+            final ExplainRequest request = new ExplainRequest("index4", "_doc", "0");
+            request.query(QueryBuilders.termQuery("field", "value"));
+
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, nullValue());
+        }
+
+        {
+            final ExplainRequest request = new ExplainRequest("index4", randomBoolean() ? "" : null, randomBoolean() ? "" : null);
+            request.query(QueryBuilders.termQuery("field", "value"));
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, not(nullValue()));
+            assertThat(validate.validationErrors(), hasItems("type is missing", "id is missing"));
+        }
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/action/get/GetRequestTests.java b/server/src/test/java/org/elasticsearch/action/get/GetRequestTests.java
@@ -0,0 +1,46 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.action.get;
+
+import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.CoreMatchers.hasItems;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
+
+public class GetRequestTests  extends ESTestCase {
+
+    public void testValidation() {
+        {
+            final GetRequest request = new GetRequest("index4", "_doc", "0");
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, nullValue());
+        }
+
+        {
+            final GetRequest request = new GetRequest("index4", randomBoolean() ? "" : null, randomBoolean() ? "" : null);
+            final ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, not(nullValue()));
+            assertThat(validate.validationErrors(), hasItems("type is missing", "id is missing"));
+        }
+    }
+}
diff --git a/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java b/server/src/test/java/org/elasticsearch/action/update/UpdateRequestTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.action.update;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.DocWriteResponse;
 import org.elasticsearch.action.delete.DeleteRequest;
 import org.elasticsearch.action.index.IndexRequest;
@@ -61,6 +62,9 @@
 import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
 import static org.elasticsearch.script.MockScriptEngine.mockInlineScript;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
+import static org.hamcrest.CoreMatchers.hasItems;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
@@ -511,6 +515,25 @@ public void testToValidateUpsertRequestWithVersion() {
         assertThat(updateRequest.validate().validationErrors(), contains("can't provide version in upsert request"));
     }
 
+    public void testValidate() {
+        {
+            UpdateRequest request = new UpdateRequest("index", "type", "id");
+            request.doc("{}", XContentType.JSON);
+            ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, nullValue());
+        }
+
+        {
+            UpdateRequest request = new UpdateRequest("index", randomBoolean() ? "" : null, randomBoolean() ? "" : null);
+            request.doc("{}", XContentType.JSON);
+            ActionRequestValidationException validate = request.validate();
+
+            assertThat(validate, not(nullValue()));
+            assertThat(validate.validationErrors(), hasItems("type is missing", "id is missing"));
+        }
+    }
+
     public void testRoutingExtraction() throws Exception {
         GetResult getResult = new GetResult("test", "type", "1", 0, false, null, null);
         IndexRequest indexRequest = new IndexRequest("test", "type", "1");

Original file line number	Diff line number	Diff line change
`@@ -136,10 +136,10 @@ public ActionRequestValidationException validate() {`
`136`	`136`	`if(upsertRequest != null && upsertRequest.version() != Versions.MATCH_ANY) {`
`137`	`137`	`validationException = addValidationError("can't provide version in upsert request", validationException);`
`138`	`138`	`}`
`139`		`- if (type == null) {`
	`139`	`+ if (Strings.isEmpty(type)) {`
`140`	`140`	`validationException = addValidationError("type is missing", validationException);`
`141`	`141`	`}`
`142`		`- if (id == null) {`
	`142`	`+ if (Strings.isEmpty(id)) {`
`143`	`143`	`validationException = addValidationError("id is missing", validationException);`
`144`	`144`	`}`
`145`	`145`