From 86ffc1f3f25f066b16142ef8e5f641a030398b1e Mon Sep 17 00:00:00 2001
From: LOUKHNATI Mohamed Khalil <34715586+medkhabt@users.noreply.github.com>
Date: Wed, 16 Jun 2021 11:21:41 +0100
Subject: [PATCH] [Bug][WorkerServer] SqlTask NullPointerException (#5556)

* [Bug][WorkerServer] add null checks to sqlParamsMap and sqlParamsMap.get(i), Add test to verify it

* Change Test Name

* Fix Code style issues, Modify checking null for Sql params in sqlParamsMap with clearer logging.
---
 .../server/worker/task/sql/SqlTask.java       | 21 +++++++++---
 .../server/worker/task/sql/SqlTaskTest.java   | 33 ++++++++++++++++++-
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java b/dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java
index 75194a36f..0716645ef 100644
--- a/dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java
+++ b/dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java
@@ -473,8 +473,16 @@ public class SqlTask extends AbstractTask {
             String paramName = m.group(1);
             Property prop = paramsPropsMap.get(paramName);
 
-            sqlParamsMap.put(index, prop);
-            index++;
+            if (prop == null) {
+                logger.error("setSqlParamsMap: No Property with paramName: {} is found in paramsPropsMap of task instance"
+                        + " with id: {}. So couldn't put Property in sqlParamsMap.", paramName, taskExecutionContext.getTaskInstanceId());
+            }
+            else {
+                sqlParamsMap.put(index, prop);
+                index++;
+                logger.info("setSqlParamsMap: Property with paramName: {} put in sqlParamsMap of content {} successfully.", paramName, content);
+            }
+
         }
     }
 
@@ -490,8 +498,13 @@ public class SqlTask extends AbstractTask {
         //parameter print style
         logger.info("after replace sql , preparing : {}", formatSql);
         StringBuilder logPrint = new StringBuilder("replaced sql , parameters:");
-        for (int i = 1; i <= sqlParamsMap.size(); i++) {
-            logPrint.append(sqlParamsMap.get(i).getValue() + "(" + sqlParamsMap.get(i).getType() + ")");
+        if (sqlParamsMap == null) {
+            logger.info("printReplacedSql: sqlParamsMap is null.");
+        }
+        else {
+            for (int i = 1; i <= sqlParamsMap.size(); i++) {
+                logPrint.append(sqlParamsMap.get(i).getValue() + "(" + sqlParamsMap.get(i).getType() + ")");
+            }
         }
         logger.info("Sql Params are {}", logPrint);
     }
diff --git a/dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTaskTest.java b/dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTaskTest.java
index 7746d98d1..0eb0f98ec 100644
--- a/dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTaskTest.java
+++ b/dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTaskTest.java
@@ -17,8 +17,13 @@
 
 package org.apache.dolphinscheduler.server.worker.task.sql;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 import org.apache.dolphinscheduler.common.Constants;
 import org.apache.dolphinscheduler.common.datasource.DatasourceUtil;
+import org.apache.dolphinscheduler.common.process.Property;
 import org.apache.dolphinscheduler.common.task.sql.SqlParameters;
 import org.apache.dolphinscheduler.common.utils.ParameterUtils;
 import org.apache.dolphinscheduler.dao.AlertDao;
@@ -34,6 +39,8 @@ import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.ResultSetMetaData;
 import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -118,7 +125,7 @@ public class SqlTaskTest {
         PowerMockito.when(DatasourceUtil.getConnection(Mockito.any(), Mockito.any())).thenReturn(connection);
 
         sqlTask.handle();
-        Assert.assertEquals(Constants.EXIT_CODE_SUCCESS, sqlTask.getExitStatusCode());
+        assertEquals(Constants.EXIT_CODE_SUCCESS, sqlTask.getExitStatusCode());
     }
 
     @Test
@@ -150,6 +157,30 @@ public class SqlTaskTest {
         Assert.assertNotNull(result);
     }
 
+    @Test
+    public void shouldntThrowNullPointerException_When_SqlParamsMapIsNull_printReplacedSql() {
+        try {
+            sqlTask.printReplacedSql("", "", "", null);
+            assertTrue(true);
+        } catch (NullPointerException err) {
+            fail();
+        }
+    }
+
+    @Test
+    public void shouldntPutPropertyInSqlParamsMap_When_paramNameIsNotFoundInparamsPropsMap_setSqlParamsMap() {
+        Map<Integer, Property> sqlParamsMap = new HashMap<>();
+        Map<String, Property> paramsPropsMap = new HashMap<>();
+        paramsPropsMap.put("validPropertyName", new Property());
+
+        taskExecutionContext = PowerMockito.mock(TaskExecutionContext.class);
+        PowerMockito.when(taskExecutionContext.getTaskInstanceId()).thenReturn(1);
+
+        sqlTask.setSqlParamsMap("notValidPropertyName", "(notValidPropertyName)", sqlParamsMap, paramsPropsMap);
+
+        assertEquals(0, sqlParamsMap.size());
+    }
+
     @Test
     public void testQueryBySQLUsingLimit() throws Exception {
         TaskExecutionContext localTaskExecutionContext;
-- 
GitLab