From 5b01d50582847f620ad416b7d359e1c923ad9f22 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Wed, 6 Feb 2019 11:57:45 +0100 Subject: [PATCH] Add version-based validation to reindex requests --- .../elasticsearch/action/DocWriteRequest.java | 34 ++++++++++++++----- .../action/delete/DeleteRequest.java | 2 +- .../action/index/IndexRequest.java | 20 +---------- .../action/update/UpdateRequest.java | 2 +- .../index/reindex/ReindexRequest.java | 10 ++---- .../index/reindex/ReindexRequestTests.java | 10 ++++++ 6 files changed, 42 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/DocWriteRequest.java b/server/src/main/java/org/elasticsearch/action/DocWriteRequest.java index 373dfaa5c7416..1f08d7d6ebad7 100644 --- a/server/src/main/java/org/elasticsearch/action/DocWriteRequest.java +++ b/server/src/main/java/org/elasticsearch/action/DocWriteRequest.java @@ -255,10 +255,9 @@ static void writeDocumentRequest(StreamOutput out, DocWriteRequest request) } } - static ActionRequestValidationException validateSeqNoBasedCASParams( - DocWriteRequest request, ActionRequestValidationException validationException) { - final long version = request.version(); - final VersionType versionType = request.versionType(); + default ActionRequestValidationException validateVersionAndSeqNoBasedCASParams(ActionRequestValidationException validationException) { + final long version = version(); + final VersionType versionType = versionType(); if (versionType.validateVersionForWrites(version) == false) { validationException = addValidationError("illegal version value [" + version + "] for version type [" + versionType.name() + "]", validationException); @@ -272,17 +271,36 @@ static ActionRequestValidationException validateSeqNoBasedCASParams( "Please use `if_seq_no` and `if_primary_term` instead", validationException); } - if (request.ifSeqNo() != UNASSIGNED_SEQ_NO && ( + if (ifSeqNo() != UNASSIGNED_SEQ_NO && ( versionType != VersionType.INTERNAL || version != Versions.MATCH_ANY )) { validationException = addValidationError("compare and write operations can not use versioning", validationException); } - if (request.ifPrimaryTerm() == UNASSIGNED_PRIMARY_TERM && request.ifSeqNo() != UNASSIGNED_SEQ_NO) { + if (ifPrimaryTerm() == UNASSIGNED_PRIMARY_TERM && ifSeqNo() != UNASSIGNED_SEQ_NO) { validationException = addValidationError("ifSeqNo is set, but primary term is [0]", validationException); } - if (request.ifPrimaryTerm() != UNASSIGNED_PRIMARY_TERM && request.ifSeqNo() == UNASSIGNED_SEQ_NO) { + if (ifPrimaryTerm() != UNASSIGNED_PRIMARY_TERM && ifSeqNo() == UNASSIGNED_SEQ_NO) { validationException = - addValidationError("ifSeqNo is unassigned, but primary term is [" + request.ifPrimaryTerm() + "]", validationException); + addValidationError("ifSeqNo is unassigned, but primary term is [" + ifPrimaryTerm() + "]", validationException); + } + if (opType() == OpType.CREATE) { + if (versionType != VersionType.INTERNAL) { + validationException = addValidationError("create operations only support internal versioning. use index instead", + validationException); + return validationException; + } + + if (version != Versions.MATCH_DELETED) { + validationException = addValidationError("create operations do not support explicit versions. use index instead", + validationException); + return validationException; + } + + if (ifSeqNo() != UNASSIGNED_SEQ_NO || ifPrimaryTerm() != UNASSIGNED_PRIMARY_TERM) { + validationException = addValidationError("create operations do not support compare and set. use index instead", + validationException); + return validationException; + } } return validationException; diff --git a/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java b/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java index a033bf3cb000f..a6d23dc78decf 100644 --- a/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java +++ b/server/src/main/java/org/elasticsearch/action/delete/DeleteRequest.java @@ -111,7 +111,7 @@ public ActionRequestValidationException validate() { validationException = addValidationError("id is missing", validationException); } - validationException = DocWriteRequest.validateSeqNoBasedCASParams(this, validationException); + validationException = validateVersionAndSeqNoBasedCASParams(validationException); return validationException; } diff --git a/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java b/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java index 37d960831776d..94ebb857151fb 100644 --- a/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java +++ b/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java @@ -164,31 +164,13 @@ public ActionRequestValidationException validate() { validationException = addValidationError("content type is missing", validationException); } final long resolvedVersion = resolveVersionDefaults(); - if (opType() == OpType.CREATE) { - if (versionType != VersionType.INTERNAL) { - validationException = addValidationError("create operations only support internal versioning. use index instead", - validationException); - return validationException; - } - - if (resolvedVersion != Versions.MATCH_DELETED) { - validationException = addValidationError("create operations do not support explicit versions. use index instead", - validationException); - return validationException; - } - if (ifSeqNo != UNASSIGNED_SEQ_NO || ifPrimaryTerm != UNASSIGNED_PRIMARY_TERM) { - validationException = addValidationError("create operations do not support compare and set. use index instead", - validationException); - return validationException; - } - } if (opType() != OpType.INDEX && id == null) { addValidationError("an id is required for a " + opType() + " operation", validationException); } - validationException = DocWriteRequest.validateSeqNoBasedCASParams(this, validationException); + validationException = validateVersionAndSeqNoBasedCASParams(validationException); if (id != null && id.getBytes(StandardCharsets.UTF_8).length > 512) { validationException = addValidationError("id is too long, must be no longer than 512 bytes but was: " + diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java index 3693975ddab08..69b22b2f694fb 100644 --- a/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java @@ -158,7 +158,7 @@ public ActionRequestValidationException validate() { validationException = addValidationError("id is missing", validationException); } - validationException = DocWriteRequest.validateSeqNoBasedCASParams(this, validationException); + validationException = validateVersionAndSeqNoBasedCASParams(validationException); if (ifSeqNo != UNASSIGNED_SEQ_NO) { if (retryOnConflict > 0) { diff --git a/server/src/main/java/org/elasticsearch/index/reindex/ReindexRequest.java b/server/src/main/java/org/elasticsearch/index/reindex/ReindexRequest.java index cd93356bb3968..5647c960e6ecb 100644 --- a/server/src/main/java/org/elasticsearch/index/reindex/ReindexRequest.java +++ b/server/src/main/java/org/elasticsearch/index/reindex/ReindexRequest.java @@ -25,7 +25,6 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.lucene.uid.Versions; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.VersionType; @@ -37,7 +36,6 @@ import java.io.IOException; import static org.elasticsearch.action.ValidateActions.addValidationError; -import static org.elasticsearch.index.VersionType.INTERNAL; /** * Request to reindex some documents from one index to another. This implements CompositeIndicesRequest but in a misleading way. Rather than @@ -100,11 +98,9 @@ public ActionRequestValidationException validate() { if (false == routingIsValid()) { e = addValidationError("routing must be unset, [keep], [discard] or [=]", e); } - if (destination.versionType() == INTERNAL) { - if (destination.version() != Versions.MATCH_ANY && destination.version() != Versions.MATCH_DELETED) { - e = addValidationError("unsupported version for internal versioning [" + destination.version() + ']', e); - } - } + + e = destination.validateVersionAndSeqNoBasedCASParams(e); + if (getRemoteInfo() != null) { if (getSearchRequest().source().query() != null) { e = addValidationError("reindex from remote sources should use RemoteInfo's query instead of source's query", e); diff --git a/server/src/test/java/org/elasticsearch/index/reindex/ReindexRequestTests.java b/server/src/test/java/org/elasticsearch/index/reindex/ReindexRequestTests.java index 1c3d539263e7d..ff8a688d481b5 100644 --- a/server/src/test/java/org/elasticsearch/index/reindex/ReindexRequestTests.java +++ b/server/src/test/java/org/elasticsearch/index/reindex/ReindexRequestTests.java @@ -21,11 +21,13 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.index.VersionType; import org.elasticsearch.search.slice.SliceBuilder; import static java.util.Collections.emptyMap; import static org.elasticsearch.common.unit.TimeValue.parseTimeValue; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; +import static org.hamcrest.Matchers.containsString; /** * Tests some of the validation of {@linkplain ReindexRequest}. See reindex's rest tests for much more. @@ -57,6 +59,14 @@ public void testReindexFromRemoteDoesNotSupportSlices() { e.getMessage()); } + public void testReindexShouldThrowErrorWhenCreateIsUsedWithExternalVersionType() { + ReindexRequest reindex = newRequest(); + reindex.setDestOpType("create"); + reindex.setDestVersionType(VersionType.EXTERNAL); + ActionRequestValidationException e = reindex.validate(); + assertThat(e.getMessage(), containsString("create operations only support internal versioning. use index instead;")); + } + public void testNoSliceBuilderSetWithSlicedRequest() { ReindexRequest reindex = newRequest(); reindex.getSearchRequest().source().slice(new SliceBuilder(0, 4));