optimize SchedulerService.setScheduleState code (#3136)

* Optimize SchedulerService.setScheduleState code

* modify the test case to PowerMock

* modify code smell

* modify code smell

Co-authored-by: dailidong <dailidong66@gmail.com>

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/SchedulerService.java

@@ -19,6 +19,7 @@ package org.apache.dolphinscheduler.api.service;
 
 import org.apache.dolphinscheduler.api.dto.ScheduleParam;
 import org.apache.dolphinscheduler.api.enums.Status;
+import org.apache.dolphinscheduler.api.exceptions.ServiceException;
 import org.apache.dolphinscheduler.api.utils.PageInfo;
 import org.apache.dolphinscheduler.common.Constants;
 import org.apache.dolphinscheduler.common.enums.*;
@@ -333,10 +334,9 @@ public class SchedulerService extends BaseService {
         if(scheduleStatus == ReleaseState.ONLINE){
             // check process definition release state
             if(processDefinition.getReleaseState() != ReleaseState.ONLINE){
-                ProcessDefinition definition = processDefinitionMapper.selectById(scheduleObj.getProcessDefinitionId());
                 logger.info("not release process definition id: {} , name : {}",
                         processDefinition.getId(), processDefinition.getName());
-                putMsg(result, Status.PROCESS_DEFINE_NOT_RELEASE, definition.getName());
+                putMsg(result, Status.PROCESS_DEFINE_NOT_RELEASE, processDefinition.getName());
                 return result;
             }
             // check sub process definition release state
@@ -380,7 +380,7 @@ public class SchedulerService extends BaseService {
             switch (scheduleStatus) {
                 case ONLINE: {
                     logger.info("Call master client set schedule online, project id: {}, flow id: {},host: {}", project.getId(), processDefinition.getId(), masterServers);
-                    setSchedule(project.getId(), id);
+                    setSchedule(project.getId(), scheduleObj);
                     break;
                 }
                 case OFFLINE: {
@@ -395,7 +395,7 @@ public class SchedulerService extends BaseService {
             }
         } catch (Exception e) {
             result.put(Constants.MSG, scheduleStatus == ReleaseState.ONLINE ? "set online failure" : "set offline failure");
-            throw new RuntimeException(result.get(Constants.MSG).toString());
+            throw new ServiceException(result.get(Constants.MSG).toString());
         }
 
         putMsg(result, Status.SUCCESS);
@@ -472,15 +472,10 @@ public class SchedulerService extends BaseService {
         return result;
     }
 
-    public void setSchedule(int projectId, int scheduleId) throws RuntimeException{
+    public void setSchedule(int projectId, Schedule schedule) {
-        logger.info("set schedule, project id: {}, scheduleId: {}", projectId, scheduleId);
-
 
-        Schedule schedule = processService.querySchedule(scheduleId);
+        int scheduleId = schedule.getId();
-        if (schedule == null) {
+        logger.info("set schedule, project id: {}, scheduleId: {}", projectId, scheduleId);
-            logger.warn("process schedule info not exists");
-            return;
-        }
 
         Date startDate = schedule.getStartTime();
         Date endDate = schedule.getEndTime();
@@ -502,7 +497,7 @@ public class SchedulerService extends BaseService {
      * @param scheduleId schedule id
      * @throws RuntimeException runtime exception
      */
-    public static void deleteSchedule(int projectId, int scheduleId) throws RuntimeException{
+    public static void deleteSchedule(int projectId, int scheduleId) {
         logger.info("delete schedules of project id:{}, schedule id:{}", projectId, scheduleId);
 
         String jobName = QuartzExecutors.buildJobName(scheduleId);
@@ -510,7 +505,7 @@ public class SchedulerService extends BaseService {
 
         if(!QuartzExecutors.getInstance().deleteJob(jobName, jobGroupName)){
             logger.warn("set offline failure:projectId:{},scheduleId:{}",projectId,scheduleId);
-            throw new RuntimeException(String.format("set offline failure"));
+            throw new ServiceException("set offline failure");
         }
 
     }

dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SchedulerServiceTest.java

@@ -16,43 +16,183 @@
  */
 package org.apache.dolphinscheduler.api.service;
 
-import org.apache.dolphinscheduler.api.ApiApplicationServer;
 import org.apache.dolphinscheduler.api.enums.Status;
 import org.apache.dolphinscheduler.common.Constants;
 import org.apache.dolphinscheduler.common.enums.ReleaseState;
-import org.apache.dolphinscheduler.common.enums.UserType;
+import org.apache.dolphinscheduler.common.model.Server;
+import org.apache.dolphinscheduler.dao.entity.ProcessDefinition;
 import org.apache.dolphinscheduler.dao.entity.Project;
+import org.apache.dolphinscheduler.dao.entity.Schedule;
 import org.apache.dolphinscheduler.dao.entity.User;
+import org.apache.dolphinscheduler.dao.mapper.ProcessDefinitionMapper;
+import org.apache.dolphinscheduler.dao.mapper.ProjectMapper;
+import org.apache.dolphinscheduler.dao.mapper.ProjectUserMapper;
+import org.apache.dolphinscheduler.dao.mapper.ScheduleMapper;
+import org.apache.dolphinscheduler.service.process.ProcessService;
+import org.apache.dolphinscheduler.service.quartz.QuartzExecutors;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.slf4j.Logger;
+import org.mockito.InjectMocks;
-import org.slf4j.LoggerFactory;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.quartz.Scheduler;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.boot.test.context.SpringBootTest;
-import org.springframework.test.context.junit4.SpringRunner;
 
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
-@RunWith(SpringRunner.class)
+@RunWith(PowerMockRunner.class)
-@SpringBootTest(classes = ApiApplicationServer.class)
+@PrepareForTest(QuartzExecutors.class)
+
 public class SchedulerServiceTest {
-    private static final Logger logger = LoggerFactory.getLogger(ExecutorServiceTest.class);
 
-    @Autowired
+
+    @InjectMocks
     private SchedulerService schedulerService;
 
+
+    @Autowired
+    private ExecutorService executorService;
+
+    @Mock
+    private MonitorService monitorService;
+
+    @Mock
+    private ProcessService processService;
+
+    @Mock
+    private ScheduleMapper scheduleMapper;
+
+    @Mock
+    private ProjectMapper projectMapper;
+    @Mock
+    private ProjectUserMapper projectUserMapper;
+    @Mock
+    private ProjectService projectService;
+
+    @Mock
+    private ProcessDefinitionMapper processDefinitionMapper;
+
+    @Mock
+    private QuartzExecutors quartzExecutors;
+
+    @Mock
+    private Scheduler scheduler;
+
+
+    @Before
+    public void setUp() {
+
+        quartzExecutors = PowerMockito.mock(QuartzExecutors.class);
+        PowerMockito.mockStatic(QuartzExecutors.class);
+        try {
+            PowerMockito.doReturn(quartzExecutors).when(QuartzExecutors.class, "getInstance");
+        } catch (Exception e) {
+            e.printStackTrace();
+        }
+
+    }
+
     @Test
-    public void testSetScheduleState(){
+    public void testSetScheduleState() {
+
+
+        String projectName = "test";
         User loginUser = new User();
-        loginUser.setId(-1);
+        loginUser.setId(1);
-        loginUser.setUserType(UserType.GENERAL_USER);
+        Map<String, Object> result = new HashMap<String, Object>();
+        Project project = getProject(projectName);
+
+
+        ProcessDefinition processDefinition = new ProcessDefinition();
+
+        Schedule schedule = new Schedule();
+        schedule.setId(1);
+        schedule.setProcessDefinitionId(1);
+        schedule.setReleaseState(ReleaseState.OFFLINE);
+
+        List<Server> masterServers = new ArrayList<>();
+        masterServers.add(new Server());
+
+        Mockito.when(scheduleMapper.selectById(1)).thenReturn(schedule);
+
+        Mockito.when(projectMapper.queryByName(projectName)).thenReturn(project);
+
+        Mockito.when(processService.findProcessDefineById(1)).thenReturn(processDefinition);
+
+        //hash no auth
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE);
+
+        Mockito.when(projectService.hasProjectAndPerm(loginUser, project, result)).thenReturn(true);
+        //schedule not exists
+        result = schedulerService.setScheduleState(loginUser, projectName, 2, ReleaseState.ONLINE);
+        Assert.assertEquals(Status.SCHEDULE_CRON_NOT_EXISTS, result.get(Constants.STATUS));
+
+        //SCHEDULE_CRON_REALEASE_NEED_NOT_CHANGE
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.OFFLINE);
+        Assert.assertEquals(Status.SCHEDULE_CRON_REALEASE_NEED_NOT_CHANGE, result.get(Constants.STATUS));
+
+        //PROCESS_DEFINE_NOT_EXIST
+        schedule.setProcessDefinitionId(2);
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE);
+        Assert.assertEquals(Status.PROCESS_DEFINE_NOT_EXIST, result.get(Constants.STATUS));
+        schedule.setProcessDefinitionId(1);
+
+        // PROCESS_DEFINE_NOT_RELEASE
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE);
+        Assert.assertEquals(Status.PROCESS_DEFINE_NOT_RELEASE, result.get(Constants.STATUS));
+
+        processDefinition.setReleaseState(ReleaseState.ONLINE);
+        Mockito.when(processService.findProcessDefineById(1)).thenReturn(processDefinition);
+
+        //MASTER_NOT_EXISTS
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE);
+        Assert.assertEquals(Status.MASTER_NOT_EXISTS, result.get(Constants.STATUS));
+
+        //set master
+        Mockito.when(monitorService.getServerListFromZK(true)).thenReturn(masterServers);
+
+        //SUCCESS
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE);
+        Assert.assertEquals(Status.SUCCESS, result.get(Constants.STATUS));
+
+        //OFFLINE
+        Mockito.when(quartzExecutors.deleteJob(null, null)).thenReturn(true);
+        result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.OFFLINE);
+        Assert.assertEquals(Status.SUCCESS, result.get(Constants.STATUS));
+
+    }
+
+    @Test
+    public void testDeleteSchedule() {
+
+        Mockito.when(quartzExecutors.deleteJob("1", "1")).thenReturn(true);
+        Mockito.when(quartzExecutors.buildJobGroupName(1)).thenReturn("1");
+        Mockito.when(quartzExecutors.buildJobName(1)).thenReturn("1");
+       boolean flag = true;
+        try {
+            schedulerService.deleteSchedule(1, 1);
+        }catch (Exception e){
+            flag = false;
+        }
+        Assert.assertTrue(flag);
+
+    }
+
+    private Project getProject(String name) {
+
         Project project = new Project();
-        project.setName("project_test1");
+        project.setName(name);
-        project.setId(-1);
+        project.setUserId(1);
 
-        Map<String, Object> map = schedulerService.setScheduleState(loginUser, project.getName(), 44, ReleaseState.ONLINE);
+        return project;
-        Assert.assertEquals(Status.PROJECT_NOT_FOUNT, map.get(Constants.STATUS));
     }
 
 }