Fix NOOP bulk updates (#32819)

#31821 introduced an unreleased bug where NOOP updates were incorrectly mutating the bulk shard request, inserting null item to be replicated, which would result in NullPointerExceptions when serializing the request to be shipped to the replicas. Closes #32808
elastic · Aug 14, 2018 · 5f656d7 · 5f656d7
1 parent 28309b1
commit 5f656d7
Show file tree

Hide file tree

Showing 3 changed files with 4 additions and 3 deletions.
diff --git a/server/src/main/java/org/elasticsearch/action/bulk/BulkPrimaryExecutionContext.java b/server/src/main/java/org/elasticsearch/action/bulk/BulkPrimaryExecutionContext.java
@@ -290,10 +290,10 @@ public void markOperationAsExecuted(Engine.Result result) {
     /** finishes the execution of the current request, with the response that should be returned to the user */
     public void markAsCompleted(BulkItemResponse translatedResponse) {
         assertInvariants(ItemProcessingState.EXECUTED);
-        assert executionResult == null || translatedResponse.getItemId() == executionResult.getItemId();
+        assert executionResult != null && translatedResponse.getItemId() == executionResult.getItemId();
         assert translatedResponse.getItemId() == getCurrentItem().id();
 
-        if (translatedResponse.isFailed() == false && requestToExecute != getCurrent())  {
+        if (translatedResponse.isFailed() == false && requestToExecute != null && requestToExecute != getCurrent())  {
             request.items()[currentIndex] = new BulkItemRequest(request.items()[currentIndex].id(), requestToExecute);
         }
         getCurrentItem().setPrimaryResponse(translatedResponse);

diff --git a/server/src/test/java/org/elasticsearch/action/IndicesRequestIT.java b/server/src/test/java/org/elasticsearch/action/IndicesRequestIT.java
@@ -284,7 +284,6 @@ public void testUpdateDelete() {
         assertSameIndices(updateRequest, updateShardActions);
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32808")
     public void testBulk() {
         String[] bulkShardActions = new String[]{BulkAction.NAME + "[s][p]", BulkAction.NAME + "[s][r]"};
         interceptTransportActions(bulkShardActions);

diff --git a/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java b/server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java
@@ -472,6 +472,8 @@ public void testNoopUpdateRequest() throws Exception {
         assertThat(primaryResponse.getResponse(), equalTo(noopUpdateResponse));
         assertThat(primaryResponse.getResponse().getResult(),
             equalTo(DocWriteResponse.Result.NOOP));
+        assertThat(bulkShardRequest.items().length, equalTo(1));
+        assertEquals(primaryRequest, bulkShardRequest.items()[0]); // check that bulk item was not mutated
         assertThat(primaryResponse.getResponse().getSeqNo(), equalTo(SequenceNumbers.UNASSIGNED_SEQ_NO));
     }