You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by ke...@apache.org on 2022/11/14 03:33:53 UTC
[dolphinscheduler] branch 3.1.2-prepare updated: cherry-pick Fix-12832][API] Fix update worker group exception group name already exists. #12874
This is an automated email from the ASF dual-hosted git repository.
kerwin pushed a commit to branch 3.1.2-prepare
in repository https://gitbox.apache.org/repos/asf/dolphinscheduler.git
The following commit(s) were added to refs/heads/3.1.2-prepare by this push:
new c592bcfbb6 cherry-pick Fix-12832][API] Fix update worker group exception group name already exists. #12874
c592bcfbb6 is described below
commit c592bcfbb6916201c4de713e8176bbbf62069453
Author: Kerwin <37...@users.noreply.github.com>
AuthorDate: Mon Nov 14 11:20:51 2022 +0800
cherry-pick Fix-12832][API] Fix update worker group exception group name already exists. #12874
---
.../api/service/impl/WorkerGroupServiceImpl.java | 28 +--
.../api/service/WorkerGroupServiceTest.java | 275 +++++++++++++++------
2 files changed, 217 insertions(+), 86 deletions(-)
diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java
index 0c1f04d235..285c5e82eb 100644
--- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java
+++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkerGroupServiceImpl.java
@@ -102,15 +102,11 @@ public class WorkerGroupServiceImpl extends BaseServiceImpl implements WorkerGro
return result;
}
Date now = new Date();
- WorkerGroup workerGroup;
+ WorkerGroup workerGroup = null;
if (id != 0) {
workerGroup = workerGroupMapper.selectById(id);
- // check exist
- if (workerGroup == null) {
- workerGroup = new WorkerGroup();
- workerGroup.setCreateTime(now);
- }
- } else {
+ }
+ if (workerGroup == null) {
workerGroup = new WorkerGroup();
workerGroup.setCreateTime(now);
}
@@ -151,23 +147,21 @@ public class WorkerGroupServiceImpl extends BaseServiceImpl implements WorkerGro
* @return boolean
*/
private boolean checkWorkerGroupNameExists(WorkerGroup workerGroup) {
+ // check database
List<WorkerGroup> workerGroupList = workerGroupMapper.queryWorkerGroupByName(workerGroup.getName());
if (CollectionUtils.isNotEmpty(workerGroupList)) {
- // new group has same name
+ // create group, the same group name exists in the database
if (workerGroup.getId() == null) {
return true;
}
- // check group id
- for (WorkerGroup group : workerGroupList) {
- if (Objects.equals(group.getId(), workerGroup.getId())) {
- return true;
- }
+ // update group, the database exists with the same group name except itself
+ Optional<WorkerGroup> sameNameWorkGroupOptional = workerGroupList.stream()
+ .filter(group -> !Objects.equals(group.getId(), workerGroup.getId())).findFirst();
+ if (sameNameWorkGroupOptional.isPresent()) {
+ return true;
}
}
- // check zookeeper
- String workerGroupPath =
- Constants.REGISTRY_DOLPHINSCHEDULER_WORKERS + Constants.SINGLE_SLASH + workerGroup.getName();
- return registryClient.exists(workerGroupPath);
+ return false;
}
/**
diff --git a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/WorkerGroupServiceTest.java b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/WorkerGroupServiceTest.java
index 7b199183a6..a219a40188 100644
--- a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/WorkerGroupServiceTest.java
+++ b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/WorkerGroupServiceTest.java
@@ -17,11 +17,18 @@
package org.apache.dolphinscheduler.api.service;
-import org.apache.dolphinscheduler.api.ApiApplicationServer;
+import static org.apache.dolphinscheduler.api.constants.ApiFuncIdentificationConstant.WORKER_GROUP_CREATE;
+import static org.apache.dolphinscheduler.api.constants.ApiFuncIdentificationConstant.WORKER_GROUP_DELETE;
+import static org.mockito.ArgumentMatchers.any;
+
import org.apache.dolphinscheduler.api.enums.Status;
+import org.apache.dolphinscheduler.api.permission.ResourcePermissionCheckService;
+import org.apache.dolphinscheduler.api.service.impl.BaseServiceImpl;
import org.apache.dolphinscheduler.api.service.impl.WorkerGroupServiceImpl;
+import org.apache.dolphinscheduler.api.utils.Result;
import org.apache.dolphinscheduler.common.constants.Constants;
-import org.apache.dolphinscheduler.common.enums.ProfileType;
+import org.apache.dolphinscheduler.common.enums.AuthorizationType;
+import org.apache.dolphinscheduler.common.enums.NodeType;
import org.apache.dolphinscheduler.common.enums.UserType;
import org.apache.dolphinscheduler.dao.entity.ProcessInstance;
import org.apache.dolphinscheduler.dao.entity.User;
@@ -32,95 +39,235 @@ import org.apache.dolphinscheduler.dao.mapper.WorkerGroupMapper;
import org.apache.dolphinscheduler.service.registry.RegistryClient;
import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.boot.test.context.SpringBootTest;
-import org.springframework.boot.test.mock.mockito.MockBean;
-import org.springframework.test.context.ActiveProfiles;
-import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
-
-@ActiveProfiles(value = {ProfileType.H2})
-@RunWith(SpringJUnit4ClassRunner.class)
-@SpringBootTest(classes = ApiApplicationServer.class)
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
public class WorkerGroupServiceTest {
- @MockBean(name = "registryClient")
- private RegistryClient registryClient;
+ private static final Logger logger = LoggerFactory.getLogger(WorkerGroupServiceTest.class);
+
+ private static final Logger baseServiceLogger = LoggerFactory.getLogger(BaseServiceImpl.class);
- @Autowired
+ private static final Logger serviceLogger = LoggerFactory.getLogger(WorkerGroupService.class);
+
+ @InjectMocks
private WorkerGroupServiceImpl workerGroupService;
- @MockBean(name = "workerGroupMapper")
+ @Mock
private WorkerGroupMapper workerGroupMapper;
- @MockBean(name = "processInstanceMapper")
+ @Mock
private ProcessInstanceMapper processInstanceMapper;
- private String groupName = "groupName000001";
+ @Mock
+ private RegistryClient registryClient;
+
+ @Mock
+ private ResourcePermissionCheckService resourcePermissionCheckService;
- private User loginUSer;
@Mock
private EnvironmentWorkerGroupRelationMapper environmentWorkerGroupRelationMapper;
- @Before
- public void init(){
- loginUSer = new User();
- loginUSer.setUserType(UserType.ADMIN_USER);
+ private final String GROUP_NAME = "testWorkerGroup";
+
+ private User getLoginUser() {
+ User loginUser = new User();
+ loginUser.setUserType(UserType.GENERAL_USER);
+ loginUser.setUserName("workerGroupTestUser");
+ loginUser.setId(1);
+ return loginUser;
+ }
+
+ @Test
+ public void giveNoPermission_whenSaveWorkerGroup_expectNoOperation() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_CREATE, baseServiceLogger)).thenReturn(false);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(false);
+ Map<String, Object> result =
+ workerGroupService.saveWorkerGroup(loginUser, 1, GROUP_NAME, "localhost:0000", "test group", "");
+ Assertions.assertEquals(Status.USER_NO_OPERATION_PERM.getCode(),
+ ((Status) result.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveNullName_whenSaveWorkerGroup_expectNAME_NULL() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_CREATE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ Map<String, Object> result =
+ workerGroupService.saveWorkerGroup(loginUser, 1, "", "localhost:0000", "test group", "");
+ Assertions.assertEquals(Status.NAME_NULL.getCode(),
+ ((Status) result.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveSameUserName_whenSaveWorkerGroup_expectNAME_EXIST() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_CREATE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(null);
+ List<WorkerGroup> workerGroupList = new ArrayList<WorkerGroup>();
+ workerGroupList.add(getWorkerGroup(1));
+ Mockito.when(workerGroupMapper.queryWorkerGroupByName(GROUP_NAME)).thenReturn(workerGroupList);
+
+ Map<String, Object> result =
+ workerGroupService.saveWorkerGroup(loginUser, 1, GROUP_NAME, "localhost:0000", "test group", "");
+ Assertions.assertEquals(Status.NAME_EXIST.getCode(),
+ ((Status) result.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveInvalidAddress_whenSaveWorkerGroup_expectADDRESS_INVALID() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_CREATE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(null);
+ Mockito.when(workerGroupMapper.queryWorkerGroupByName(GROUP_NAME)).thenReturn(null);
+ Map<String, String> serverMaps = new HashMap<>();
+ serverMaps.put("localhost1:0000", "");
+ Mockito.when(registryClient.getServerMaps(NodeType.WORKER)).thenReturn(serverMaps);
+
+ Map<String, Object> result =
+ workerGroupService.saveWorkerGroup(loginUser, 1, GROUP_NAME, "localhost:0000", "test group", "");
+ Assertions.assertEquals(Status.WORKER_ADDRESS_INVALID.getCode(),
+ ((Status) result.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveValidWorkerGroup_whenSaveWorkerGroup_expectSuccess() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_CREATE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(null);
+ Mockito.when(workerGroupMapper.queryWorkerGroupByName(GROUP_NAME)).thenReturn(null);
+ Map<String, String> serverMaps = new HashMap<>();
+ serverMaps.put("localhost:0000", "");
+ Mockito.when(registryClient.getServerMaps(NodeType.WORKER)).thenReturn(serverMaps);
+ Mockito.when(workerGroupMapper.insert(any())).thenReturn(1);
+
+ Map<String, Object> result =
+ workerGroupService.saveWorkerGroup(loginUser, 1, GROUP_NAME, "localhost:0000", "test group", "");
+ Assertions.assertEquals(Status.SUCCESS.getCode(),
+ ((Status) result.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveValidParams_whenQueryAllGroupPaging_expectSuccess() {
+ User loginUser = getLoginUser();
+ Set<Integer> ids = new HashSet<>();
+ ids.add(1);
+ List<WorkerGroup> workerGroups = new ArrayList<>();
+ workerGroups.add(getWorkerGroup(1));
+ Mockito.when(resourcePermissionCheckService.userOwnedResourceIdsAcquisition(AuthorizationType.WORKER_GROUP,
+ loginUser.getId(), serviceLogger)).thenReturn(ids);
+ Mockito.when(workerGroupMapper.selectBatchIds(ids)).thenReturn(workerGroups);
+ Set<String> activeWorkerNodes = new HashSet<>();
+ activeWorkerNodes.add("localhost:12345");
+ activeWorkerNodes.add("localhost:23456");
+ Mockito.when(registryClient.getServerNodeSet(NodeType.WORKER)).thenReturn(activeWorkerNodes);
+
+ Result result = workerGroupService.queryAllGroupPaging(loginUser, 1, 1, null);
+ Assertions.assertEquals(result.getCode(), Status.SUCCESS.getCode());
}
@Test
public void testQueryAllGroup() {
- Map<String, Object> result = workerGroupService.queryAllGroup(loginUSer);
+ Map<String, Object> result = workerGroupService.queryAllGroup(getLoginUser());
List<String> workerGroups = (List<String>) result.get(Constants.DATA_LIST);
- Assert.assertEquals(workerGroups.size(), 1);
+ Assertions.assertEquals(workerGroups.size(), 1);
}
- /**
- * delete group by id
- */
@Test
- public void testDeleteWorkerGroupById() {
- User user = new User();
- user.setId(1);
- user.setUserType(UserType.ADMIN_USER);
- WorkerGroup wg2 = getWorkerGroup(2);
- Mockito.when(workerGroupMapper.selectById(2)).thenReturn(wg2);
- Mockito.when(processInstanceMapper.queryByWorkerGroupNameAndStatus(wg2.getName(), org.apache.dolphinscheduler.service.utils.Constants.NOT_TERMINATED_STATES)).thenReturn(getProcessInstanceList());
- Map<String, Object> result = workerGroupService.deleteWorkerGroupById(user, 1);
- Assert.assertEquals(Status.DELETE_WORKER_GROUP_NOT_EXIST.getCode(), ((Status) result.get(Constants.STATUS)).getCode());
- result = workerGroupService.deleteWorkerGroupById(user, 2);
- Assert.assertEquals(Status.DELETE_WORKER_GROUP_BY_ID_FAIL.getCode(), ((Status) result.get(Constants.STATUS)).getCode());
- // correct
- WorkerGroup wg3 = getWorkerGroup(3);
- Mockito.when(workerGroupMapper.selectById(3)).thenReturn(wg3);
- Mockito.when(processInstanceMapper.queryByWorkerGroupNameAndStatus(wg3.getName(), org.apache.dolphinscheduler.service.utils.Constants.NOT_TERMINATED_STATES)).thenReturn(new ArrayList<>());
- result = workerGroupService.deleteWorkerGroupById(user, 3);
- Assert.assertEquals(Status.SUCCESS.getMsg(), result.get(Constants.MSG));
+ public void giveNotExistsWorkerGroup_whenDeleteWorkerGroupById_expectNotExists() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_DELETE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(null);
+
+ Map<String, Object> notExistResult = workerGroupService.deleteWorkerGroupById(loginUser, 1);
+ Assertions.assertEquals(Status.DELETE_WORKER_GROUP_NOT_EXIST.getCode(),
+ ((Status) notExistResult.get(Constants.STATUS)).getCode());
}
- /**
- * get processInstances
- */
- private List<ProcessInstance> getProcessInstanceList() {
- List<ProcessInstance> processInstances = new ArrayList<>();
- processInstances.add(new ProcessInstance());
- return processInstances;
+ @Test
+ public void giveRunningProcess_whenDeleteWorkerGroupById_expectFailed() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_DELETE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ WorkerGroup workerGroup = getWorkerGroup(1);
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(workerGroup);
+ ProcessInstance processInstance = new ProcessInstance();
+ processInstance.setId(1);
+ List<ProcessInstance> processInstances = new ArrayList<ProcessInstance>();
+ processInstances.add(processInstance);
+ Mockito.when(processInstanceMapper.queryByWorkerGroupNameAndStatus(workerGroup.getName(),
+ org.apache.dolphinscheduler.service.utils.Constants.NOT_TERMINATED_STATES))
+ .thenReturn(processInstances);
+
+ Map<String, Object> deleteFailed = workerGroupService.deleteWorkerGroupById(loginUser, 1);
+ Assertions.assertEquals(Status.DELETE_WORKER_GROUP_BY_ID_FAIL.getCode(),
+ ((Status) deleteFailed.get(Constants.STATUS)).getCode());
+ }
+
+ @Test
+ public void giveValidParams_whenDeleteWorkerGroupById_expectSuccess() {
+ User loginUser = getLoginUser();
+ Mockito.when(resourcePermissionCheckService.operationPermissionCheck(AuthorizationType.WORKER_GROUP, null, loginUser.getId(),
+ WORKER_GROUP_DELETE, baseServiceLogger)).thenReturn(true);
+ Mockito.when(resourcePermissionCheckService.resourcePermissionCheck(AuthorizationType.WORKER_GROUP, null, 1,
+ baseServiceLogger)).thenReturn(true);
+ WorkerGroup workerGroup = getWorkerGroup(1);
+ Mockito.when(workerGroupMapper.selectById(1)).thenReturn(workerGroup);
+ Mockito.when(processInstanceMapper.queryByWorkerGroupNameAndStatus(workerGroup.getName(),
+ org.apache.dolphinscheduler.service.utils.Constants.NOT_TERMINATED_STATES)).thenReturn(null);
+ Mockito.when(workerGroupMapper.deleteById(1)).thenReturn(1);
+ Mockito.when(processInstanceMapper.updateProcessInstanceByWorkerGroupName(workerGroup.getName(), ""))
+ .thenReturn(1);
+ Mockito.when(environmentWorkerGroupRelationMapper.queryByWorkerGroupName(workerGroup.getName()))
+ .thenReturn(null);
+ Map<String, Object> successResult = workerGroupService.deleteWorkerGroupById(loginUser, 1);
+ Assertions.assertEquals(Status.SUCCESS.getCode(),
+ ((Status) successResult.get(Constants.STATUS)).getCode());
}
@Test
public void testQueryAllGroupWithDefault() {
- Map<String, Object> result = workerGroupService.queryAllGroup(loginUSer);
+ Map<String, Object> result = workerGroupService.queryAllGroup(getLoginUser());
List<String> workerGroups = (List<String>) result.get(Constants.DATA_LIST);
- Assert.assertEquals(1, workerGroups.size());
- Assert.assertEquals("default", workerGroups.toArray()[0]);
+ Assertions.assertEquals(1, workerGroups.size());
+ Assertions.assertEquals("default", workerGroups.toArray()[0]);
}
/**
@@ -128,19 +275,9 @@ public class WorkerGroupServiceTest {
*/
private WorkerGroup getWorkerGroup(int id) {
WorkerGroup workerGroup = new WorkerGroup();
- workerGroup.setName(groupName);
+ workerGroup.setName(GROUP_NAME);
workerGroup.setId(id);
return workerGroup;
}
- private WorkerGroup getWorkerGroup() {
- return getWorkerGroup(1);
- }
-
- private List<WorkerGroup> getList() {
- List<WorkerGroup> list = new ArrayList<>();
- list.add(getWorkerGroup());
- return list;
- }
-
}