From 49f41d127bcad5c21383f7d9c37099995aa2be3b Mon Sep 17 00:00:00 2001
From: Hendrik Muhs <hendrik.muhs@elastic.co>
Date: Mon, 2 Mar 2020 08:02:26 +0100
Subject: [PATCH] =?UTF-8?q?[Transform]=20fix=20NPE=20in=20derive=20stats?=
 =?UTF-8?q?=20if=20shouldStopAtNextCheckpo=E2=80=A6=20(#52940)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

fixes a NPE in _stats in case shouldStopAtNextCheckpoint is set.
---
 .../TransportGetTransformStatsAction.java     |   3 +-
 ...TransportGetTransformStatsActionTests.java | 150 ++++++++++++------
 2 files changed, 106 insertions(+), 47 deletions(-)

diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsAction.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsAction.java
index 363392e4ad6..764d00537af 100644
--- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsAction.java
+++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsAction.java
@@ -18,6 +18,7 @@ import org.elasticsearch.action.support.tasks.TransportTasksAction;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.persistent.PersistentTasksCustomMetaData;
@@ -189,7 +190,7 @@ public class TransportGetTransformStatsAction extends TransportTasksAction<Trans
             && derivedState.equals(TransformStats.State.STOPPED) == false
             && derivedState.equals(TransformStats.State.FAILED) == false) {
             derivedState = TransformStats.State.STOPPING;
-            reason = reason.isEmpty() ? "transform is set to stop at the next checkpoint" : reason;
+            reason = Strings.isNullOrEmpty(reason) ? "transform is set to stop at the next checkpoint" : reason;
         }
         return new TransformStats(
             task.getTransformId(),
diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsActionTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsActionTests.java
index 7dbe2b4f3a2..28b5d11633a 100644
--- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsActionTests.java
+++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/action/TransportGetTransformStatsActionTests.java
@@ -28,101 +28,159 @@ public class TransportGetTransformStatsActionTests extends ESTestCase {
 
     public void testDeriveStatsStopped() {
         String transformId = "transform-with-stats";
-        String reason = "";
+        String reason = null;
         TransformIndexerStats stats = TransformIndexerStatsTests.randomStats();
-        TransformState stoppedState =
-            new TransformState(TransformTaskState.STOPPED, IndexerState.STOPPED, null, 0, reason, null, null, true);
+        TransformState stoppedState = new TransformState(
+            TransformTaskState.STOPPED,
+            IndexerState.STOPPED,
+            null,
+            0,
+            reason,
+            null,
+            null,
+            true
+        );
         withIdStateAndStats(transformId, stoppedState, stats);
         TransformCheckpointingInfo info = new TransformCheckpointingInfo(
             new TransformCheckpointStats(1, null, null, 1, 1),
             new TransformCheckpointStats(2, null, null, 2, 5),
             2,
-            Instant.now());
-
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, "", null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, "", null, stats, info)));
+            Instant.now()
+        );
 
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, info))
+        );
 
         reason = "foo";
         stoppedState = new TransformState(TransformTaskState.STOPPED, IndexerState.STOPPED, null, 0, reason, null, null, true);
         withIdStateAndStats(transformId, stoppedState, stats);
 
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, info)));
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPED, reason, null, stats, info))
+        );
     }
 
     public void testDeriveStatsFailed() {
         String transformId = "transform-with-stats";
-        String reason = "";
+        String reason = null;
         TransformIndexerStats stats = TransformIndexerStatsTests.randomStats();
-        TransformState failedState =
-            new TransformState(TransformTaskState.FAILED, IndexerState.STOPPED, null, 0, reason, null, null, true);
+        TransformState failedState = new TransformState(TransformTaskState.FAILED, IndexerState.STOPPED, null, 0, reason, null, null, true);
         withIdStateAndStats(transformId, failedState, stats);
         TransformCheckpointingInfo info = new TransformCheckpointingInfo(
             new TransformCheckpointStats(1, null, null, 1, 1),
             new TransformCheckpointStats(2, null, null, 2, 5),
             2,
-            Instant.now());
-
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, "", null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, "", null, stats, info)));
+            Instant.now()
+        );
 
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, info))
+        );
 
         reason = "the task is failed";
         failedState = new TransformState(TransformTaskState.FAILED, IndexerState.STOPPED, null, 0, reason, null, null, true);
         withIdStateAndStats(transformId, failedState, stats);
 
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, info)));
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.FAILED, reason, null, stats, info))
+        );
     }
 
-
     public void testDeriveStats() {
         String transformId = "transform-with-stats";
-        String reason = "";
+        String reason = null;
         TransformIndexerStats stats = TransformIndexerStatsTests.randomStats();
-        TransformState runningState =
-            new TransformState(TransformTaskState.STARTED, IndexerState.INDEXING, null, 0, reason, null, null, true);
+        TransformState runningState = new TransformState(
+            TransformTaskState.STARTED,
+            IndexerState.INDEXING,
+            null,
+            0,
+            reason,
+            null,
+            null,
+            true
+        );
         withIdStateAndStats(transformId, runningState, stats);
         TransformCheckpointingInfo info = new TransformCheckpointingInfo(
             new TransformCheckpointStats(1, null, null, 1, 1),
             new TransformCheckpointStats(2, null, null, 2, 5),
             2,
-            Instant.now());
-
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING,
-                "transform is set to stop at the next checkpoint", null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING,
-                "transform is set to stop at the next checkpoint", null, stats, info)));
+            Instant.now()
+        );
 
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(
+                new TransformStats(
+                    transformId,
+                    TransformStats.State.STOPPING,
+                    "transform is set to stop at the next checkpoint",
+                    null,
+                    stats,
+                    TransformCheckpointingInfo.EMPTY
+                )
+            )
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(
+                new TransformStats(
+                    transformId,
+                    TransformStats.State.STOPPING,
+                    "transform is set to stop at the next checkpoint",
+                    null,
+                    stats,
+                    info
+                )
+            )
+        );
 
         reason = "foo";
         runningState = new TransformState(TransformTaskState.STARTED, IndexerState.INDEXING, null, 0, reason, null, null, true);
         withIdStateAndStats(transformId, runningState, stats);
 
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING, reason, null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING, reason, null, stats, info)));
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.STOPPING, reason, null, stats, info))
+        );
 
         // Stop at next checkpoint is false.
         runningState = new TransformState(TransformTaskState.STARTED, IndexerState.INDEXING, null, 0, reason, null, null, false);
         withIdStateAndStats(transformId, runningState, stats);
 
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, null),
-            equalTo(new TransformStats(transformId, TransformStats.State.INDEXING, reason, null, stats, TransformCheckpointingInfo.EMPTY)));
-        assertThat(TransportGetTransformStatsAction.deriveStats(task, info),
-            equalTo(new TransformStats(transformId, TransformStats.State.INDEXING, reason, null, stats, info)));
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, null),
+            equalTo(new TransformStats(transformId, TransformStats.State.INDEXING, reason, null, stats, TransformCheckpointingInfo.EMPTY))
+        );
+        assertThat(
+            TransportGetTransformStatsAction.deriveStats(task, info),
+            equalTo(new TransformStats(transformId, TransformStats.State.INDEXING, reason, null, stats, info))
+        );
     }
 
     private void withIdStateAndStats(String transformId, TransformState state, TransformIndexerStats stats) {