From d41407a45981a88659ecd65d7ccd6e60dec68fcf Mon Sep 17 00:00:00 2001 From: Benjamin Eckstein <13351939+benjamineckstein@users.noreply.github.com> Date: Wed, 11 Dec 2019 14:50:26 +0100 Subject: [PATCH] TSK-987: Fix multithread problem identified by spotbugs --- .../impl/DaysToWorkingDaysConverter.java | 47 +++++++++++-------- .../acceptance/task/CreateTaskAccTest.java | 37 +++++++-------- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/lib/taskana-core/src/main/java/pro/taskana/impl/DaysToWorkingDaysConverter.java b/lib/taskana-core/src/main/java/pro/taskana/impl/DaysToWorkingDaysConverter.java index c352fd316..3092a8e3a 100644 --- a/lib/taskana-core/src/main/java/pro/taskana/impl/DaysToWorkingDaysConverter.java +++ b/lib/taskana-core/src/main/java/pro/taskana/impl/DaysToWorkingDaysConverter.java @@ -11,7 +11,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Optional; +import java.util.Objects; import java.util.Set; import java.util.stream.LongStream; import java.util.stream.Stream; @@ -33,13 +33,12 @@ import pro.taskana.impl.util.LoggerUtils; public final class DaysToWorkingDaysConverter { private static final Logger LOGGER = LoggerFactory.getLogger(TaskMonitorServiceImpl.class); - private static DaysToWorkingDaysConverter instance; + private static boolean germanHolidaysEnabled; + private static Set customHolidays = new HashSet<>(); private List positiveDaysToWorkingDays; private List negativeDaysToWorkingDays; private Instant dateCreated; private LocalDate easterSunday; - private static boolean germanHolidaysEnabled; - private static Set customHolidays = new HashSet<>(); private DaysToWorkingDaysConverter(List columnHeaders, Instant referenceDate) { @@ -49,6 +48,11 @@ public final class DaysToWorkingDaysConverter { negativeDaysToWorkingDays = generateNegativeDaysToWorkingDays(columnHeaders, referenceDate); } + public static DaysToWorkingDaysConverter initialize() + throws InvalidArgumentException { + return initialize(Collections.singletonList(new TimeIntervalColumnHeader(0)), Instant.now()); + } + /** * Initializes the DaysToWorkingDaysConverter for a list of {@link TimeIntervalColumnHeader}s and the current day. A * new table is only created if there are bigger limits or the date has changed. @@ -83,22 +87,8 @@ public final class DaysToWorkingDaysConverter { if (referenceDate == null) { throw new InvalidArgumentException("ReferenceDate can´t be used as NULL-Parameter"); } - int largesLowerLimit = TimeIntervalColumnHeader.getLargestLowerLimit(columnHeaders); - int smallestUpperLimit = TimeIntervalColumnHeader.getSmallestUpperLimit(columnHeaders); - if (instance == null - || !instance.positiveDaysToWorkingDays.contains(largesLowerLimit) - || !instance.negativeDaysToWorkingDays.contains(smallestUpperLimit) - || !instance.dateCreated.truncatedTo(DAYS).equals(referenceDate.truncatedTo(DAYS))) { - instance = new DaysToWorkingDaysConverter(columnHeaders, referenceDate); - LOGGER.debug("Create new converter for the values from {} until {} for the date: {}.", largesLowerLimit, - smallestUpperLimit, instance.dateCreated); - } - return instance; - } - - public static Optional getLastCreatedInstance() { - return Optional.ofNullable(instance); + return new DaysToWorkingDaysConverter(columnHeaders, referenceDate); } public static void setGermanPublicHolidaysEnabled(boolean germanPublicHolidaysEnabled) { @@ -323,4 +313,23 @@ public final class DaysToWorkingDaysConverter { return date.getDayOfMonth() == day && date.getMonthValue() == month; } } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + DaysToWorkingDaysConverter that = (DaysToWorkingDaysConverter) o; + return positiveDaysToWorkingDays.equals(that.positiveDaysToWorkingDays) + && negativeDaysToWorkingDays.equals(that.negativeDaysToWorkingDays) + && dateCreated.equals(that.dateCreated); + } + + @Override + public int hashCode() { + return Objects.hash(positiveDaysToWorkingDays, negativeDaysToWorkingDays, dateCreated); + } } diff --git a/lib/taskana-core/src/test/java/acceptance/task/CreateTaskAccTest.java b/lib/taskana-core/src/test/java/acceptance/task/CreateTaskAccTest.java index 73359ec6f..5620dcf06 100644 --- a/lib/taskana-core/src/test/java/acceptance/task/CreateTaskAccTest.java +++ b/lib/taskana-core/src/test/java/acceptance/task/CreateTaskAccTest.java @@ -14,7 +14,6 @@ import java.time.temporal.ChronoUnit; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Optional; import java.util.function.Consumer; import org.apache.ibatis.session.Configuration; @@ -151,7 +150,7 @@ class CreateTaskAccTest extends AbstractAccTest { userName = "user_1_1", groupNames = {"group_1"}) @Test - void testCreateTaskWithValidPlannedAndDue() throws ClassificationNotFoundException { + void testCreateTaskWithValidPlannedAndDue() throws ClassificationNotFoundException, InvalidArgumentException { Classification classification = classificationService.getClassification("T2100", "DOMAIN_A"); long serviceLevelDays = Duration.parse(classification.getServiceLevel()).toDays(); @@ -162,16 +161,14 @@ class CreateTaskAccTest extends AbstractAccTest { newTask.setPrimaryObjRef(createObjectReference("COMPANY_A", "SYSTEM_A", "INSTANCE_A", "VNR", "1234567")); newTask.setOwner("user_1_1"); - Optional converter = DaysToWorkingDaysConverter.getLastCreatedInstance(); + DaysToWorkingDaysConverter converter = DaysToWorkingDaysConverter.initialize(); //TODO: this is a temporal bug fix because we did not define what happens when a task is planned on the weekend. - long i = converter.get().convertWorkingDaysToDays(instantPlanned, 0); + long i = converter.convertWorkingDaysToDays(instantPlanned, 0); newTask.setPlanned(instantPlanned.plus(Duration.ofDays(i))); //due date according to service level - Instant shouldBeDueDate = converter - .map(c -> c.convertWorkingDaysToDays(newTask.getPlanned(), serviceLevelDays)) - .map( - calendarDays -> newTask.getPlanned().plus(Duration.ofDays(calendarDays))).orElseThrow( - RuntimeException::new); + long calendarDays = converter.convertWorkingDaysToDays(newTask.getPlanned(), serviceLevelDays); + Instant shouldBeDueDate = newTask.getPlanned().plus(Duration.ofDays(calendarDays)); + newTask.setDue(shouldBeDueDate); Assertions.assertDoesNotThrow(() -> taskService.createTask(newTask)); } @@ -411,12 +408,11 @@ class CreateTaskAccTest extends AbstractAccTest { assertNotNull(readTask); assertEquals(planned, readTask.getPlanned()); - Optional shouldBeDueDate = DaysToWorkingDaysConverter.getLastCreatedInstance() - .map(converter -> converter.convertWorkingDaysToDays(readTask.getPlanned(), serviceLevelDays)) - .map( - calendarDays -> readTask.getPlanned().plus(Duration.ofDays(calendarDays))); - assertTrue(shouldBeDueDate.isPresent()); - assertEquals(readTask.getDue(), shouldBeDueDate.get()); + long calendarDays = DaysToWorkingDaysConverter.initialize() + .convertWorkingDaysToDays(readTask.getPlanned(), serviceLevelDays); + + Instant shouldBeDueDate = readTask.getPlanned().plus(Duration.ofDays(calendarDays)); + assertEquals(readTask.getDue(), shouldBeDueDate); } @WithAccessId( @@ -445,14 +441,13 @@ class CreateTaskAccTest extends AbstractAccTest { assertNotNull(readTask); assertEquals(due, readTask.getDue()); - Optional calendarDaysToSubstract = DaysToWorkingDaysConverter.getLastCreatedInstance() - .map(converter -> converter.convertWorkingDaysToDays(due, -serviceLevelDays)); + long calendarDaysToSubstract = DaysToWorkingDaysConverter.initialize() + .convertWorkingDaysToDays(due, -serviceLevelDays); - assertTrue(calendarDaysToSubstract.isPresent()); - assertTrue(calendarDaysToSubstract.get() < 0); - assertTrue(calendarDaysToSubstract.get() <= -serviceLevelDays); + assertTrue(calendarDaysToSubstract < 0); + assertTrue(calendarDaysToSubstract <= -serviceLevelDays); - Instant shouldBePlannedDate = due.plus(Duration.ofDays(calendarDaysToSubstract.get())); + Instant shouldBePlannedDate = due.plus(Duration.ofDays(calendarDaysToSubstract)); assertEquals(readTask.getPlanned(), shouldBePlannedDate); }