fix concurrent TimeClock issue

src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockController.java

@@ -4,9 +4,11 @@
 import de.focusshift.zeiterfassung.user.UserId;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.validation.Valid;
+import org.slf4j.Logger;
 import org.springframework.security.core.annotation.AuthenticationPrincipal;
 import org.springframework.security.oauth2.core.oidc.user.DefaultOidcUser;
 import org.springframework.stereotype.Controller;
+import org.springframework.transaction.TransactionSystemException;
 import org.springframework.ui.Model;
 import org.springframework.validation.BindingResult;
 import org.springframework.web.bind.annotation.GetMapping;
@@ -21,7 +23,8 @@
 import java.util.Optional;
 
 import static java.lang.String.format;
-import static org.springframework.http.HttpStatus.CONFLICT;
+import static java.lang.invoke.MethodHandles.lookup;
+import static org.slf4j.LoggerFactory.getLogger;
 import static org.springframework.http.HttpStatus.PRECONDITION_REQUIRED;
 import static org.springframework.http.HttpStatus.UNPROCESSABLE_ENTITY;
 import static org.springframework.util.StringUtils.hasText;
@@ -30,6 +33,8 @@
 @RequestMapping("timeclock")
 class TimeClockController implements HasTimeClock, HasLaunchpad {
 
+    private static final Logger LOG = getLogger(lookup().lookupClass());
+
     private final TimeClockService timeClockService;
 
     TimeClockController(TimeClockService timeClockService) {
@@ -78,14 +83,14 @@ public String startTimeClock(@AuthenticationPrincipal DefaultOidcUser principal,
 
         final UserId userId = principalToUserId(principal);
 
-        // TODO should we do this in the service?
-        final Optional<TimeClock> maybeCurrentTimeClock = timeClockService.getCurrentTimeClock(userId);
-        if (maybeCurrentTimeClock.isPresent()) {
-            throw new ResponseStatusException(CONFLICT, "Time clock has been started already.");
+        try {
+            timeClockService.startTimeClock(userId);
+        } catch (TransactionSystemException e) {
+            // just log and redirect the user to the previous page
+            // since the time clock should be running already
+            LOG.warn("TimeClock could not be started. Maybe it is running already and user submitted the form twice with a double click.", e);
         }
 
-        timeClockService.startTimeClock(userId);
-
         return redirectToPreviousPage(request);
     }
 

src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockNotStartedException.java

@@ -2,7 +2,7 @@
 
 import de.focusshift.zeiterfassung.user.UserId;
 
-class TimeClockNotStartedException extends Exception {
+public class TimeClockNotStartedException extends Exception {
 
     TimeClockNotStartedException(UserId userId) {
         super("time clock for userId=%s is not running.".formatted(userId));

src/main/java/de/focusshift/zeiterfassung/timeclock/TimeClockService.java

@@ -3,16 +3,25 @@
 import de.focusshift.zeiterfassung.timeentry.TimeEntryService;
 import de.focusshift.zeiterfassung.user.UserId;
 import de.focusshift.zeiterfassung.user.UserSettingsProvider;
+import org.slf4j.Logger;
 import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Isolation;
+import org.springframework.transaction.annotation.Propagation;
+import org.springframework.transaction.annotation.Transactional;
 
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.util.List;
 import java.util.Optional;
 
+import static java.lang.invoke.MethodHandles.lookup;
+import static org.slf4j.LoggerFactory.getLogger;
+
 @Service
 public class TimeClockService {
 
+    private static final Logger LOG = getLogger(lookup().lookupClass());
+
     private final TimeClockRepository timeClockRepository;
     private final TimeEntryService timeEntryService;
     private final UserSettingsProvider userSettingsProvider;
@@ -27,10 +36,27 @@ Optional<TimeClock> getCurrentTimeClock(UserId userId) {
         return timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value()).map(TimeClockService::toTimeClock);
     }
 
-    void startTimeClock(UserId userId) {
+    /**
+     * Starts the {@linkplain TimeClock} for the given user. Does nothing when one is running already.
+     *
+     * @param userId user to start the {@linkplain TimeClock}
+     *
+     * @throws org.springframework.transaction.TransactionSystemException
+     *      when there are concurrent hits and the TimeClock cannot be written anymore
+     */
+    @Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE)
+    public void startTimeClock(UserId userId) {
+
+        final boolean present = timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value()).isPresent();
+        if (present) {
+            LOG.info("Not starting TimeClock for user {} since already started", userId.value());
+            return;
+        }
+
         final ZonedDateTime now = ZonedDateTime.now(userSettingsProvider.zoneId());
         final TimeClock timeClock = new TimeClock(userId, now);
 
+        LOG.info("Starting TimeClock for user {}", userId.value());
         timeClockRepository.save(toEntity(timeClock));
     }
 
@@ -60,30 +86,54 @@ public List<TimeClock> findAllTimeClocks(UserId userId) {
             .toList();
     }
 
-    TimeClock updateTimeClock(UserId userId, TimeClockUpdate timeClockUpdate) throws TimeClockNotStartedException {
+    /**
+     * Updates the current {@linkplain TimeClock}.
+     *
+     * @param userId user to update the {@linkplain TimeClock}
+     * @param timeClockUpdate new timeClock info
+     *
+     * @return the updated {@linkplain TimeClock}
+     *
+     * @throws TimeClockNotStartedException
+     *              when there is no running timeClock
+     * @throws org.springframework.transaction.TransactionSystemException
+     *              when there are concurrent hits and the TimeClock cannot be written anymore
+     */
+    @Transactional(propagation = Propagation.REQUIRED, isolation = Isolation.SERIALIZABLE)
+    public TimeClock updateTimeClock(UserId userId, TimeClockUpdate timeClockUpdate) throws TimeClockNotStartedException {
 
         final TimeClock timeClock = getCurrentTimeClock(userId)
             .map(existingTimeClock -> prepareTimeClockUpdate(existingTimeClock, timeClockUpdate))
             .orElseThrow(() -> new TimeClockNotStartedException(userId));
 
         final TimeClockEntity timeClockEntity = toEntity(timeClock);
 
+        LOG.info("Updating TimeClock for user {}", userId.value());
+        LOG.debug("Next TimeClock: {}", timeClock);
+
         return toTimeClock(timeClockRepository.save(timeClockEntity));
     }
 
+    /**
+     * When there is a running {@linkplain TimeClock} it will be stopped, otherwise nothing is done.
+     *
+     * @param userId id of the user which timeClock will be stopped
+     */
     void stopTimeClock(UserId userId) {
         timeClockRepository.findByOwnerAndStoppedAtIsNull(userId.value())
             .map(entity -> timeClockEntityWithStoppedAt(entity, ZonedDateTime.now(userSettingsProvider.zoneId())))
             .map(timeClockRepository::save)
             .map(TimeClockService::toTimeClock)
-            .ifPresent(timeClock -> {
-
-                final ZonedDateTime start = timeClock.startedAt();
-                final ZonedDateTime end = timeClock.stoppedAt()
-                    .orElseThrow(() -> new IllegalStateException("expected stoppedAt to contain a value."));
-
-                timeEntryService.createTimeEntry(userId, timeClock.comment(), start, end, timeClock.isBreak());
-            });
+            .ifPresentOrElse(
+                timeClock -> {
+                    final ZonedDateTime start = timeClock.startedAt();
+                    final ZonedDateTime end = timeClock.stoppedAt()
+                        .orElseThrow(() -> new IllegalStateException("expected stoppedAt to contain a value."));
+
+                    LOG.info("Stopping TimeClock={} for user={} and creating TimeEntry for it.", timeClock.id(), userId.value());
+                    timeEntryService.createTimeEntry(userId, timeClock.comment(), start, end, timeClock.isBreak());
+                },
+                () -> LOG.info("Not stopping TimeClock for user {} since nothing found. No TimeEntry created.", userId.value()));
     }
 
     private static TimeClockEntity toEntity(TimeClock timeClock) {

src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockControllerTest.java

@@ -10,6 +10,7 @@
 import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver;
 import org.springframework.test.web.servlet.ResultActions;
 import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
+import org.springframework.transaction.TransactionSystemException;
 
 import java.time.Instant;
 import java.time.LocalDate;
@@ -25,6 +26,7 @@
 import static org.hamcrest.core.AllOf.allOf;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
@@ -63,25 +65,24 @@ void ensureStartTimeClock() throws Exception {
             .andExpect(status().is3xxRedirection())
             .andExpect(redirectedUrl("referer-url"));
 
-        verify(timeClockService).getCurrentTimeClock(new UserId("batman"));
         verify(timeClockService).startTimeClock(new UserId("batman"));
         verifyNoMoreInteractions(timeClockService);
     }
 
     @Test
     void ensureStartTimeClockThrowsWhenClockIsRunningAlready() throws Exception {
 
-        final ZonedDateTime startedAt = ZonedDateTime.of(2023, 1, 11, 13, 37, 0, 0, ZONE_EUROPE_BERLIN);
-        final TimeClock timeClock = new TimeClock(1L, new UserId("batman"), startedAt, "awesome comment", false, Optional.empty());
-        when(timeClockService.getCurrentTimeClock(new UserId("batman"))).thenReturn(Optional.of(timeClock));
+        doThrow(new TransactionSystemException(""))
+            .when(timeClockService).startTimeClock(new UserId("batman"));
 
         perform(
             post("/timeclock/start")
                 .with(oidcLogin().userInfoToken(builder -> builder.subject("batman")))
+                .header("Referer", "referer-url")
         )
-            .andExpect(status().isConflict());
+            .andExpect(status().is3xxRedirection())
+            .andExpect(redirectedUrl("referer-url"));
 
-        verify(timeClockService).getCurrentTimeClock(new UserId("batman"));
         verifyNoMoreInteractions(timeClockService);
     }
 

src/test/java/de/focusshift/zeiterfassung/timeclock/TimeClockServiceTest.java

@@ -100,6 +100,17 @@ void ensureStartTimeClockPersistsNewEntity() {
         assertThat(persistedTimeClockEntity.getStoppedAt()).isNull();
     }
 
+    @Test
+    void ensureStartTimeClockDoesNothingWhenThereIsAlreadyOne() {
+
+        when(timeClockRepository.findByOwnerAndStoppedAtIsNull("batman")).thenReturn(Optional.of(new TimeClockEntity()));
+
+        sut.startTimeClock(new UserId("batman"));
+
+        verifyNoMoreInteractions(timeClockRepository);
+        verifyNoInteractions(userSettingsProvider);
+    }
+
     @Test
     void ensureStopTimeClockDoesNothingWhenThereIsNothingRunningCurrently() {