diff --git a/src/main/java/bio/terra/app/controller/DatasetsApiController.java b/src/main/java/bio/terra/app/controller/DatasetsApiController.java index 91a892013f..2892b55866 100644 --- a/src/main/java/bio/terra/app/controller/DatasetsApiController.java +++ b/src/main/java/bio/terra/app/controller/DatasetsApiController.java @@ -34,6 +34,8 @@ import bio.terra.model.PolicyMemberRequest; import bio.terra.model.PolicyModel; import bio.terra.model.PolicyResponse; +import bio.terra.model.QueryColumnStatisticsRequestModel; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SamPolicyModel; import bio.terra.model.SnapshotBuilderAccessRequest; import bio.terra.model.SnapshotBuilderGetConceptsResponse; @@ -80,7 +82,6 @@ @Controller @Api(tags = {"datasets"}) public class DatasetsApiController implements DatasetsApi { - public static final String RETRIEVE_INCLUDE_DEFAULT_VALUE = "SCHEMA,PROFILE,DATA_PROJECT,STORAGE"; private final HttpServletRequest request; @@ -195,6 +196,27 @@ public ResponseEntity getConcepts( return ResponseEntity.ok(snapshotBuilderService.getConceptChildren(id, conceptId)); } + @Override + public ResponseEntity queryDatasetDataById( + UUID id, String table, QueryDataRequestModel queryDataRequest) { + AuthenticatedUserRequest userReq = getAuthenticatedInfo(); + verifyDatasetAuthorization(userReq, id.toString(), IamAction.READ_DATA); + // TODO: Remove after https://broadworkbench.atlassian.net/browse/DR-2588 is fixed + SqlSortDirection sortDirection = + Objects.requireNonNullElse(queryDataRequest.getDirection(), SqlSortDirection.ASC); + DatasetDataModel previewModel = + datasetService.retrieveData( + userReq, + id, + table, + queryDataRequest.getLimit(), + queryDataRequest.getOffset(), + queryDataRequest.getSort(), + sortDirection, + queryDataRequest.getFilter()); + return ResponseEntity.ok(previewModel); + } + @Override public ResponseEntity lookupDatasetDataById( UUID id, @@ -204,22 +226,31 @@ public ResponseEntity lookupDatasetDataById( String sort, SqlSortDirection direction, String filter) { - AuthenticatedUserRequest userReq = getAuthenticatedInfo(); - verifyDatasetAuthorization(userReq, id.toString(), IamAction.READ_DATA); - // TODO: Remove after https://broadworkbench.atlassian.net/browse/DR-2588 is fixed - SqlSortDirection sortDirection = Objects.requireNonNullElse(direction, SqlSortDirection.ASC); - DatasetDataModel previewModel = - datasetService.retrieveData(userReq, id, table, limit, offset, sort, sortDirection, filter); - return ResponseEntity.ok(previewModel); + var request = + new QueryDataRequestModel() + .offset(offset) + .limit(limit) + .sort(sort) + .direction(direction) + .filter(filter); + return queryDatasetDataById(id, table, request); } @Override public ResponseEntity lookupDatasetColumnStatisticsById( UUID id, String table, String column, String filter) { + return queryDatasetColumnStatisticsById( + id, table, column, new QueryColumnStatisticsRequestModel().filter(filter)); + } + + @Override + public ResponseEntity queryDatasetColumnStatisticsById( + UUID id, String table, String column, QueryColumnStatisticsRequestModel requestModel) { AuthenticatedUserRequest userReq = getAuthenticatedInfo(); verifyDatasetAuthorization(userReq, id.toString(), IamAction.READ_DATA); ColumnStatisticsModel columnStatisticsModel = - datasetService.retrieveColumnStatistics(userReq, id, table, column, filter); + datasetService.retrieveColumnStatistics( + userReq, id, table, column, requestModel.getFilter()); return ResponseEntity.ok(columnStatisticsModel); } diff --git a/src/main/java/bio/terra/app/controller/SnapshotsApiController.java b/src/main/java/bio/terra/app/controller/SnapshotsApiController.java index f5b7d0ffb5..232962deb7 100644 --- a/src/main/java/bio/terra/app/controller/SnapshotsApiController.java +++ b/src/main/java/bio/terra/app/controller/SnapshotsApiController.java @@ -16,6 +16,7 @@ import bio.terra.model.PolicyMemberRequest; import bio.terra.model.PolicyModel; import bio.terra.model.PolicyResponse; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SnapshotIdsAndRolesModel; import bio.terra.model.SnapshotLinkDuosDatasetResponse; import bio.terra.model.SnapshotModel; @@ -265,6 +266,26 @@ public ResponseEntity lookupSnapshotFileByPath( return ResponseEntity.ok(fileModel); } + @Override + public ResponseEntity querySnapshotDataById( + UUID id, String table, QueryDataRequestModel queryDataRequest) { + snapshotService.verifySnapshotReadable(id, getAuthenticatedInfo()); + // TODO: Remove after https://broadworkbench.atlassian.net/browse/DR-2588 is fixed + SqlSortDirection sortDirection = + Objects.requireNonNullElse(queryDataRequest.getDirection(), SqlSortDirection.ASC); + SnapshotPreviewModel previewModel = + snapshotService.retrievePreview( + getAuthenticatedInfo(), + id, + table, + queryDataRequest.getLimit(), + queryDataRequest.getOffset(), + queryDataRequest.getSort(), + sortDirection, + queryDataRequest.getFilter()); + return ResponseEntity.ok(previewModel); + } + @Override public ResponseEntity lookupSnapshotPreviewById( UUID id, @@ -274,15 +295,15 @@ public ResponseEntity lookupSnapshotPreviewById( String sort, SqlSortDirection direction, String filter) { - logger.debug("Verifying user access"); - snapshotService.verifySnapshotReadable(id, getAuthenticatedInfo()); - logger.debug("Retrieving snapshot id {}", id); - // TODO: Remove after https://broadworkbench.atlassian.net/browse/DR-2588 is fixed - SqlSortDirection sortDirection = Objects.requireNonNullElse(direction, SqlSortDirection.ASC); - SnapshotPreviewModel previewModel = - snapshotService.retrievePreview( - getAuthenticatedInfo(), id, table, limit, offset, sort, sortDirection, filter); - return ResponseEntity.ok(previewModel); + return querySnapshotDataById( + id, + table, + new QueryDataRequestModel() + .offset(offset) + .limit(limit) + .sort(sort) + .direction(direction) + .filter(filter)); } // --snapshot auth domains -- diff --git a/src/main/resources/api/data-repository-openapi.yaml b/src/main/resources/api/data-repository-openapi.yaml index 7a889b9f6f..b430c98ce9 100644 --- a/src/main/resources/api/data-repository-openapi.yaml +++ b/src/main/resources/api/data-repository-openapi.yaml @@ -826,7 +826,8 @@ paths: - snapshots - search - repository - description: Retrieve data for a table in a snapshot + deprecated: true + description: Retrieve data for a table in a snapshot. This endpoint is deprecated, please use the POST version. operationId: lookupSnapshotPreviewById parameters: - $ref: '#/components/parameters/Id' @@ -875,6 +876,34 @@ paths: application/json: schema: $ref: '#/components/schemas/SnapshotPreviewModel' + post: + tags: + - snapshots + - search + - repository + description: Retrieve data for a table in a snapshot. + operationId: querySnapshotDataById + parameters: + - $ref: '#/components/parameters/Id' + - name: table + in: path + description: Name of table to get data from + required: true + schema: + type: string + requestBody: + description: Parameters to filter results + content: + application/json: + schema: + $ref: '#/components/schemas/QueryDataRequestModel' + responses: + 200: + description: Returns the table data from a snapshot + content: + application/json: + schema: + $ref: '#/components/schemas/SnapshotPreviewModel' /api/repository/v1/snapshots/{id}/export: get: tags: @@ -1898,7 +1927,8 @@ paths: - datasets - search - repository - description: Retrieve data for a table in a dataset + deprecated: true + description: Retrieve data for a table in a dataset. This endpoint is deprecated, please use the POST version. operationId: lookupDatasetDataById parameters: - $ref: '#/components/parameters/Id' @@ -1947,11 +1977,42 @@ paths: application/json: schema: $ref: '#/components/schemas/DatasetDataModel' + post: + tags: + - datasets + - search + - repository + description: Retrieve data for a table in a dataset. + operationId: queryDatasetDataById + parameters: + - $ref: '#/components/parameters/Id' + - name: table + in: path + description: Name of table to get data from + required: true + schema: + type: string + requestBody: + description: Parameters to filter results + content: + application/json: + schema: + $ref: '#/components/schemas/QueryDataRequestModel' + required: false + responses: + 200: + description: Returns the table data from a dataset + content: + application/json: + schema: + $ref: '#/components/schemas/DatasetDataModel' /api/repository/v1/datasets/{id}/data/{table}/statistics/{column}: get: tags: - datasets - description: Retrieve statiscs about data for a column in a table in a dataset + deprecated: true + description: >- + Retrieve statistics about data for a column in a table in a dataset. This endpoint is deprecated, please use the POST version. operationId: lookupDatasetColumnStatisticsById parameters: - $ref: '#/components/parameters/Id' @@ -1963,7 +2024,7 @@ paths: type: string - name: column in: path - description: Name of column in the table to get statisitcs about + description: Name of column in the table to get statistics about required: true schema: type: string @@ -1976,7 +2037,8 @@ paths: For GCP array string columns, if you wanted to include all rows that contain 'value1' in column1, the filter clause would look like 'WHERE 'value1' IN UNNEST(column1)'. Note that "count" value includes all occurrences of a value including duplicates of the same value in a single array. - i.e. if we had two rows in a table where the value for column1, row1 = ['value1', 'value1', 'value2'] and column1, row2 = ['value1'] the count for 'value1' would be 3. + i.e. if we had two rows in a table where the value for column1, row1 = ['value1', 'value1', 'value2'] + and column1, row2 = ['value1'] the count for 'value1' would be 3. schema: type: string responses: @@ -1986,6 +2048,38 @@ paths: application/json: schema: $ref: '#/components/schemas/ColumnStatisticsModel' + post: + tags: + - datasets + description: Retrieve statistics about data for a column in a table in a dataset. + operationId: queryDatasetColumnStatisticsById + parameters: + - $ref: '#/components/parameters/Id' + - name: table + in: path + description: Name of table where column lives + required: true + schema: + type: string + - name: column + in: path + description: Name of column in the table to get statistics about + required: true + schema: + type: string + requestBody: + description: Parameters to filter results + content: + application/json: + schema: + $ref: '#/components/schemas/QueryColumnStatisticsRequestModel' + responses: + 200: + description: Returns the statistics about a column from a dataset's table + content: + application/json: + schema: + $ref: '#/components/schemas/ColumnStatisticsModel' /api/repository/v1/datasets/{id}/summary: get: tags: @@ -5160,6 +5254,46 @@ components: dataset bucket. Eventually, this will include attributes of the storage including billing, temperature, geography, etc. But for now... + QueryDataRequestModel: + type: object + properties: + offset: + description: The number of rows to skip when retrieving the next page + type: integer + default: 0 + minimum: 0 + limit: + description: The number of rows to return for the data + type: integer + default: 30 + minimum: 1 + maximum: 1000 + sort: + description: The table column to sort by + type: string + default: "datarepo_row_id" + direction: + $ref: '#/components/schemas/SqlSortDirection' + description: The direction to sort. + default: asc + filter: + description: A SQL WHERE clause to filter the table results. + type: string + default: "" + QueryColumnStatisticsRequestModel: + type: object + properties: + filter: + description: >- + A SQL WHERE clause to filter results included in column statistics. + + + For GCP array string columns, if you wanted to include all rows that contain 'value1' in column1, + the filter clause would look like 'WHERE 'value1' IN UNNEST(column1)'. Note that "count" value + includes all occurrences of a value including duplicates of the same value in a single array. + i.e. if we had two rows in a table where the value for column1, row1 = ['value1', 'value1', 'value2'] + and column1, row2 = ['value1'] the count for 'value1' would be 3. + type: string DatasetDataModel: type: object properties: diff --git a/src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java b/src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java index 2e5abbd905..750e8d8cfa 100644 --- a/src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java +++ b/src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java @@ -3,6 +3,7 @@ import static bio.terra.service.snapshotbuilder.SnapshotBuilderTestData.SETTINGS; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; @@ -17,9 +18,13 @@ import bio.terra.common.fixtures.AuthenticationFixtures; import bio.terra.common.iam.AuthenticatedUserRequest; import bio.terra.common.iam.AuthenticatedUserRequestFactory; +import bio.terra.model.ColumnStatisticsTextModel; +import bio.terra.model.ColumnStatisticsTextValue; import bio.terra.model.DatasetDataModel; import bio.terra.model.DatasetModel; import bio.terra.model.DatasetRequestAccessIncludeModel; +import bio.terra.model.QueryColumnStatisticsRequestModel; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SnapshotBuilderAccessRequest; import bio.terra.model.SnapshotBuilderCohort; import bio.terra.model.SnapshotBuilderConcept; @@ -49,11 +54,13 @@ import bio.terra.service.snapshotbuilder.SnapshotBuilderService; import java.util.List; import java.util.UUID; +import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; @@ -61,6 +68,7 @@ import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; @ActiveProfiles({"google", "unittest"}) @ContextConfiguration(classes = {DatasetsApiController.class, GlobalExceptionHandler.class}) @@ -88,7 +96,10 @@ public class DatasetsApiControllerTest { RETRIEVE_DATASET_ENDPOINT + "/createSnapshotRequest"; private static final DatasetRequestAccessIncludeModel INCLUDE = DatasetRequestAccessIncludeModel.NONE; - private static final String GET_PREVIEW_ENDPOINT = RETRIEVE_DATASET_ENDPOINT + "/data/{table}"; + private static final String QUERY_DATA_ENDPOINT = RETRIEVE_DATASET_ENDPOINT + "/data/{table}"; + + private static final String QUERY_COLUMN_STATISTICS_ENDPOINT = + QUERY_DATA_ENDPOINT + "/statistics/{column}"; private static final String GET_SNAPSHOT_BUILDER_SETTINGS_ENDPOINT = RETRIEVE_DATASET_ENDPOINT + "/snapshotBuilder/settings"; private static final String GET_CONCEPTS_ENDPOINT = @@ -99,6 +110,7 @@ public class DatasetsApiControllerTest { private static final int LIMIT = 10; private static final int OFFSET = 0; private static final String FILTER = null; + private static final String TABLE_NAME = "good_table"; @BeforeEach void setUp() { @@ -153,78 +165,109 @@ void testRetrieveDatasetForbidden() throws Exception { } @ParameterizedTest - @ValueSource(strings = {"good_column", "datarepo_row_id"}) - void testDatasetViewDataById(String column) throws Exception { - var table = "good_table"; + @MethodSource + void testQueryDatasetDataById(String column, MockHttpServletRequestBuilder request) + throws Exception { when(datasetService.retrieveData( - TEST_USER, DATASET_ID, table, LIMIT, OFFSET, column, DIRECTION, FILTER)) + TEST_USER, DATASET_ID, TABLE_NAME, LIMIT, OFFSET, column, DIRECTION, FILTER)) .thenReturn(new DatasetDataModel().addResultItem("hello").addResultItem("world")); + mockValidators(); + mvc.perform(request).andExpect(status().isOk()).andExpect(jsonPath("$.result").isArray()); + verifyAuthorizationCall(IamAction.READ_DATA); - mvc.perform( - get(GET_PREVIEW_ENDPOINT, DATASET_ID, table) - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", column) - .queryParam("direction", DIRECTION.name())) - .andExpect(status().isOk()) - .andExpect(jsonPath("$.result").isArray()); + verify(datasetService) + .retrieveData(TEST_USER, DATASET_ID, TABLE_NAME, LIMIT, OFFSET, column, DIRECTION, FILTER); + } + + private static Stream testQueryDatasetDataById() { + return Stream.of( + arguments("goodColumn", postRequset(TABLE_NAME, "goodColumn")), + arguments("datarepo_row_id", getRequest(TABLE_NAME, "datarepo_row_id"))); + } + + private static Stream testQueryDatasetColumnStatistics() { + return Stream.of( + arguments( + post(QUERY_COLUMN_STATISTICS_ENDPOINT, DATASET_ID, "good_table", "good_column") + .contentType(MediaType.APPLICATION_JSON) + .content( + TestUtils.mapToJson(new QueryColumnStatisticsRequestModel().filter(FILTER)))), + arguments( + get(QUERY_COLUMN_STATISTICS_ENDPOINT, DATASET_ID, "good_table", "good_column") + .queryParam("filter", FILTER))); + } + + @ParameterizedTest + @MethodSource + void testQueryDatasetColumnStatistics(MockHttpServletRequestBuilder request) throws Exception { + var table = "good_table"; + var column = "good_column"; + var expected = + new ColumnStatisticsTextModel() + .values(List.of(new ColumnStatisticsTextValue().value("hello").count(2))); + + when(datasetService.retrieveColumnStatistics(TEST_USER, DATASET_ID, table, column, FILTER)) + .thenReturn(expected); + mockValidators(); + + String result = mvc.perform(request).andReturn().getResponse().getContentAsString(); + ColumnStatisticsTextModel actual = + TestUtils.mapFromJson(result, ColumnStatisticsTextModel.class); + assertThat("Correct ColumnStatisticsTextModel is returned", actual, equalTo(expected)); verifyAuthorizationCall(IamAction.READ_DATA); - verify(datasetService) - .retrieveData(TEST_USER, DATASET_ID, table, LIMIT, OFFSET, column, DIRECTION, FILTER); + verify(datasetService).retrieveColumnStatistics(TEST_USER, DATASET_ID, table, column, FILTER); } - @Test - void testDatasetViewDataNotFound() throws Exception { + private static Stream provideRequests() { + return Stream.of( + arguments(postRequset(TABLE_NAME, "goodColumn")), + arguments(getRequest(TABLE_NAME, "goodColumn"))); + } + + @ParameterizedTest + @MethodSource("provideRequests") + void testQueryDatasetDataNotFound(MockHttpServletRequestBuilder request) throws Exception { mockNotFound(); - mvc.perform( - get(GET_PREVIEW_ENDPOINT, DATASET_ID, "table") - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", "column") - .queryParam("direction", DIRECTION.name())) - .andExpect(status().isNotFound()); + mockValidators(); + mvc.perform(request).andExpect(status().isNotFound()); verifyNoInteractions(iamService); } - @Test - void testDatasetViewDataForbidden() throws Exception { + @ParameterizedTest + @MethodSource("provideRequests") + void testQueryDatasetDataForbidden(MockHttpServletRequestBuilder request) throws Exception { IamAction iamAction = IamAction.READ_DATA; mockForbidden(iamAction); - - mvc.perform( - get(GET_PREVIEW_ENDPOINT, DATASET_ID, "table") - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", "column") - .queryParam("direction", DIRECTION.name())) - .andExpect(status().isForbidden()); + mockValidators(); + mvc.perform(request).andExpect(status().isForbidden()); verifyAuthorizationCall(iamAction); } - @Test - void testDatasetViewDataRetrievalFails() throws Exception { + @ParameterizedTest + @MethodSource + void testQueryDatasetDataRetrievalFails(MockHttpServletRequestBuilder request) throws Exception { var table = "bad_table"; var column = "good_column"; when(datasetService.retrieveData( TEST_USER, DATASET_ID, table, LIMIT, OFFSET, column, DIRECTION, FILTER)) .thenThrow(DatasetDataException.class); - - mvc.perform( - get(GET_PREVIEW_ENDPOINT, DATASET_ID, table) - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", column) - .queryParam("direction", DIRECTION.name())) - .andExpect(status().is5xxServerError()); + mockValidators(); + mvc.perform(request).andExpect(status().is5xxServerError()); verifyAuthorizationCall(IamAction.READ_DATA); verify(datasetService) .retrieveData(TEST_USER, DATASET_ID, table, LIMIT, OFFSET, column, DIRECTION, FILTER); } + private static Stream testQueryDatasetDataRetrievalFails() { + return Stream.of( + arguments(postRequset("bad_table", "good_column")), + arguments(getRequest("bad_table", "good_column"))); + } + @Test void testUpdateSnapshotBuilderSettings() throws Exception { when(datasetService.retrieveDatasetModel( @@ -374,4 +417,26 @@ private void mockValidators() { when(dataDeletionRequestValidator.supports(any())).thenReturn(true); when(datasetSchemaUpdateValidator.supports(any())).thenReturn(true); } + + private static MockHttpServletRequestBuilder postRequset(String tableName, String columnName) { + return post(QUERY_DATA_ENDPOINT, DATASET_ID, tableName) + .contentType(MediaType.APPLICATION_JSON) + .content( + TestUtils.mapToJson( + new QueryDataRequestModel() + .direction(DIRECTION) + .limit(LIMIT) + .offset(OFFSET) + .sort(columnName) + .filter(FILTER))); + } + + private static MockHttpServletRequestBuilder getRequest(String tableName, String columnName) { + return get(QUERY_DATA_ENDPOINT, DATASET_ID, tableName) + .queryParam("limit", String.valueOf(LIMIT)) + .queryParam("offset", String.valueOf(OFFSET)) + .queryParam("sort", columnName) + .queryParam("direction", DIRECTION.name()) + .queryParam("filter", FILTER); + } } diff --git a/src/test/java/bio/terra/app/controller/SearchApiControllerTest.java b/src/test/java/bio/terra/app/controller/SearchApiControllerTest.java index c24803fc4b..a5767639f7 100644 --- a/src/test/java/bio/terra/app/controller/SearchApiControllerTest.java +++ b/src/test/java/bio/terra/app/controller/SearchApiControllerTest.java @@ -4,7 +4,6 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -12,6 +11,7 @@ import bio.terra.common.TestUtils; import bio.terra.common.category.Unit; import bio.terra.common.iam.AuthenticatedUserRequest; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SearchIndexRequest; import bio.terra.model.SnapshotPreviewModel; import bio.terra.model.SqlSortDirection; @@ -35,6 +35,7 @@ import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.ResultActions; @RunWith(SpringRunner.class) @SpringBootTest( @@ -56,7 +57,7 @@ public class SearchApiControllerTest { private static final SqlSortDirection DIRECTION = SqlSortDirection.ASC; private static final int LIMIT = 10; private static final int OFFSET = 0; - private static final String FILTER = null; + private static final String FILTER = ""; @Autowired private MockMvc mvc; @@ -73,12 +74,7 @@ private void mockSnapshotPreviewByIdSuccess(UUID id, String table, String column when(snapshotService.retrievePreview( any(), eq(id), eq(table), eq(LIMIT), eq(OFFSET), eq(column), eq(DIRECTION), eq(FILTER))) .thenReturn(result); - mvc.perform( - get(GET_PREVIEW_ENDPOINT, id, table) - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", column) - .queryParam("direction", String.valueOf(DIRECTION))) + performQueryDataPost(id, table, column) .andExpect(status().isOk()) .andExpect(jsonPath("$.result").isArray()); } @@ -87,13 +83,7 @@ private void mockSnapshotPreviewByIdError(UUID id, String table, String column) when(snapshotService.retrievePreview( any(), eq(id), eq(table), eq(LIMIT), eq(OFFSET), eq(column), eq(DIRECTION), eq(FILTER))) .thenThrow(SnapshotPreviewException.class); - mvc.perform( - get(GET_PREVIEW_ENDPOINT, id, table) - .queryParam("limit", String.valueOf(LIMIT)) - .queryParam("offset", String.valueOf(OFFSET)) - .queryParam("sort", column) - .queryParam("direction", String.valueOf(DIRECTION))) - .andExpect(status().is5xxServerError()); + performQueryDataPost(id, table, column).andExpect(status().is5xxServerError()); } @Test @@ -160,4 +150,18 @@ public void testCreateSearchIndex() throws Exception { any(), eq(IamResourceType.DATASNAPSHOT), eq(id.toString()), eq(IamAction.READ_DATA)); verify(searchService).indexSnapshot(eq(snapshot), eq(searchIndexRequest)); } + + private ResultActions performQueryDataPost(UUID id, String table, String column) + throws Exception { + return mvc.perform( + post(GET_PREVIEW_ENDPOINT, id, table) + .contentType(MediaType.APPLICATION_JSON) + .content( + TestUtils.mapToJson( + new QueryDataRequestModel() + .direction(DIRECTION) + .limit(LIMIT) + .offset(OFFSET) + .sort(column)))); + } } diff --git a/src/test/java/bio/terra/app/controller/SnapshotsApiControllerTest.java b/src/test/java/bio/terra/app/controller/SnapshotsApiControllerTest.java index 994c725102..eb4ac58a8c 100644 --- a/src/test/java/bio/terra/app/controller/SnapshotsApiControllerTest.java +++ b/src/test/java/bio/terra/app/controller/SnapshotsApiControllerTest.java @@ -1,13 +1,16 @@ package bio.terra.app.controller; +import static bio.terra.common.PdaoConstant.PDAO_ROW_ID_COLUMN; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import bio.terra.common.TestUtils; @@ -16,6 +19,9 @@ import bio.terra.common.iam.AuthenticatedUserRequest; import bio.terra.common.iam.AuthenticatedUserRequestFactory; import bio.terra.model.JobModel; +import bio.terra.model.QueryDataRequestModel; +import bio.terra.model.SnapshotPreviewModel; +import bio.terra.model.SqlSortDirection; import bio.terra.service.auth.iam.IamAction; import bio.terra.service.auth.iam.IamResourceType; import bio.terra.service.auth.iam.IamService; @@ -28,15 +34,21 @@ import bio.terra.service.snapshot.SnapshotService; import bio.terra.service.snapshot.exception.SnapshotNotFoundException; import java.util.UUID; +import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; @ActiveProfiles({"google", "unittest"}) @ContextConfiguration(classes = {SnapshotsApiController.class, GlobalExceptionHandler.class}) @@ -62,8 +74,17 @@ class SnapshotsApiControllerTest { private static final Boolean EXPORT_GCS_PATHS = false; private static final Boolean VALIDATE_PRIMARY_KEY_UNIQUENESS = true; private static final Boolean SIGN_URLS = true; + private static final String TABLE_NAME = "table1"; + private static final String COLUMN_NAME = PDAO_ROW_ID_COLUMN; + private static final int LIMIT = 100; + private static final int OFFSET = 0; + private static final SqlSortDirection DIRECTION = SqlSortDirection.ASC; + private static final String FILTER = ""; private static final String RETRIEVE_SNAPSHOT_ENDPOINT = "/api/repository/v1/snapshots/{id}"; + + private static final String QUERY_SNAPSHOT_DATA_ENDPOINT = + RETRIEVE_SNAPSHOT_ENDPOINT + "/data/{table}"; private static final String EXPORT_SNAPSHOT_ENDPOINT = RETRIEVE_SNAPSHOT_ENDPOINT + "/export"; @BeforeEach @@ -134,10 +155,73 @@ void testExportSnapshotForbidden() throws Exception { verifyAuthorizationCall(iamAction); } + private static Stream testQuerySnapshotData() { + return Stream.of( + arguments( + post(QUERY_SNAPSHOT_DATA_ENDPOINT, SNAPSHOT_ID, TABLE_NAME) + .contentType(MediaType.APPLICATION_JSON) + .content( + TestUtils.mapToJson( + new QueryDataRequestModel() + .direction(DIRECTION) + .limit(LIMIT) + .offset(OFFSET) + .sort(COLUMN_NAME) + .filter(FILTER))), + arguments( + get(QUERY_SNAPSHOT_DATA_ENDPOINT, SNAPSHOT_ID, TABLE_NAME) + .queryParam("direction", String.valueOf(DIRECTION)) + .queryParam("limit", String.valueOf(LIMIT)) + .queryParam("offset", String.valueOf(OFFSET)) + .queryParam("sort", COLUMN_NAME) + .queryParam("filter", FILTER)))); + } + + @ParameterizedTest + @MethodSource + void testQuerySnapshotData(MockHttpServletRequestBuilder request) throws Exception { + mockValidators(); + var expectedSnapshotPreview = + new SnapshotPreviewModel().addResultItem("row1").addResultItem("row2"); + when(snapshotService.retrievePreview( + TEST_USER, + SNAPSHOT_ID, + TABLE_NAME, + LIMIT, + OFFSET, + PDAO_ROW_ID_COLUMN, + DIRECTION, + FILTER)) + .thenReturn(expectedSnapshotPreview); + + String actualJson = mvc.perform(request).andReturn().getResponse().getContentAsString(); + SnapshotPreviewModel actual = TestUtils.mapFromJson(actualJson, SnapshotPreviewModel.class); + assertThat("Job model is returned", actual, equalTo(expectedSnapshotPreview)); + + // Actual auth check is verified in SnapshotServiceTest.verifySnapshotReadableBySam() + verify(snapshotService).verifySnapshotReadable(SNAPSHOT_ID, TEST_USER); + verify(snapshotService) + .retrievePreview( + TEST_USER, + SNAPSHOT_ID, + TABLE_NAME, + LIMIT, + OFFSET, + PDAO_ROW_ID_COLUMN, + DIRECTION, + FILTER); + } + /** Verify that snapshot authorization was checked. */ private void verifyAuthorizationCall(IamAction iamAction) { verify(iamService) .verifyAuthorization( TEST_USER, IamResourceType.DATASNAPSHOT, SNAPSHOT_ID.toString(), iamAction); } + + private void mockValidators() { + when(snapshotRequestValidator.supports(any())).thenReturn(true); + when(ingestRequestValidator.supports(any())).thenReturn(true); + when(assetModelValidator.supports(any())).thenReturn(true); + } } diff --git a/src/test/java/bio/terra/common/fixtures/ConnectedOperations.java b/src/test/java/bio/terra/common/fixtures/ConnectedOperations.java index ead05a6bba..c8ce573115 100644 --- a/src/test/java/bio/terra/common/fixtures/ConnectedOperations.java +++ b/src/test/java/bio/terra/common/fixtures/ConnectedOperations.java @@ -45,6 +45,7 @@ import bio.terra.model.IngestRequestModel; import bio.terra.model.IngestResponseModel; import bio.terra.model.JobModel; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SnapshotModel; import bio.terra.model.SnapshotPreviewModel; import bio.terra.model.SnapshotRequestModel; @@ -904,16 +905,17 @@ public MockHttpServletResponse retrieveDatasetDataByIdRaw( UUID datasetId, String tableName, int limit, int offset, String filter, String sort) throws Exception { String url = "/api/repository/v1/datasets/{id}/data/{table}"; - MockHttpServletRequestBuilder request = - get(url, datasetId, tableName) - .param("limit", String.valueOf(limit)) - .param("offset", String.valueOf(offset)); + var requestModel = new QueryDataRequestModel().limit(limit).offset(offset); if (sort != null) { - request.param("sort", sort); + requestModel.sort(sort); } if (filter != null) { - request.param("filter", filter); + requestModel.filter(filter); } + MockHttpServletRequestBuilder request = + post(url, datasetId, tableName) + .contentType(MediaType.APPLICATION_JSON) + .content(TestUtils.mapToJson(requestModel)); MvcResult result = mvc.perform(request).andReturn(); return result.getResponse(); } @@ -945,16 +947,17 @@ public MockHttpServletResponse retrieveSnapshotPreviewByIdRaw( UUID snapshotId, String tableName, int limit, int offset, String filter, String sort) throws Exception { String url = "/api/repository/v1/snapshots/{id}/data/{table}"; - MockHttpServletRequestBuilder request = - get(url, snapshotId, tableName) - .param("limit", String.valueOf(limit)) - .param("offset", String.valueOf(offset)); - if (filter != null) { - request.param("filter", filter); - } + var requestModel = new QueryDataRequestModel().limit(limit).offset(offset); if (sort != null) { - request.param("sort", sort); + requestModel.sort(sort); + } + if (filter != null) { + requestModel.filter(filter); } + MockHttpServletRequestBuilder request = + post(url, snapshotId, tableName) + .contentType(MediaType.APPLICATION_JSON) + .content(TestUtils.mapToJson(requestModel)); MvcResult result = mvc.perform(request).andReturn(); return result.getResponse(); } diff --git a/src/test/java/bio/terra/integration/DataRepoFixtures.java b/src/test/java/bio/terra/integration/DataRepoFixtures.java index 23a5e362d8..7ebac7e4c7 100644 --- a/src/test/java/bio/terra/integration/DataRepoFixtures.java +++ b/src/test/java/bio/terra/integration/DataRepoFixtures.java @@ -56,12 +56,15 @@ import bio.terra.model.JobModel; import bio.terra.model.PolicyMemberRequest; import bio.terra.model.PolicyResponse; +import bio.terra.model.QueryColumnStatisticsRequestModel; +import bio.terra.model.QueryDataRequestModel; import bio.terra.model.SnapshotExportResponseModel; import bio.terra.model.SnapshotModel; import bio.terra.model.SnapshotPreviewModel; import bio.terra.model.SnapshotRequestModel; import bio.terra.model.SnapshotRetrieveIncludeModel; import bio.terra.model.SnapshotSummaryModel; +import bio.terra.model.SqlSortDirection; import bio.terra.model.TransactionCloseModel; import bio.terra.model.TransactionCreateModel; import bio.terra.model.TransactionModel; @@ -910,18 +913,13 @@ private DataRepoResponse retrieveSnapshotPreviewByIdRaw( String sort) throws Exception { String url = "/api/repository/v1/snapshots/%s/data/%s".formatted(snapshotId, table); - - offset = Objects.requireNonNullElse(offset, 0); - limit = Objects.requireNonNullElse(limit, 10); - String queryParams = "?offset=%s&limit=%s".formatted(offset, limit); - - if (filter != null) { - queryParams += "&filter=%s".formatted(filter); - } - if (sort != null) { - queryParams += "&sort=%s".formatted(sort); - } - return dataRepoClient.get(user, url + queryParams, new TypeReference<>() {}); + QueryDataRequestModel requestModel = new QueryDataRequestModel(); + requestModel.offset(Objects.requireNonNullElse(offset, 0)); + requestModel.limit(Objects.requireNonNullElse(limit, 10)); + requestModel.filter(Objects.requireNonNullElse(filter, "")); + requestModel.sort(Objects.requireNonNullElse(sort, PDAO_ROW_ID_COLUMN)); + return dataRepoClient.post( + user, url, TestUtils.mapToJson(requestModel), new TypeReference<>() {}); } public List getRowIds( @@ -1053,20 +1051,14 @@ private DataRepoResponse retrieveDatasetDataByIdRaw( throws Exception { String url = "/api/repository/v1/datasets/%s/data/%s".formatted(datasetId, table); - offset = Objects.requireNonNullElse(offset, 0); - limit = Objects.requireNonNullElse(limit, 10); - String queryParams = "?offset=%s&limit=%s".formatted(offset, limit); - - if (filter != null) { - queryParams += "&filter=%s".formatted(filter); - } - if (sort != null) { - queryParams += "&sort=%s".formatted(sort); - } - if (direction != null) { - queryParams += "&direction=%s".formatted(direction); - } - return dataRepoClient.get(user, url + queryParams, new TypeReference<>() {}); + QueryDataRequestModel request = new QueryDataRequestModel(); + request.setOffset(Objects.requireNonNullElse(offset, 0)); + request.setLimit(Objects.requireNonNullElse(limit, 10)); + request.filter(Objects.requireNonNullElse(filter, "")); + request.sort(Objects.requireNonNullElse(sort, PDAO_ROW_ID_COLUMN)); + request.setDirection( + Objects.requireNonNullElse(SqlSortDirection.fromValue(direction), SqlSortDirection.ASC)); + return dataRepoClient.post(user, url, TestUtils.mapToJson(request), new TypeReference<>() {}); } public void assertColumnTextValueCount( @@ -1133,13 +1125,10 @@ private DataRepoResponse retrieveColumnStatsIntRaw( String url = "/api/repository/v1/datasets/%s/data/%s/statistics/%s" .formatted(datasetId, table, columnName); - - String queryParams = ""; - - if (filter != null) { - queryParams += "?filter=%s".formatted(filter); - } - return dataRepoClient.get(user, url + queryParams, new TypeReference<>() {}); + var requestModel = new QueryColumnStatisticsRequestModel(); + requestModel.filter(Objects.requireNonNullElse(filter, "")); + return dataRepoClient.post( + user, url, TestUtils.mapToJson(requestModel), new TypeReference<>() {}); } private DataRepoResponse retrieveColumnStatsTextRaw( @@ -1148,13 +1137,10 @@ private DataRepoResponse retrieveColumnStatsTextRaw( String url = "/api/repository/v1/datasets/%s/data/%s/statistics/%s" .formatted(datasetId, table, columnName); - - String queryParams = ""; - - if (filter != null) { - queryParams += "&filter=%s".formatted(filter); - } - return dataRepoClient.get(user, url + queryParams, new TypeReference<>() {}); + var requestModel = new QueryColumnStatisticsRequestModel(); + requestModel.filter(Objects.requireNonNullElse(filter, "")); + return dataRepoClient.post( + user, url, TestUtils.mapToJson(requestModel), new TypeReference<>() {}); } public void assertFailToGetSnapshot(TestConfiguration.User user, UUID snapshotId)