From 6c67e8c88d4e66a54de845d7c676d476295b14c4 Mon Sep 17 00:00:00 2001 From: Jose Ignacio Recuerda Cambil Date: Wed, 30 Jan 2019 11:08:16 +0100 Subject: [PATCH] TSK-771 - Invalid parentKey does not result in appropriate error message --- .../impl/ClassificationServiceImpl.java | 167 +++++++++++------- .../UpdateClassificationAccTest.java | 15 ++ 2 files changed, 121 insertions(+), 61 deletions(-) diff --git a/lib/taskana-core/src/main/java/pro/taskana/impl/ClassificationServiceImpl.java b/lib/taskana-core/src/main/java/pro/taskana/impl/ClassificationServiceImpl.java index 10fada41e..fda9984e4 100644 --- a/lib/taskana-core/src/main/java/pro/taskana/impl/ClassificationServiceImpl.java +++ b/lib/taskana-core/src/main/java/pro/taskana/impl/ClassificationServiceImpl.java @@ -166,70 +166,26 @@ public class ClassificationServiceImpl implements ClassificationService { ClassificationImpl classificationImpl = null; try { taskanaEngine.openConnection(); - classificationImpl = (ClassificationImpl) classification; - - // Check if current object is based on the newest (by modified) - Classification oldClassification = this.getClassification(classificationImpl.getKey(), - classificationImpl.getDomain()); - if (!oldClassification.getModified().equals(classificationImpl.getModified())) { - throw new ConcurrencyException( - "The current Classification has been modified while editing. The values can not be updated. Classification " - + classificationImpl.toString()); + if (classification.getKey().equals(classification.getParentKey())) { + throw new InvalidArgumentException( + "The classification " + classification.getName() + " has the same key and parentKey"); } + + classificationImpl = (ClassificationImpl) classification; + Classification oldClassification = this.getExistingClassificationAndVerifyTimestampHasNotChanged( + classificationImpl); classificationImpl.setModified(Instant.now()); this.initDefaultClassificationValues(classificationImpl); - // Update classification fields used by tasks if (oldClassification.getCategory() != classificationImpl.getCategory()) { - List taskSummaries = taskanaEngine.getTaskService() - .createTaskQuery() - .classificationIdIn(oldClassification.getId()) - .list(); - - boolean categoryChanged = !(oldClassification.getCategory() == null - ? classification.getCategory() == null - : oldClassification.getCategory().equals(classification.getCategory())); - if (!taskSummaries.isEmpty() && categoryChanged) { - List taskIds = new ArrayList<>(); - taskSummaries.stream().forEach(ts -> taskIds.add(ts.getTaskId())); - taskMapper.updateClassificationCategoryOnChange(taskIds, classificationImpl.getCategory()); - } - } - - // Check if parentId changed and object does exist - if (!oldClassification.getParentId().equals(classificationImpl.getParentId())) { - if (classificationImpl.getParentId() != null && !classificationImpl.getParentId().isEmpty()) { - this.getClassification(classificationImpl.getParentId()); - } - } - - // Check if parentKey changed and object does exist - if (!oldClassification.getParentKey().equals(classificationImpl.getParentKey())) { - if (classificationImpl.getParentKey() != null && !classificationImpl.getParentKey().isEmpty()) { - this.getClassification(classificationImpl.getParentKey(), classificationImpl.getDomain()); - } + this.updateCategoryOnAssociatedTasks(classificationImpl, oldClassification); } + this.checkExistenceOfParentClassification(oldClassification, classificationImpl); classificationMapper.update(classificationImpl); - boolean priorityChanged = oldClassification.getPriority() != classification.getPriority(); + this.createJobIfPriorityOrServiceLevelHasChanged(oldClassification, classificationImpl); - boolean serviceLevelChanged = !Objects.equals(oldClassification.getServiceLevel(), - classification.getServiceLevel()); - - if (priorityChanged || serviceLevelChanged) { - Map args = new HashMap<>(); - args.put(ClassificationChangedJob.CLASSIFICATION_ID, classificationImpl.getId()); - args.put(ClassificationChangedJob.PRIORITY_CHANGED, String.valueOf(priorityChanged)); - args.put(ClassificationChangedJob.SERVICE_LEVEL_CHANGED, - String.valueOf(serviceLevelChanged)); - ScheduledJob job = new ScheduledJob(); - job.setArguments(args); - job.setType(ScheduledJob.Type.CLASSIFICATIONCHANGEDJOB); - taskanaEngine.getJobService().createJob(job); - } - - LOGGER.debug("Method updateClassification() updated the classification {}.", - classificationImpl); + LOGGER.debug("Method updateClassification() updated the classification {}.", classificationImpl); return classification; } finally { taskanaEngine.returnConnection(); @@ -283,9 +239,13 @@ public class ClassificationServiceImpl implements ClassificationService { } if (classification.getCategory() != null - && !taskanaEngine.getConfiguration().getClassificationCategoriesByType(classification.getType()).contains(classification.getCategory())) { - throw new InvalidArgumentException("Given classification category " + classification.getCategory() + " with type " + classification.getType() - + " is not valid according to the configuration."); + && !taskanaEngine.getConfiguration() + .getClassificationCategoriesByType(classification.getType()) + .contains(classification.getCategory())) { + throw new InvalidArgumentException( + "Given classification category " + classification.getCategory() + " with type " + + classification.getType() + + " is not valid according to the configuration."); } if (classification.getDomain().isEmpty()) { @@ -308,8 +268,9 @@ public class ClassificationServiceImpl implements ClassificationService { if (!('p' == serviceLevelLower.charAt(0)) || !('d' == serviceLevelLower.charAt(serviceLevel.length() - 1))) { - throw new InvalidArgumentException("Invalid service level " + serviceLevel + ". Taskana only supports service levels that" - + " contain a number of whole days specified according to the format 'PnD' where n is the number of days"); + throw new InvalidArgumentException( + "Invalid service level " + serviceLevel + ". Taskana only supports service levels that" + + " contain a number of whole days specified according to the format 'PnD' where n is the number of days"); } } @@ -341,7 +302,7 @@ public class ClassificationServiceImpl implements ClassificationService { throw new ClassificationNotFoundException(key, domain, "Classification for null key and domain " + domain + " was not found."); } - LOGGER.debug("entry to getClassification(key = {}, domain = {})", key, domain); + Classification result = null; try { taskanaEngine.openConnection(); @@ -467,4 +428,88 @@ public class ClassificationServiceImpl implements ClassificationService { return e.getCause() instanceof SQLException && ((SQLException) e.getCause()).getSQLState().equals("23503"); } + /** + * Check if current object is based on the newest (by modified). + * + * @param classificationImpl the classification + * @return the old classification + */ + private Classification getExistingClassificationAndVerifyTimestampHasNotChanged( + ClassificationImpl classificationImpl) + throws ConcurrencyException, ClassificationNotFoundException { + Classification oldClassification = this.getClassification(classificationImpl.getKey(), + classificationImpl.getDomain()); + if (!oldClassification.getModified().equals(classificationImpl.getModified())) { + throw new ConcurrencyException( + "The current Classification has been modified while editing. The values can not be updated. Classification " + + classificationImpl.toString()); + } + return oldClassification; + } + + /** + * Update classification fields used by tasks. + * + * @param classificationImpl the new classification + * @param oldClassification the old classification + */ + private void updateCategoryOnAssociatedTasks(ClassificationImpl classificationImpl, + Classification oldClassification) { + List taskSummaries = taskanaEngine.getTaskService() + .createTaskQuery() + .classificationIdIn(oldClassification.getId()) + .list(); + + if (!taskSummaries.isEmpty()) { + List taskIds = new ArrayList<>(); + taskSummaries.forEach(ts -> taskIds.add(ts.getTaskId())); + taskMapper.updateClassificationCategoryOnChange(taskIds, classificationImpl.getCategory()); + } + } + + /** + * Check if parentId or parentKey were changed and if the classification exist. + * + * @param classificationImpl the new classification + * @param oldClassification the old classification + */ + private void checkExistenceOfParentClassification(Classification oldClassification, + ClassificationImpl classificationImpl) + throws ClassificationNotFoundException { + if (!oldClassification.getParentId().equals(classificationImpl.getParentId()) + && classificationImpl.getParentId() != null && !classificationImpl.getParentId().isEmpty()) { + this.getClassification(classificationImpl.getParentId()); + } + + if (!oldClassification.getParentKey().equals(classificationImpl.getParentKey()) + && classificationImpl.getParentKey() != null && !classificationImpl.getParentKey().isEmpty()) { + this.getClassification(classificationImpl.getParentKey(), classificationImpl.getDomain()); + } + } + + /** + * Check if priority or service level were changed and create the new job. + * + * @param classificationImpl the new classification + * @param oldClassification the old classification + */ + private void createJobIfPriorityOrServiceLevelHasChanged(Classification oldClassification, + ClassificationImpl classificationImpl) { + boolean priorityChanged = oldClassification.getPriority() != classificationImpl.getPriority(); + boolean serviceLevelChanged = !Objects.equals(oldClassification.getServiceLevel(), + classificationImpl.getServiceLevel()); + + if (priorityChanged || serviceLevelChanged) { + Map args = new HashMap<>(); + args.put(ClassificationChangedJob.CLASSIFICATION_ID, classificationImpl.getId()); + args.put(ClassificationChangedJob.PRIORITY_CHANGED, String.valueOf(priorityChanged)); + args.put(ClassificationChangedJob.SERVICE_LEVEL_CHANGED, + String.valueOf(serviceLevelChanged)); + ScheduledJob job = new ScheduledJob(); + job.setArguments(args); + job.setType(ScheduledJob.Type.CLASSIFICATIONCHANGEDJOB); + taskanaEngine.getJobService().createJob(job); + } + } + } diff --git a/lib/taskana-core/src/test/java/acceptance/classification/UpdateClassificationAccTest.java b/lib/taskana-core/src/test/java/acceptance/classification/UpdateClassificationAccTest.java index 28d55ad19..8686f5590 100644 --- a/lib/taskana-core/src/test/java/acceptance/classification/UpdateClassificationAccTest.java +++ b/lib/taskana-core/src/test/java/acceptance/classification/UpdateClassificationAccTest.java @@ -2,6 +2,7 @@ package acceptance.classification; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -276,6 +277,20 @@ public class UpdateClassificationAccTest extends AbstractAccTest { } + @WithAccessId( + userName = "dummy", + groupNames = {"businessadmin"}) + @Test(expected=InvalidArgumentException.class) + public void testUpdateClassificationWithSameKeyAndParentKey() + throws ClassificationNotFoundException, NotAuthorizedException, ConcurrencyException, InvalidArgumentException { + + ClassificationService classificationService = taskanaEngine.getClassificationService(); + Classification classification = classificationService.getClassification("T2100", "DOMAIN_A"); + + classification.setParentKey(classification.getKey()); + classificationService.updateClassification(classification); + } + private void validateNewTaskProperties(Instant before, List tasksWithP15D, TaskService taskService, DaysToWorkingDaysConverter converter, int serviceLevel) throws TaskNotFoundException, NotAuthorizedException { for (String taskId : tasksWithP15D) {