diff --git a/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandler.java b/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandler.java index 8b9835a5..1d78be80 100644 --- a/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandler.java +++ b/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandler.java @@ -28,13 +28,13 @@ public class DataqueryHandler { @NonNull private DataqueryRepository dataqueryRepository; - @Value("${app.maxSavedQueriesPerUser}") - private int maxDataqueriesPerUser; + @NonNull + private Integer maxDataqueriesPerUser; public Long storeDataquery(@NonNull Dataquery dataquery, @NonNull String userId) throws DataqueryException, DataqueryStorageFullException { - var usedSlots = dataqueryRepository.countByCreatedByWhereResultIsNotNull(userId); - if (usedSlots >= maxDataqueriesPerUser) { + // By definition, a user can save an unlimited amount of queries without result + if (dataquery.resultSize() != null && dataqueryRepository.countByCreatedByWhereResultIsNotNull(userId) >= maxDataqueriesPerUser) { throw new DataqueryStorageFullException(); } diff --git a/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataquerySpringConfig.java b/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataquerySpringConfig.java index 38e2f3c7..a3834f72 100644 --- a/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataquerySpringConfig.java +++ b/src/main/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataquerySpringConfig.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import de.numcodex.feasibility_gui_backend.query.persistence.DataqueryRepository; import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -12,8 +13,9 @@ public class DataquerySpringConfig { @Bean public DataqueryHandler createDataqueryHandler( @Qualifier("translation") ObjectMapper jsonUtil, - DataqueryRepository dataqueryRepository + DataqueryRepository dataqueryRepository, + @Value("${app.maxSavedQueriesPerUser}") Integer maxSavedQueriesPerUser ) { - return new DataqueryHandler(jsonUtil, dataqueryRepository); + return new DataqueryHandler(jsonUtil, dataqueryRepository, maxSavedQueriesPerUser); } } diff --git a/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/CodeableConceptRestController.java b/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/CodeableConceptRestController.java index 08933891..8c092b69 100644 --- a/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/CodeableConceptRestController.java +++ b/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/CodeableConceptRestController.java @@ -11,7 +11,7 @@ import java.util.List; @RestController -@RequestMapping("api/v4/codeable-concept") +@RequestMapping("api/v5/codeable-concept") @ConditionalOnExpression("${app.elastic.enabled}") @CrossOrigin public class CodeableConceptRestController { diff --git a/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/TerminologyRestController.java b/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/TerminologyRestController.java index f4ebc1b1..9db17dfa 100644 --- a/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/TerminologyRestController.java +++ b/src/main/java/de/numcodex/feasibility_gui_backend/terminology/v5/TerminologyRestController.java @@ -18,7 +18,7 @@ */ -@RequestMapping("api/v4/terminology") +@RequestMapping("api/v5/terminology") @RestController @CrossOrigin @ConditionalOnExpression("${app.elastic.enabled}") diff --git a/src/test/java/de/numcodex/feasibility_gui_backend/query/QueryHandlerServiceTest.java b/src/test/java/de/numcodex/feasibility_gui_backend/query/QueryHandlerServiceTest.java index 78288b9e..79df09b2 100644 --- a/src/test/java/de/numcodex/feasibility_gui_backend/query/QueryHandlerServiceTest.java +++ b/src/test/java/de/numcodex/feasibility_gui_backend/query/QueryHandlerServiceTest.java @@ -96,8 +96,8 @@ public void testRunQuery_failsWithMonoErrorOnQueryDispatchException() throws Que } @ParameterizedTest - @CsvSource({"true,true", "true,false", "false,true", "false,false"}) - void convertQueriesToQueryListEntries(String withSavedQuery, String skipValidation) throws JsonProcessingException { + @CsvSource({"false,true", "false,false"}) + void convertQueriesToQueryListEntries(String skipValidation) throws JsonProcessingException { var queryList = List.of(createQuery()); if (!Boolean.parseBoolean(skipValidation)) { doReturn( @@ -110,10 +110,6 @@ void convertQueriesToQueryListEntries(String withSavedQuery, String skipValidati assertThat(queryListEntries.size()).isEqualTo(1); assertThat(queryListEntries.get(0).id()).isEqualTo(QUERY_ID); assertThat(queryListEntries.get(0).createdAt()).isEqualTo(LAST_MODIFIED); - if (Boolean.parseBoolean(withSavedQuery)) { - assertThat(queryListEntries.get(0).label()).isEqualTo(LABEL); - assertThat(queryListEntries.get(0).totalNumberOfPatients()).isEqualTo(RESULT_SIZE); - } if (Boolean.parseBoolean(skipValidation)) { assertThat(queryListEntries.get(0).isValid()).isNull(); } else { diff --git a/src/test/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandlerTest.java b/src/test/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandlerTest.java index 77e14a88..788210e7 100644 --- a/src/test/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandlerTest.java +++ b/src/test/java/de/numcodex/feasibility_gui_backend/query/dataquery/DataqueryHandlerTest.java @@ -12,6 +12,8 @@ import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.mockito.Mock; import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; @@ -25,8 +27,7 @@ import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.*; @Tag("query") @Tag("peristence") @@ -37,6 +38,7 @@ class DataqueryHandlerTest { public static final String LABEL = "some-label"; public static final String COMMENT = "some-comment"; public static final String TIME_STRING = "1969-07-20 20:17:40.0"; + private static final int MAX_QUERIES_PER_USER = 5; @Spy private ObjectMapper jsonUtil = new ObjectMapper(); @@ -45,7 +47,7 @@ class DataqueryHandlerTest { private DataqueryRepository dataqueryRepository; private DataqueryHandler createDataqueryHandler() { - return new DataqueryHandler(jsonUtil, dataqueryRepository); + return new DataqueryHandler(jsonUtil, dataqueryRepository, MAX_QUERIES_PER_USER); } @Test @@ -78,9 +80,43 @@ void storeDataquery_throwsOnNonEmptyQueryAndEmptyUser() { assertThrows(NullPointerException.class, () -> dataqueryHandler.storeDataquery(createDataquery(), null)); } + @ParameterizedTest + @CsvSource({"true", "false"}) + @DisplayName("storeDataquery() -> trying to store a dataquery when no slots are free throws") + void storeDataquery_throwsOnNoFreeSlots(boolean withResult) throws JsonProcessingException { + var dataqueryHandler = createDataqueryHandler(); + lenient().doReturn(MAX_QUERIES_PER_USER + 1L).when(dataqueryRepository).countByCreatedByWhereResultIsNotNull(any(String.class)); + lenient().doReturn(createDataqueryEntity()).when(dataqueryRepository).save(any(de.numcodex.feasibility_gui_backend.query.persistence.Dataquery.class)); + + if (withResult) { + assertThrows(DataqueryStorageFullException.class, () -> dataqueryHandler.storeDataquery(createDataquery(withResult), CREATOR)); + } else { + assertDoesNotThrow(() -> dataqueryHandler.storeDataquery(createDataquery(withResult), CREATOR)); + } + } + + @ParameterizedTest + @CsvSource({"true,-1", "true,0", "true,1", "false,-1", "false,0", "false,1"}) + @DisplayName("storeDataquery() -> checking around the query limit") + void storeDataquery_testFreeSlotOnEdgeCases(boolean withResult, long offset) throws JsonProcessingException { + var dataqueryHandler = createDataqueryHandler(); + lenient().doReturn(MAX_QUERIES_PER_USER + offset).when(dataqueryRepository).countByCreatedByWhereResultIsNotNull(any(String.class)); + lenient().doReturn(createDataqueryEntity()).when(dataqueryRepository).save(any(de.numcodex.feasibility_gui_backend.query.persistence.Dataquery.class)); + + if (withResult) { + if (offset < 0) { + assertDoesNotThrow(() -> dataqueryHandler.storeDataquery(createDataquery(withResult), CREATOR)); + } else { + assertThrows(DataqueryStorageFullException.class, () -> dataqueryHandler.storeDataquery(createDataquery(withResult), CREATOR)); + } + } else { + assertDoesNotThrow(() -> dataqueryHandler.storeDataquery(createDataquery(withResult), CREATOR)); + } + } + @Test @DisplayName("storeDataquery() -> error in json serialization throws an exception") - void storeNewQuery_throwsOnJsonSerializationError() throws JsonProcessingException { + void storeDataquery_throwsOnJsonSerializationError() throws JsonProcessingException { var dataquery = createDataquery(); doThrow(JsonProcessingException.class).when(jsonUtil).writeValueAsString(any(Crtdl.class)); @@ -275,18 +311,22 @@ void convertPersistenceToApi() throws JsonProcessingException { assertEquals(convertedDataquery.content().cohortDefinition().exclusionCriteria(), originalCrtdl.cohortDefinition().exclusionCriteria()); } - private Dataquery createDataquery() { + private Dataquery createDataquery(boolean withResult) { return Dataquery.builder() .id(1L) .label(LABEL) .comment(COMMENT) .content(createCrtdl()) - .resultSize(123L) + .resultSize(withResult ? 123L : null) .createdBy(CREATOR) .lastModified(TIME_STRING) .build(); } + private Dataquery createDataquery() { + return createDataquery(false); + } + private Crtdl createCrtdl() { return Crtdl.builder() .cohortDefinition(createValidStructuredQuery()) diff --git a/src/test/java/de/numcodex/feasibility_gui_backend/query/ratelimiting/RateLimitingInterceptorIT.java b/src/test/java/de/numcodex/feasibility_gui_backend/query/ratelimiting/RateLimitingInterceptorIT.java index 4b62726f..578cd841 100644 --- a/src/test/java/de/numcodex/feasibility_gui_backend/query/ratelimiting/RateLimitingInterceptorIT.java +++ b/src/test/java/de/numcodex/feasibility_gui_backend/query/ratelimiting/RateLimitingInterceptorIT.java @@ -1,6 +1,6 @@ package de.numcodex.feasibility_gui_backend.query.ratelimiting; -import static de.numcodex.feasibility_gui_backend.config.WebSecurityConfig.PATH_API; +import static de.numcodex.feasibility_gui_backend.config.WebSecurityConfig.*; import static de.numcodex.feasibility_gui_backend.query.persistence.ResultType.SUCCESS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -89,7 +89,7 @@ void setupMockBehaviour() throws InvalidAuthenticationException { @EnumSource public void testGetResult_SucceedsOnFirstCall(ResultDetail resultDetail) throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1"; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1"; boolean isAdmin = false; switch (resultDetail) { @@ -118,7 +118,7 @@ public void testGetResult_SucceedsOnFirstCall(ResultDetail resultDetail) throws @EnumSource public void testGetResult_FailsOnImmediateSecondCall(ResultDetail resultDetail) throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1"; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1"; switch (resultDetail) { case SUMMARY -> requestUri = requestUri + WebSecurityConfig.PATH_SUMMARY_RESULT; @@ -152,7 +152,7 @@ public void testGetResult_FailsOnImmediateSecondCall(ResultDetail resultDetail) @EnumSource public void testGetResult_SucceedsOnDelayedSecondCall(ResultDetail resultDetail) throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1"; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1"; switch (resultDetail) { case SUMMARY -> requestUri = requestUri + WebSecurityConfig.PATH_SUMMARY_RESULT; @@ -178,7 +178,7 @@ public void testGetResult_SucceedsOnDelayedSecondCall(ResultDetail resultDetail) mockMvc .perform( - get(URI.create(PATH_API + "/query/1" + WebSecurityConfig.PATH_SUMMARY_RESULT)).with(csrf()) + get(URI.create(PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1" + WebSecurityConfig.PATH_SUMMARY_RESULT)).with(csrf()) .with(user(authorName).password("pass").roles("DATAPORTAL_TEST_USER")) ) .andExpect(status().isOk()); @@ -190,7 +190,7 @@ public void testGetResult_SucceedsOnImmediateMultipleCallsAsAdmin(ResultDetail r throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1"; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1"; switch (resultDetail) { case SUMMARY -> requestUri = requestUri + WebSecurityConfig.PATH_SUMMARY_RESULT; @@ -217,7 +217,7 @@ public void testGetResult_SucceedsOnImmediateMultipleCallsAsAdmin(ResultDetail r public void testGetResult_SucceedsOnImmediateSecondCallAsOtherUser(ResultDetail resultDetail) throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1"; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1"; switch (resultDetail) { case SUMMARY -> requestUri = requestUri + WebSecurityConfig.PATH_SUMMARY_RESULT; @@ -244,7 +244,7 @@ public void testGetResult_SucceedsOnImmediateSecondCallAsOtherUser(ResultDetail mockMvc .perform( - get(URI.create(PATH_API + "/query/1" + WebSecurityConfig.PATH_SUMMARY_RESULT)).with(csrf()) + get(URI.create(PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1" + WebSecurityConfig.PATH_SUMMARY_RESULT)).with(csrf()) .with(user(authorName).password("pass").roles("DATAPORTAL_TEST_USER")) ) .andExpect(status().isOk()); @@ -253,7 +253,7 @@ public void testGetResult_SucceedsOnImmediateSecondCallAsOtherUser(ResultDetail @Test public void testGetDetailedObfuscatedResult_FailsOnLimitExceedingCall() throws Exception { var authorName = UUID.randomUUID().toString(); - var requestUri = PATH_API + "/query/1" + WebSecurityConfig.PATH_DETAILED_OBFUSCATED_RESULT; + var requestUri = PATH_API + PATH_QUERY + PATH_FEASIBILITY + "/1" + WebSecurityConfig.PATH_DETAILED_OBFUSCATED_RESULT; doReturn(false).when(authenticationHelper) .hasAuthority(any(Authentication.class), eq("DATAPORTAL_TEST_ADMIN")); diff --git a/src/test/resources/application.yml b/src/test/resources/application.yml index a2fd418f..096a8da6 100644 --- a/src/test/resources/application.yml +++ b/src/test/resources/application.yml @@ -14,7 +14,7 @@ app: keycloakAllowedRole: "DATAPORTAL_TEST_USER" keycloakPowerRole: "DATAPORTAL_TEST_POWER" keycloakAdminRole: "DATAPORTAL_TEST_ADMIN" - maxSavedQueriesPerUser: 2 + maxSavedQueriesPerUser: 10 broker: aktin: enabled: false