From 218e0389016e29dd01a6675cf6e622fbe7182b24 Mon Sep 17 00:00:00 2001 From: tge20 <72377965+tge20@users.noreply.github.com> Date: Mon, 14 Dec 2020 12:13:16 +0100 Subject: [PATCH] TSK-1468: Added validation of name and type of workbasket when updating Workbasket (#1366) * TSK-1468: Added validation of name and type of workbasket when updating Workbasket * TSK-1468: Improvements after review --- .../workbasket/api/WorkbasketService.java | 4 +- .../internal/WorkbasketServiceImpl.java | 66 +++++++++---------- .../workbasket/UpdateWorkbasketAccTest.java | 30 +++++++++ 3 files changed, 65 insertions(+), 35 deletions(-) diff --git a/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java b/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java index 4ec1e1f30..ea0d8f67e 100644 --- a/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java +++ b/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java @@ -65,13 +65,15 @@ public interface WorkbasketService { * * @param workbasket The Workbasket to update * @return the updated Workbasket + * @throws InvalidWorkbasketException if workbasket name or type is invalid * @throws NotAuthorizedException if the current user is not authorized to update the work basket * @throws WorkbasketNotFoundException if the workbasket cannot be found. * @throws ConcurrencyException if an attempt is made to update the workbasket and another user * updated it already */ Workbasket updateWorkbasket(Workbasket workbasket) - throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException; + throws InvalidWorkbasketException, NotAuthorizedException, WorkbasketNotFoundException, + ConcurrencyException; /** * Returns a new WorkbasketAccessItem which is not persisted. diff --git a/lib/taskana-core/src/main/java/pro/taskana/workbasket/internal/WorkbasketServiceImpl.java b/lib/taskana-core/src/main/java/pro/taskana/workbasket/internal/WorkbasketServiceImpl.java index d656e1876..8aee029f6 100644 --- a/lib/taskana-core/src/main/java/pro/taskana/workbasket/internal/WorkbasketServiceImpl.java +++ b/lib/taskana-core/src/main/java/pro/taskana/workbasket/internal/WorkbasketServiceImpl.java @@ -173,38 +173,30 @@ public class WorkbasketServiceImpl implements WorkbasketService { @Override public Workbasket updateWorkbasket(Workbasket workbasketToUpdate) - throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException { + throws NotAuthorizedException, WorkbasketNotFoundException, ConcurrencyException, + InvalidWorkbasketException { LOGGER.debug("entry to updateWorkbasket(Workbasket = {})", workbasketToUpdate); taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN); - - WorkbasketImpl workbasketImplToUpdate = null; + WorkbasketImpl workbasketImplToUpdate = (WorkbasketImpl) workbasketToUpdate; + validateNameAndType(workbasketToUpdate); try { - taskanaEngine.openConnection(); - workbasketImplToUpdate = (WorkbasketImpl) workbasketToUpdate; - Workbasket oldWorkbasket = this.getWorkbasket(workbasketImplToUpdate.getKey(), workbasketImplToUpdate.getDomain()); - checkModifiedHasNotChanged(oldWorkbasket, workbasketImplToUpdate); - workbasketImplToUpdate.setModified(Instant.now()); if (workbasketImplToUpdate.getId() == null || workbasketImplToUpdate.getId().isEmpty()) { - workbasketMapper.updateByKeyAndDomain(workbasketImplToUpdate); - } else { - workbasketMapper.update(workbasketImplToUpdate); } if (HistoryEventManager.isHistoryEnabled()) { - String details = ObjectAttributeChangeDetector.determineChangesInAttributes( oldWorkbasket, workbasketToUpdate); @@ -216,15 +208,13 @@ public class WorkbasketServiceImpl implements WorkbasketService { taskanaEngine.getEngine().getCurrentUserContext().getUserid(), details)); } + LOGGER.debug( "Method updateWorkbasket() updated workbasket '{}'", workbasketImplToUpdate.getId()); return workbasketImplToUpdate; - } finally { - taskanaEngine.returnConnection(); - LOGGER.debug("exit from updateWorkbasket(). Returning result {} ", workbasketImplToUpdate); } } @@ -841,7 +831,7 @@ public class WorkbasketServiceImpl implements WorkbasketService { LOGGER.debug("entry to deleteWorkbasket(workbasketId = {})", workbasketId); taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN); - validateWorkbasketId(workbasketId); + validateId(workbasketId); try { taskanaEngine.openConnection(); @@ -1104,16 +1094,6 @@ public class WorkbasketServiceImpl implements WorkbasketService { return accessItems; } - private void validateWorkbasketId(String workbasketId) throws InvalidArgumentException { - if (workbasketId == null) { - throw new InvalidArgumentException("The WorkbasketId can´t be NULL"); - } - - if (workbasketId.isEmpty()) { - throw new InvalidArgumentException("The WorkbasketId can´t be EMPTY for deleteWorkbasket()"); - } - } - private long getCountTasksByWorkbasketId(String workbasketId) { return taskanaEngine .getEngine() @@ -1158,20 +1138,17 @@ public class WorkbasketServiceImpl implements WorkbasketService { private void validateWorkbasket(Workbasket workbasket) throws InvalidWorkbasketException, DomainNotFoundException { // check that required properties (database not null) are set + validateNameAndType(workbasket); + if (workbasket.getId() == null || workbasket.getId().length() == 0) { throw new InvalidWorkbasketException("Id must not be null for " + workbasket); - } else if (workbasket.getKey() == null || workbasket.getKey().length() == 0) { - throw new InvalidWorkbasketException("Key must not be null for " + workbasket); } - if (workbasket.getName() == null || workbasket.getName().length() == 0) { - throw new InvalidWorkbasketException("Name must not be null for " + workbasket); + if (workbasket.getKey() == null || workbasket.getKey().length() == 0) { + throw new InvalidWorkbasketException("Key must not be null for " + workbasket); } if (workbasket.getDomain() == null) { throw new InvalidWorkbasketException("Domain must not be null for " + workbasket); } - if (workbasket.getType() == null) { - throw new InvalidWorkbasketException("Type must not be null for " + workbasket); - } if (!taskanaEngine.domainExists(workbasket.getDomain())) { throw new DomainNotFoundException( workbasket.getDomain(), @@ -1179,6 +1156,27 @@ public class WorkbasketServiceImpl implements WorkbasketService { } } + private void validateId(String workbasketId) throws InvalidArgumentException { + if (workbasketId == null) { + throw new InvalidArgumentException("The WorkbasketId can´t be NULL"); + } + if (workbasketId.isEmpty()) { + throw new InvalidArgumentException("The WorkbasketId can´t be EMPTY for deleteWorkbasket()"); + } + } + + private void validateNameAndType(Workbasket workbasket) throws InvalidWorkbasketException { + if (workbasket.getName() == null) { + throw new InvalidWorkbasketException("Name must not be NULL for " + workbasket); + } + if (workbasket.getName().length() == 0) { + throw new InvalidWorkbasketException("Name must not be EMPTY for " + workbasket); + } + if (workbasket.getType() == null) { + throw new InvalidWorkbasketException("Type must not be NULL for " + workbasket); + } + } + private List getPermissionsFromWorkbasketAccessItem( WorkbasketAccessItem workbasketAccessItem) { List permissions = new ArrayList<>(); @@ -1199,7 +1197,7 @@ public class WorkbasketServiceImpl implements WorkbasketService { taskanaEngine.getEngine().checkRoleMembership(TaskanaRole.BUSINESS_ADMIN, TaskanaRole.ADMIN); try { taskanaEngine.openConnection(); - validateWorkbasketId(workbasketId); + validateId(workbasketId); WorkbasketImpl workbasket = workbasketMapper.findById(workbasketId); workbasket.setMarkedForDeletion(true); workbasketMapper.update(workbasket); diff --git a/lib/taskana-core/src/test/java/acceptance/workbasket/UpdateWorkbasketAccTest.java b/lib/taskana-core/src/test/java/acceptance/workbasket/UpdateWorkbasketAccTest.java index e65c46e59..3ccc6bae2 100644 --- a/lib/taskana-core/src/test/java/acceptance/workbasket/UpdateWorkbasketAccTest.java +++ b/lib/taskana-core/src/test/java/acceptance/workbasket/UpdateWorkbasketAccTest.java @@ -18,6 +18,7 @@ import pro.taskana.common.test.security.WithAccessId; import pro.taskana.workbasket.api.WorkbasketCustomField; import pro.taskana.workbasket.api.WorkbasketService; import pro.taskana.workbasket.api.WorkbasketType; +import pro.taskana.workbasket.api.exceptions.InvalidWorkbasketException; import pro.taskana.workbasket.api.exceptions.WorkbasketNotFoundException; import pro.taskana.workbasket.api.models.Workbasket; import pro.taskana.workbasket.internal.models.WorkbasketImpl; @@ -57,6 +58,35 @@ class UpdateWorkbasketAccTest extends AbstractAccTest { assertThat(updatedWorkbasket.getModified()).isNotEqualTo(modified); } + @WithAccessId(user = "businessadmin") + @Test + void should_ThrowException_When_UpdatingWorkbasketWithInvalidName() throws Exception { + WorkbasketService workbasketService = taskanaEngine.getWorkbasketService(); + + WorkbasketImpl workbasket = + (WorkbasketImpl) workbasketService.getWorkbasket("GPK_KSC", "DOMAIN_A"); + workbasket.setName(null); + assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket)) + .isInstanceOf(InvalidWorkbasketException.class); + + workbasket.setName(""); + assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket)) + .isInstanceOf(InvalidWorkbasketException.class); + } + + @WithAccessId(user = "businessadmin") + @Test + void should_ThrowException_When_UpdatingWorkbasketWithTypeNull() throws Exception { + WorkbasketService workbasketService = taskanaEngine.getWorkbasketService(); + + WorkbasketImpl workbasket = + (WorkbasketImpl) workbasketService.getWorkbasket("GPK_KSC", "DOMAIN_A"); + workbasket.setType(null); + + assertThatThrownBy(() -> workbasketService.updateWorkbasket(workbasket)) + .isInstanceOf(InvalidWorkbasketException.class); + } + @WithAccessId(user = "businessadmin") @Test void testUpdateWorkbasketWithConcurrentModificationShouldThrowException() throws Exception {