From db40020a48d809f3ffad73473092d9cf92ea5a57 Mon Sep 17 00:00:00 2001 From: Jan Cortiel Date: Thu, 12 Dec 2024 11:52:00 +0100 Subject: [PATCH 1/3] #350: Refactor timestamp validation and interval handling. Renamed methods and variables for better clarity and consistency across classes. Adjusted logic to streamline processing, including closing streams explicitly to avoid resource leaks. Updated response messages for improved user feedback. --- .../more/data/controller/ExternalDataApiV1Controller.java | 4 ++-- .../io/redlink/more/data/model/scheduler/Interval.java | 4 ++-- .../io/redlink/more/data/repository/StudyRepository.java | 7 ++++--- .../java/io/redlink/more/data/service/ExternalService.java | 5 +++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/main/java/io/redlink/more/data/controller/ExternalDataApiV1Controller.java b/src/main/java/io/redlink/more/data/controller/ExternalDataApiV1Controller.java index 9d3733e..8c1cfa1 100644 --- a/src/main/java/io/redlink/more/data/controller/ExternalDataApiV1Controller.java +++ b/src/main/java/io/redlink/more/data/controller/ExternalDataApiV1Controller.java @@ -66,7 +66,7 @@ public ResponseEntity storeExternalBulk(String moreApiToken, EndpointDat ApiRoutingInfo apiRoutingInfo = externalService.getRoutingInfo(moreApiToken); Integer participantId = Integer.valueOf(endpointDataBulkDTO.getParticipantId()); - externalService.allTimestampsInBulkAreValid(apiRoutingInfo.studyId(), apiRoutingInfo.observationId(), participantId, endpointDataBulkDTO); + externalService.assertTimestampsInBulk(apiRoutingInfo.studyId(), apiRoutingInfo.observationId(), participantId, endpointDataBulkDTO); final RoutingInfo routingInfo = externalService.validateAndCreateRoutingInfo(apiRoutingInfo, participantId); LoggingUtils.createContext(routingInfo); @@ -81,7 +81,7 @@ public ResponseEntity storeExternalBulk(String moreApiToken, EndpointDat numberOfDiscardedIds, routingInfo.studyId(), routingInfo.participantId()); return ResponseEntity.status(HttpStatus.CONFLICT).body("Study or participant is not active"); } - return ResponseEntity.accepted().body("Data has been successfully processed."); + return ResponseEntity.accepted().body("%d data-points have been successfully processed".formatted(endpointDataBulkDTO.getDataPoints().size())); } catch (AccessDeniedException e) { return ResponseEntity.status(HttpStatus.FORBIDDEN).body("Invalid token!"); } catch (NumberFormatException e) { diff --git a/src/main/java/io/redlink/more/data/model/scheduler/Interval.java b/src/main/java/io/redlink/more/data/model/scheduler/Interval.java index dc48bd1..9792fad 100644 --- a/src/main/java/io/redlink/more/data/model/scheduler/Interval.java +++ b/src/main/java/io/redlink/more/data/model/scheduler/Interval.java @@ -32,8 +32,8 @@ public Instant getEnd() { return end; } - public boolean containsTimestamp(Instant timestamp) { - return !timestamp.isBefore(start) && !timestamp.isAfter(end); + public boolean contains(Instant instant) { + return !instant.isBefore(start) && !instant.isAfter(end); } diff --git a/src/main/java/io/redlink/more/data/repository/StudyRepository.java b/src/main/java/io/redlink/more/data/repository/StudyRepository.java index e804ae4..2ec8f0c 100644 --- a/src/main/java/io/redlink/more/data/repository/StudyRepository.java +++ b/src/main/java/io/redlink/more/data/repository/StudyRepository.java @@ -431,8 +431,7 @@ private static MapSqlParameterSource toParameterSource(long studyId, int partici private static MapSqlParameterSource toParameterSource(long studyId, int participantId, ParticipantConsent.ObservationConsent consent) { return toParameterSource(studyId, participantId) - .addValue("observation_id", consent.observationId()) - ; + .addValue("observation_id", consent.observationId()); } @@ -445,9 +444,11 @@ public List getIntervals(Long studyId, Integer participantId, Relative }, studyId, participantId )) { - return stream + var intervalList = stream .flatMap(List::stream) .collect(Collectors.toList()); + stream.close(); + return intervalList; } catch (Exception e) { throw new BadRequestException("Failed to retrieve intervals for studyId: " + studyId + " and participantId: " + participantId); } diff --git a/src/main/java/io/redlink/more/data/service/ExternalService.java b/src/main/java/io/redlink/more/data/service/ExternalService.java index 59e150f..71b04ba 100644 --- a/src/main/java/io/redlink/more/data/service/ExternalService.java +++ b/src/main/java/io/redlink/more/data/service/ExternalService.java @@ -83,7 +83,7 @@ public RoutingInfo validateAndCreateRoutingInfo(ApiRoutingInfo apiRoutingInfo, I } @Cacheable(CachingConfiguration.OBSERVATION_ENDINGS) - public void allTimestampsInBulkAreValid(Long studyId, Integer observationId, Integer participantId, EndpointDataBulkDTO dataBulkDTO) { + public void assertTimestampsInBulk(Long studyId, Integer observationId, Integer participantId, EndpointDataBulkDTO dataBulkDTO) { Stream scheduleEvents = Optional.ofNullable(repository.getObservationSchedule(studyId, observationId)) .orElseThrow(() -> BadRequestException.NotFound(studyId, observationId)); @@ -98,6 +98,7 @@ public void allTimestampsInBulkAreValid(Long studyId, Integer observationId, Int } }) .toList(); + scheduleEvents.close(); if (intervalList.isEmpty()) { throw BadRequestException.NotFound(studyId, observationId); @@ -106,7 +107,7 @@ public void allTimestampsInBulkAreValid(Long studyId, Integer observationId, Int boolean allValid = dataBulkDTO.getDataPoints().stream() .map(ExternalDataDTO::getTimestamp) .allMatch(timestamp -> intervalList.stream() - .anyMatch(interval -> interval.containsTimestamp(timestamp)) + .anyMatch(interval -> interval.contains(timestamp)) ); if (!allValid) { throw TimeFrameException.InvalidDataPointInterval(dataBulkDTO.getParticipantId(), intervalList); From b55ba0d9cbbfa8f1e9b612dc2511164e5cda33e2 Mon Sep 17 00:00:00 2001 From: Jan Cortiel Date: Thu, 12 Dec 2024 14:21:11 +0100 Subject: [PATCH 2/3] #350: Refactor schedule handling and streamline interval processing Replaced stream-based processing with object-based handling for schedules to improve readability and maintainability. Removed unused imports and methods, simplifying the codebase. Added error handling for unsupported event types and encapsulated relative event scheduling logic into a helper method. --- .../more/data/repository/StudyRepository.java | 29 +++----- .../more/data/service/ExternalService.java | 66 ++++++++++--------- 2 files changed, 44 insertions(+), 51 deletions(-) diff --git a/src/main/java/io/redlink/more/data/repository/StudyRepository.java b/src/main/java/io/redlink/more/data/repository/StudyRepository.java index 2ec8f0c..e05efe5 100644 --- a/src/main/java/io/redlink/more/data/repository/StudyRepository.java +++ b/src/main/java/io/redlink/more/data/repository/StudyRepository.java @@ -5,8 +5,6 @@ import io.redlink.more.data.exception.BadRequestException; import io.redlink.more.data.model.*; -import io.redlink.more.data.model.scheduler.Interval; -import io.redlink.more.data.model.scheduler.RelativeEvent; import io.redlink.more.data.model.scheduler.ScheduleEvent; import io.redlink.more.data.schedule.SchedulerUtils; import org.apache.commons.lang3.tuple.Pair; @@ -27,8 +25,6 @@ import java.util.Optional; import java.util.OptionalInt; import java.util.function.Supplier; -import java.util.stream.Collectors; -import java.util.stream.Stream; import static io.redlink.more.data.repository.DbUtils.toInstant; import static io.redlink.more.data.repository.DbUtils.toLocalDate; @@ -169,8 +165,8 @@ public Optional getApiRoutingInfo(Long studyId, Integer observat } } - public Stream getObservationSchedule(Long studyId, Integer observationId) { - return jdbcTemplate.queryForStream( + public ScheduleEvent getObservationSchedule(Long studyId, Integer observationId) { + return jdbcTemplate.queryForObject( GET_OBSERVATION_SCHEDULE, getObservationScheduleRowMapper(), studyId, observationId @@ -435,20 +431,13 @@ private static MapSqlParameterSource toParameterSource(long studyId, int partici } - public List getIntervals(Long studyId, Integer participantId, RelativeEvent event) { - try (var stream = jdbcTemplate.queryForStream( - GET_PARTICIPANT_INFO_AND_START_DURATION_END_FOR_STUDY_AND_PARTICIPANT, - (rs, rowNum) -> { - Instant start = rs.getTimestamp("start").toInstant(); - return Interval.fromRanges(SchedulerUtils.parseToObservationSchedulesForRelativeEvent(event, start)); - }, - studyId, participantId - )) { - var intervalList = stream - .flatMap(List::stream) - .collect(Collectors.toList()); - stream.close(); - return intervalList; + public Instant getStudyStartFor(Long studyId, Integer participantId) { + try { + return jdbcTemplate.queryForObject( + GET_PARTICIPANT_INFO_AND_START_DURATION_END_FOR_STUDY_AND_PARTICIPANT, + (rs, rowNum) -> rs.getTimestamp("start").toInstant(), + studyId, participantId + ); } catch (Exception e) { throw new BadRequestException("Failed to retrieve intervals for studyId: " + studyId + " and participantId: " + participantId); } diff --git a/src/main/java/io/redlink/more/data/service/ExternalService.java b/src/main/java/io/redlink/more/data/service/ExternalService.java index 71b04ba..78f8cac 100644 --- a/src/main/java/io/redlink/more/data/service/ExternalService.java +++ b/src/main/java/io/redlink/more/data/service/ExternalService.java @@ -22,16 +22,14 @@ import io.redlink.more.data.model.scheduler.RelativeEvent; import io.redlink.more.data.model.scheduler.ScheduleEvent; import io.redlink.more.data.repository.StudyRepository; +import io.redlink.more.data.schedule.SchedulerUtils; import org.springframework.cache.annotation.Cacheable; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; -import java.util.Base64; -import java.util.List; -import java.util.Optional; -import java.util.OptionalInt; -import java.util.stream.Stream; +import java.time.Instant; +import java.util.*; @Service public class ExternalService { @@ -84,37 +82,43 @@ public RoutingInfo validateAndCreateRoutingInfo(ApiRoutingInfo apiRoutingInfo, I @Cacheable(CachingConfiguration.OBSERVATION_ENDINGS) public void assertTimestampsInBulk(Long studyId, Integer observationId, Integer participantId, EndpointDataBulkDTO dataBulkDTO) { - Stream scheduleEvents = Optional.ofNullable(repository.getObservationSchedule(studyId, observationId)) - .orElseThrow(() -> BadRequestException.NotFound(studyId, observationId)); - - List intervalList = scheduleEvents - .flatMap(scheduleEvent -> { - if (scheduleEvent instanceof Event) { - return Stream.of(Interval.from((Event) scheduleEvent)); - } else if (scheduleEvent instanceof RelativeEvent) { - return repository.getIntervals(studyId, participantId, (RelativeEvent) scheduleEvent).stream(); - } else { - throw new BadRequestException("Unsupported ScheduleEvent type: " + scheduleEvent.getClass()); - } - }) - .toList(); - scheduleEvents.close(); - - if (intervalList.isEmpty()) { - throw BadRequestException.NotFound(studyId, observationId); - } + try { + ScheduleEvent scheduleEvent = repository.getObservationSchedule(studyId, observationId); + + List intervalList = new ArrayList<>(); + if (scheduleEvent instanceof Event) { + intervalList.add(Interval.from((Event) scheduleEvent)); + } else if (scheduleEvent instanceof RelativeEvent) { + Instant studyStart = repository.getStudyStartFor(studyId, participantId); + intervalList.addAll(createSchedulesFromRelativeEvent((RelativeEvent) scheduleEvent, studyStart)); + } else { + throw new BadRequestException("Unsupported ScheduleEvent type: " + scheduleEvent.getClass()); + } + + if (intervalList.isEmpty()) { + throw BadRequestException.NotFound(studyId, observationId); + } - boolean allValid = dataBulkDTO.getDataPoints().stream() - .map(ExternalDataDTO::getTimestamp) - .allMatch(timestamp -> intervalList.stream() - .anyMatch(interval -> interval.contains(timestamp)) - ); - if (!allValid) { - throw TimeFrameException.InvalidDataPointInterval(dataBulkDTO.getParticipantId(), intervalList); + boolean allValid = dataBulkDTO.getDataPoints().stream() + .map(ExternalDataDTO::getTimestamp) + .allMatch(timestamp -> intervalList.stream() + .anyMatch(interval -> interval.contains(timestamp)) + ); + if (!allValid) { + throw TimeFrameException.InvalidDataPointInterval(dataBulkDTO.getParticipantId(), intervalList); + } + } catch (BadRequestException e) { + throw e; + } catch (Exception e) { + throw BadRequestException.NotFound(studyId, observationId); } } public List listParticipants(Long studyId, OptionalInt studyGroupId) { return repository.listParticipants(studyId, studyGroupId); } + + private List createSchedulesFromRelativeEvent(RelativeEvent event, Instant start) { + return Interval.fromRanges(SchedulerUtils.parseToObservationSchedulesForRelativeEvent(event, start)); + } } From 3d9f45ccea7e3328a80171643ea6bc960fa3cf93 Mon Sep 17 00:00:00 2001 From: Jan Cortiel Date: Mon, 16 Dec 2024 08:50:36 +0100 Subject: [PATCH 3/3] #350: Refactor repository methods to return Optionals. Updated `getObservationSchedule` and `getStudyStartFor` methods to return `Optional` instead of throwing exceptions. Adjusted related logic in `ExternalService` to handle the absence of results more gracefully. --- .../more/data/repository/StudyRepository.java | 28 +++++++++++-------- .../more/data/service/ExternalService.java | 6 ++-- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/main/java/io/redlink/more/data/repository/StudyRepository.java b/src/main/java/io/redlink/more/data/repository/StudyRepository.java index e05efe5..49425c1 100644 --- a/src/main/java/io/redlink/more/data/repository/StudyRepository.java +++ b/src/main/java/io/redlink/more/data/repository/StudyRepository.java @@ -3,11 +3,11 @@ */ package io.redlink.more.data.repository; -import io.redlink.more.data.exception.BadRequestException; import io.redlink.more.data.model.*; import io.redlink.more.data.model.scheduler.ScheduleEvent; import io.redlink.more.data.schedule.SchedulerUtils; import org.apache.commons.lang3.tuple.Pair; +import org.springframework.dao.DataAccessException; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.RowMapper; @@ -165,12 +165,16 @@ public Optional getApiRoutingInfo(Long studyId, Integer observat } } - public ScheduleEvent getObservationSchedule(Long studyId, Integer observationId) { - return jdbcTemplate.queryForObject( - GET_OBSERVATION_SCHEDULE, - getObservationScheduleRowMapper(), - studyId, observationId - ); + public Optional getObservationSchedule(Long studyId, Integer observationId) { + try { + return Optional.ofNullable(jdbcTemplate.queryForObject( + GET_OBSERVATION_SCHEDULE, + getObservationScheduleRowMapper(), + studyId, observationId + )); + } catch (EmptyResultDataAccessException e) { + return Optional.empty(); + } } public Optional findByRegistrationToken(String registrationToken) { @@ -431,15 +435,15 @@ private static MapSqlParameterSource toParameterSource(long studyId, int partici } - public Instant getStudyStartFor(Long studyId, Integer participantId) { + public Optional getStudyStartFor(Long studyId, Integer participantId) { try { - return jdbcTemplate.queryForObject( + return Optional.ofNullable(jdbcTemplate.queryForObject( GET_PARTICIPANT_INFO_AND_START_DURATION_END_FOR_STUDY_AND_PARTICIPANT, (rs, rowNum) -> rs.getTimestamp("start").toInstant(), studyId, participantId - ); - } catch (Exception e) { - throw new BadRequestException("Failed to retrieve intervals for studyId: " + studyId + " and participantId: " + participantId); + )); + } catch (DataAccessException e) { + return Optional.empty(); } } diff --git a/src/main/java/io/redlink/more/data/service/ExternalService.java b/src/main/java/io/redlink/more/data/service/ExternalService.java index 78f8cac..5cb2d83 100644 --- a/src/main/java/io/redlink/more/data/service/ExternalService.java +++ b/src/main/java/io/redlink/more/data/service/ExternalService.java @@ -83,14 +83,14 @@ public RoutingInfo validateAndCreateRoutingInfo(ApiRoutingInfo apiRoutingInfo, I @Cacheable(CachingConfiguration.OBSERVATION_ENDINGS) public void assertTimestampsInBulk(Long studyId, Integer observationId, Integer participantId, EndpointDataBulkDTO dataBulkDTO) { try { - ScheduleEvent scheduleEvent = repository.getObservationSchedule(studyId, observationId); + ScheduleEvent scheduleEvent = repository.getObservationSchedule(studyId, observationId).orElseThrow(() -> BadRequestException.NotFound(studyId, observationId)); List intervalList = new ArrayList<>(); if (scheduleEvent instanceof Event) { intervalList.add(Interval.from((Event) scheduleEvent)); } else if (scheduleEvent instanceof RelativeEvent) { - Instant studyStart = repository.getStudyStartFor(studyId, participantId); - intervalList.addAll(createSchedulesFromRelativeEvent((RelativeEvent) scheduleEvent, studyStart)); + Optional studyStart = repository.getStudyStartFor(studyId, participantId); + studyStart.ifPresent(instant -> intervalList.addAll(createSchedulesFromRelativeEvent((RelativeEvent) scheduleEvent, instant))); } else { throw new BadRequestException("Unsupported ScheduleEvent type: " + scheduleEvent.getClass()); }