From 6b6e9ba64060eac215d9477cbab32f9660705051 Mon Sep 17 00:00:00 2001 From: navodzoysa Date: Tue, 2 Aug 2022 20:22:40 +0530 Subject: [PATCH] Fix group names being able to be duplicated when renaming --- .../service/impl/GroupManagementServiceImpl.java | 6 +++++- .../service/impl/GroupManagementServiceImplTest.java | 2 +- .../mgt/core/dao/impl/AbstractGroupDAOImpl.java | 2 +- .../core/service/GroupManagementProviderService.java | 3 ++- .../service/GroupManagementProviderServiceImpl.java | 10 +++++++--- .../service/GroupManagementProviderServiceTest.java | 12 ++++++------ 6 files changed, 22 insertions(+), 13 deletions(-) diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImpl.java index 3fed8e7504..8927ea533d 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImpl.java @@ -218,7 +218,11 @@ public class GroupManagementServiceImpl implements GroupManagementService { log.error(msg, e); return Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(msg).build(); } catch (GroupNotExistException e) { - String msg = "There is another group already exists with name '" + deviceGroup.getName() + "'."; + String msg = "Group doesn't exist with ID '" + deviceGroup.getGroupId() + "'."; + log.warn(msg); + return Response.status(Response.Status.CONFLICT).entity(msg).build(); + } catch (GroupAlreadyExistException e) { + String msg = "Group already exists with name '" + deviceGroup.getName() + "'."; log.warn(msg); return Response.status(Response.Status.CONFLICT).entity(msg).build(); } diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImplTest.java b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImplTest.java index 2d430c09f6..9602d70f55 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImplTest.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.api/src/test/java/org/wso2/carbon/device/mgt/jaxrs/service/impl/GroupManagementServiceImplTest.java @@ -205,7 +205,7 @@ public class GroupManagementServiceImplTest { } @Test(description = "This method tests the functionality of updateGroup method under various conditions") - public void testUpdateGroup() throws GroupManagementException, GroupNotExistException { + public void testUpdateGroup() throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { PowerMockito.stub(PowerMockito.method(DeviceMgtAPIUtils.class, "getGroupManagementProviderService")) .toReturn(groupManagementProviderService); DeviceGroup deviceGroup = new DeviceGroup(); diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/dao/impl/AbstractGroupDAOImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/dao/impl/AbstractGroupDAOImpl.java index 7584b5fea7..06375ca2a1 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/dao/impl/AbstractGroupDAOImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/dao/impl/AbstractGroupDAOImpl.java @@ -727,7 +727,7 @@ public abstract class AbstractGroupDAOImpl implements GroupDAO { Connection conn = GroupManagementDAOFactory.getConnection(); String sql = "SELECT ID, DESCRIPTION, GROUP_NAME, OWNER, STATUS, PARENT_PATH FROM DM_GROUP " - + "WHERE GROUP_NAME = ? AND TENANT_ID = ?"; + + "WHERE LOWER(GROUP_NAME) = LOWER(?) AND TENANT_ID = ?"; stmt = conn.prepareStatement(sql); stmt.setString(1, groupName); stmt.setInt(2, tenantId); diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderService.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderService.java index c5e7417c86..5028f2560b 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderService.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderService.java @@ -71,7 +71,8 @@ public interface GroupManagementProviderService { * @param groupId of the group. * @throws GroupManagementException */ - void updateGroup(DeviceGroup deviceGroup, int groupId) throws GroupManagementException, GroupNotExistException; + void updateGroup(DeviceGroup deviceGroup, int groupId) + throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException; /** * Delete existing device group. diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceImpl.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceImpl.java index 2db3bd5f58..029191d246 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceImpl.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/main/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceImpl.java @@ -132,7 +132,7 @@ public class GroupManagementProviderServiceImpl implements GroupManagementProvid } GroupManagementDAOFactory.commitTransaction(); } else { - throw new GroupAlreadyExistException("Group exist with name " + deviceGroup.getName()); + throw new GroupAlreadyExistException("Group already exists with name '" + deviceGroup.getName() + "'."); } } catch (GroupManagementDAOException e) { GroupManagementDAOFactory.rollbackTransaction(); @@ -163,7 +163,7 @@ public class GroupManagementProviderServiceImpl implements GroupManagementProvid */ @Override public void updateGroup(DeviceGroup deviceGroup, int groupId) - throws GroupManagementException, GroupNotExistException { + throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { if (deviceGroup == null) { String msg = "Received incomplete data for updateGroup"; log.error(msg); @@ -177,6 +177,10 @@ public class GroupManagementProviderServiceImpl implements GroupManagementProvid GroupManagementDAOFactory.beginTransaction(); DeviceGroup existingGroup = this.groupDAO.getGroup(groupId, tenantId); if (existingGroup != null) { + boolean existingGroupName = this.groupDAO.getGroup(deviceGroup.getName(), tenantId) != null; + if (existingGroupName) { + throw new GroupAlreadyExistException("Group already exists with name '" + deviceGroup.getName() + "'."); + } List groupsToUpdate = new ArrayList<>(); String immediateParentID = StringUtils.substringAfterLast(existingGroup.getParentPath(), DeviceGroupConstants.HierarchicalGroup.SEPERATOR); String parentPath = ""; @@ -222,7 +226,7 @@ public class GroupManagementProviderServiceImpl implements GroupManagementProvid String msg = "Error occurred while initiating transaction."; log.error(msg, e); throw new GroupManagementException(msg, e); - } catch (GroupNotExistException ex) { + } catch (GroupNotExistException | GroupAlreadyExistException ex) { throw ex; } catch (Exception e) { String msg = "Error occurred in updating the device group with ID - '" + groupId + "'."; diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/test/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceTest.java b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/test/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceTest.java index 3073b5ee3d..977c1cb62d 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.core/src/test/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceTest.java +++ b/components/device-mgt/org.wso2.carbon.device.mgt.core/src/test/java/org/wso2/carbon/device/mgt/core/service/GroupManagementProviderServiceTest.java @@ -103,7 +103,7 @@ public class GroupManagementProviderServiceTest extends BaseDeviceManagementTest } @Test(dependsOnMethods = "createGroup") - public void updateGroup() throws GroupManagementException, GroupNotExistException { + public void updateGroup() throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { DeviceGroup deviceGroup = groupManagementProviderService.getGroup(TestUtils.createDeviceGroup1().getName(), false); deviceGroup.setName(deviceGroup.getName() + "_UPDATED"); groupManagementProviderService.updateGroup(deviceGroup, deviceGroup.getGroupId()); @@ -116,19 +116,19 @@ public class GroupManagementProviderServiceTest extends BaseDeviceManagementTest // Rename again to use in different place. @Test(dependsOnMethods = "updateGroup") - public void updateGroupSecondTime() throws GroupManagementException, GroupNotExistException { + public void updateGroupSecondTime() throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { DeviceGroup deviceGroup = groupManagementProviderService.getGroup(TestUtils.createDeviceGroup1().getName() + "_UPDATED", true); deviceGroup.setName(TestUtils.createDeviceGroup1().getName()); groupManagementProviderService.updateGroup(deviceGroup, deviceGroup.getGroupId()); } - @Test(dependsOnMethods = "createGroup", expectedExceptions = {GroupManagementException.class, GroupNotExistException.class}) - public void updateGroupError() throws GroupManagementException, GroupNotExistException { + @Test(dependsOnMethods = "createGroup", expectedExceptions = {GroupManagementException.class, GroupNotExistException.class, GroupAlreadyExistException.class}) + public void updateGroupError() throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { groupManagementProviderService.updateGroup(null, 1); } - @Test(dependsOnMethods = "createGroup", expectedExceptions = {GroupManagementException.class, GroupNotExistException.class}) - public void updateGroupErrorNotExist() throws GroupManagementException, GroupNotExistException { + @Test(dependsOnMethods = "createGroup", expectedExceptions = {GroupManagementException.class, GroupNotExistException.class, GroupAlreadyExistException.class}) + public void updateGroupErrorNotExist() throws GroupManagementException, GroupNotExistException, GroupAlreadyExistException { DeviceGroup deviceGroup = groupManagementProviderService.getGroup(TestUtils.createDeviceGroup2().getName(), false); deviceGroup.setName(deviceGroup.getName() + "_UPDATED"); groupManagementProviderService.updateGroup(deviceGroup, 6);