From e22d69a9c5c0ad9e543fb31f39eeed2a697320ba Mon Sep 17 00:00:00 2001 From: Tobias Kampmann Date: Mon, 5 Feb 2024 17:01:09 +0100 Subject: [PATCH] fix(backend): address PR --- .../loculus/backend/api/SubmissionTypes.kt | 6 +- .../SubmissionControllerDescriptions.kt | 8 +-- .../submission/SubmissionDatabaseService.kt | 11 ++-- .../submission/GetSequencesEndpointTest.kt | 55 ++++++++++++++++--- .../submission/SubmissionControllerClient.kt | 13 ++--- .../submission/SubmissionConvenienceClient.kt | 4 +- 6 files changed, 63 insertions(+), 34 deletions(-) diff --git a/backend/src/main/kotlin/org/loculus/backend/api/SubmissionTypes.kt b/backend/src/main/kotlin/org/loculus/backend/api/SubmissionTypes.kt index 5acc4dc524..66ba84c92a 100644 --- a/backend/src/main/kotlin/org/loculus/backend/api/SubmissionTypes.kt +++ b/backend/src/main/kotlin/org/loculus/backend/api/SubmissionTypes.kt @@ -151,7 +151,7 @@ data class SequenceEntryStatus( override val accession: Accession, override val version: Version, val status: Status, - val group: Group, + val group: String, val isRevocation: Boolean = false, ) : AccessionVersionInterface @@ -202,10 +202,6 @@ enum class Status { return stringToEnumMap[statusString] ?: throw IllegalArgumentException("Unknown status: $statusString") } - - fun getListOfStatuses(): List { - return stringToEnumMap.values.toList() - } } } diff --git a/backend/src/main/kotlin/org/loculus/backend/controller/SubmissionControllerDescriptions.kt b/backend/src/main/kotlin/org/loculus/backend/controller/SubmissionControllerDescriptions.kt index 3505a0c085..7b293fc92e 100644 --- a/backend/src/main/kotlin/org/loculus/backend/controller/SubmissionControllerDescriptions.kt +++ b/backend/src/main/kotlin/org/loculus/backend/controller/SubmissionControllerDescriptions.kt @@ -69,10 +69,10 @@ The accession version must be in status 'HAS_ERRORS' or 'AWAITING_APPROVAL'. """ const val GET_SEQUENCES_DESCRIPTION = """ -Get a list of submitted accession versions and their status. -There are two optional parameter that filter for groups and statuses. -When no constraints are given the endpoint returns all sequences of all groups the user is member of. -If a group is filtered for that the user is not a member of the endpoint will return an error. +Retrieve a list of submitted accession versions along with their statuses. +There are two optional parameters that filter by groups and statuses. +When no constraints are applied, the endpoint returns all sequences from all groups the user is a member of. +If a filter is applied for a group the user is not a member of, the endpoint will return an error. """ const val APPROVE_PROCESSED_DATA_DESCRIPTION = """ diff --git a/backend/src/main/kotlin/org/loculus/backend/service/submission/SubmissionDatabaseService.kt b/backend/src/main/kotlin/org/loculus/backend/service/submission/SubmissionDatabaseService.kt index 4917583934..cee5bec8de 100644 --- a/backend/src/main/kotlin/org/loculus/backend/service/submission/SubmissionDatabaseService.kt +++ b/backend/src/main/kotlin/org/loculus/backend/service/submission/SubmissionDatabaseService.kt @@ -361,6 +361,8 @@ class SubmissionDatabaseService( groupsFilter: List?, statusesFilter: List?, ): List { + log.info { "getting sequence for user $username (groupFilter: $groupsFilter in statuses $statusesFilter" } + val validatedGroupNames = if (groupsFilter === null) { groupManagementDatabaseService.getGroupsOfUser(username) } else { @@ -368,12 +370,9 @@ class SubmissionDatabaseService( groupsFilter.map { Group(it) } } - val listOfStatuses = statusesFilter ?: Status.getListOfStatuses() - - log.info { "getting sequence for user $username from groups $validatedGroupNames in statuses $listOfStatuses" } + val listOfStatuses = statusesFilter ?: Status.entries sequenceEntriesTableProvider.get(organism).let { table -> - return table .slice( table.accessionColumn, @@ -394,7 +393,7 @@ class SubmissionDatabaseService( row[table.accessionColumn], row[table.versionColumn], Status.fromString(row[table.statusColumn]), - Group(row[table.groupNameColumn]), + row[table.groupNameColumn], row[table.isRevocationColumn], ) } @@ -463,7 +462,7 @@ class SubmissionDatabaseService( it[table.accessionColumn], it[table.versionColumn], AWAITING_APPROVAL_FOR_REVOCATION, - Group(it[table.groupNameColumn]), + it[table.groupNameColumn], it[table.isRevocationColumn], ) }.sortedBy { it.accession } diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/submission/GetSequencesEndpointTest.kt b/backend/src/test/kotlin/org/loculus/backend/controller/submission/GetSequencesEndpointTest.kt index a2867b1271..ecdb736261 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/submission/GetSequencesEndpointTest.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/submission/GetSequencesEndpointTest.kt @@ -11,6 +11,8 @@ import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.MethodSource import org.loculus.backend.api.AccessionVersion import org.loculus.backend.api.Status +import org.loculus.backend.api.Status.AWAITING_APPROVAL +import org.loculus.backend.api.Status.IN_PROCESSING import org.loculus.backend.controller.ALTERNATIVE_DEFAULT_GROUP_NAME import org.loculus.backend.controller.ALTERNATIVE_DEFAULT_USER_NAME import org.loculus.backend.controller.DEFAULT_GROUP_NAME @@ -18,6 +20,7 @@ import org.loculus.backend.controller.DEFAULT_ORGANISM import org.loculus.backend.controller.EndpointTest import org.loculus.backend.controller.OTHER_ORGANISM import org.loculus.backend.controller.expectUnauthorizedResponse +import org.loculus.backend.controller.generateJwtFor import org.loculus.backend.controller.getAccessionVersions import org.loculus.backend.controller.submission.SubmitFiles.DefaultFiles.NUMBER_OF_SEQUENCES import org.loculus.backend.controller.submission.SubmitFiles.DefaultFiles.firstAccession @@ -39,29 +42,29 @@ class GetSequencesEndpointTest( @Test fun `GIVEN data submitted from a group member THEN another group member sees the data`() { convenienceClient.submitDefaultFiles(DEFAULT_USER_NAME) + val sequencesOfUser = convenienceClient.getSequenceEntries( username = DEFAULT_USER_NAME, - groupsFilter = listOf(DEFAULT_GROUP_NAME), - statusesFilter = listOf(Status.RECEIVED), - organism = DEFAULT_ORGANISM, ) + assertThat(sequencesOfUser, hasSize(NUMBER_OF_SEQUENCES)) + val sequencesOfAlternativeUser = convenienceClient.getSequenceEntries( username = ALTERNATIVE_DEFAULT_USER_NAME, - groupsFilter = listOf(DEFAULT_GROUP_NAME), - statusesFilter = listOf(Status.RECEIVED), - organism = DEFAULT_ORGANISM, ) + assertThat(sequencesOfAlternativeUser, hasSize(NUMBER_OF_SEQUENCES)) } @Test fun `GIVEN data submitted to a group WHEN querying another group THEN only shows entries of the given group`() { convenienceClient.submitDefaultFiles(DEFAULT_USER_NAME, DEFAULT_GROUP_NAME) + val sequencesOfUser = convenienceClient.getSequenceEntries( username = DEFAULT_USER_NAME, groupsFilter = listOf(ALTERNATIVE_DEFAULT_GROUP_NAME), ) + assertThat(sequencesOfUser, hasSize(0)) } @@ -87,6 +90,7 @@ class GetSequencesEndpointTest( username = DEFAULT_USER_NAME, organism = OTHER_ORGANISM, ) + assertThat( sequencesOfUser.getAccessionVersions(), containsInAnyOrder(*otherOrganismData.getAccessionVersions().toTypedArray()), @@ -97,6 +101,41 @@ class GetSequencesEndpointTest( ) } + @Test + fun `WHEN querying sequences of an existing group that you're not a member of THEN this is forbidden`() { + client.getSequenceEntries( + groupsFilter = listOf(ALTERNATIVE_DEFAULT_GROUP_NAME), + jwt = generateJwtFor(ALTERNATIVE_DEFAULT_USER_NAME), + ).andExpect(status().isForbidden).andExpect { + jsonPath( + "$.detail", + containsString( + "User $ALTERNATIVE_DEFAULT_USER_NAME is not " + + "a member of groups $ALTERNATIVE_DEFAULT_GROUP_NAME.", + ), + ) + } + } + + @Test + fun `GIVEN data in many statuses WHEN querying sequences for a certain one THEN return only those sequences`() { + convenienceClient.prepareDataTo(AWAITING_APPROVAL) + + val sequencesInAwaitingApproval = convenienceClient.getSequenceEntries( + username = ALTERNATIVE_DEFAULT_USER_NAME, + statusesFilter = listOf(AWAITING_APPROVAL), + ) + + assertThat(sequencesInAwaitingApproval, hasSize(10)) + + val sequencesInProcessing = convenienceClient.getSequenceEntries( + username = ALTERNATIVE_DEFAULT_USER_NAME, + statusesFilter = listOf(IN_PROCESSING), + ) + + assertThat(sequencesInProcessing, hasSize(0)) + } + @ParameterizedTest(name = "{arguments}") @MethodSource("provideStatusScenarios") fun `GIVEN database in prepared state THEN returns sequence entries in expected status`(scenario: Scenario) { @@ -122,7 +161,7 @@ class GetSequencesEndpointTest( Scenario( setupDescription = "I started processing sequence entries", prepareDatabase = { it.prepareDefaultSequenceEntriesToInProcessing() }, - expectedStatus = Status.IN_PROCESSING, + expectedStatus = IN_PROCESSING, expectedIsRevocation = false, ), Scenario( @@ -138,7 +177,7 @@ class GetSequencesEndpointTest( PreparedProcessedData.successfullyProcessed(), ) }, - expectedStatus = Status.AWAITING_APPROVAL, + expectedStatus = AWAITING_APPROVAL, expectedIsRevocation = false, ), Scenario( diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionControllerClient.kt b/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionControllerClient.kt index 24a4fa3efd..13d5f8faf0 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionControllerClient.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionControllerClient.kt @@ -1,7 +1,6 @@ package org.loculus.backend.controller.submission import com.fasterxml.jackson.databind.ObjectMapper -import mu.KotlinLogging import org.loculus.backend.api.AccessionVersion import org.loculus.backend.api.DataUseTerms import org.loculus.backend.api.Status @@ -25,7 +24,6 @@ import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.multi import org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post const val DEFAULT_USER_NAME = "testuser" -private val logger = KotlinLogging.logger {} class SubmissionControllerClient(private val mockMvc: MockMvc, private val objectMapper: ObjectMapper) { fun submit( metadataFile: MockMultipartFile, @@ -83,18 +81,15 @@ class SubmissionControllerClient(private val mockMvc: MockMvc, private val objec fun getSequenceEntries( organism: String = DEFAULT_ORGANISM, - groupsFilter: List = listOf(DEFAULT_GROUP_NAME), - statusesFilter: List = Status.getListOfStatuses(), + groupsFilter: List? = null, + statusesFilter: List? = null, jwt: String? = jwtForDefaultUser, ): ResultActions { - logger.info { - "Getting sequences for organism $organism, groupNames $groupsFilter, statuses $statusesFilter" - } return mockMvc.perform( get(addOrganismToPath("/get-sequences", organism = organism)) .withAuth(jwt) - .param("groupsFilter", groupsFilter.joinToString { it }) - .param("statusesFilter", statusesFilter.joinToString { it.name }), + .param("groupsFilter", groupsFilter?.joinToString { it }) + .param("statusesFilter", statusesFilter?.joinToString { it.name }), ) } diff --git a/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionConvenienceClient.kt b/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionConvenienceClient.kt index 7614b6a6ea..ff49434066 100644 --- a/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionConvenienceClient.kt +++ b/backend/src/test/kotlin/org/loculus/backend/controller/submission/SubmissionConvenienceClient.kt @@ -161,8 +161,8 @@ class SubmissionConvenienceClient( fun getSequenceEntries( username: String = DEFAULT_USER_NAME, - groupsFilter: List = listOf(DEFAULT_GROUP_NAME), - statusesFilter: List = Status.getListOfStatuses(), + groupsFilter: List? = null, + statusesFilter: List? = null, organism: String = DEFAULT_ORGANISM, ): List = deserializeJsonResponse( client.getSequenceEntries(