Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate getAllTables api #127

Merged
merged 2 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@ public interface TablesApiHandler {
ApiResponse<GetTableResponseBody> getTable(
String databaseId, String tableId, String actingPrincipal);

/**
* Function to Get all Table Resources in a given databaseId
*
* @param databaseId
* @return Response containing a list of tables that would be returned to the client.
*/
ApiResponse<GetAllTablesResponseBody> getAllTables(String databaseId);

/**
* Function to Get all Table Resources in a given databaseId by filters and return requested
* columns. If no columns are specified only identifier columns are returned.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,21 +44,6 @@ public ApiResponse<GetTableResponseBody> getTable(
.build();
}

@Override
public ApiResponse<GetAllTablesResponseBody> getAllTables(String databaseId) {
tablesApiValidator.validateGetAllTables(databaseId);
return ApiResponse.<GetAllTablesResponseBody>builder()
.httpStatus(HttpStatus.OK)
.responseBody(
GetAllTablesResponseBody.builder()
.results(
tableService.getAllTables(databaseId).stream()
.map(tableDto -> tablesMapper.toGetTableResponseBody(tableDto))
.collect(Collectors.toList()))
.build())
.build();
}

@Override
public ApiResponse<GetAllTablesResponseBody> searchTables(String databaseId) {
tablesApiValidator.validateSearchTables(databaseId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,6 @@ public interface TablesApiValidator {
*/
void validateGetTable(String databaseId, String tableId);

/**
* Function to validate a request to get all Table Resources in a given databaseId
*
* @param databaseId
* @throws com.linkedin.openhouse.common.exception.RequestValidationFailureException if request is
* invalid
*/
void validateGetAllTables(String databaseId);

/**
* Function to validate a request to get all Table Resources in a given databaseId by filters and
* requested columns.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,14 @@ public void validateGetTable(String databaseId, String tableId) {
}

@Override
public void validateGetAllTables(String databaseId) {
public void validateSearchTables(String databaseId) {
List<String> validationFailures = new ArrayList<>();
validateDatabaseId(databaseId, validationFailures);
if (!validationFailures.isEmpty()) {
throw new RequestValidationFailureException(validationFailures);
}
}

@Override
public void validateSearchTables(String databaseId) {
// Validation is similar to GetAllTables.
validateGetAllTables(databaseId);
}

@SuppressWarnings("checkstyle:OperatorWrap")
@Override
public void validateCreateTable(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,30 +66,6 @@ public ResponseEntity<GetTableResponseBody> getTable(
apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus());
}

@Operation(
summary = "List Tables in a Database",
description =
"Returns a list of Table resources present in a database identified by databaseId.",
tags = {"Table"})
@ApiResponses(
value = {
@ApiResponse(responseCode = "200", description = "Table GET_ALL: OK"),
@ApiResponse(responseCode = "401", description = "Table GET_ALL: UNAUTHORIZED"),
@ApiResponse(responseCode = "404", description = "Table GET_ALL: DB NOT_FOUND")
})
@GetMapping(
value = {"/v0/databases/{databaseId}/tables", "/v1/databases/{databaseId}/tables"},
produces = {"application/json"})
public ResponseEntity<GetAllTablesResponseBody> getAllTables(
@Parameter(description = "Database ID", required = true) @PathVariable String databaseId) {

com.linkedin.openhouse.common.api.spec.ApiResponse<GetAllTablesResponseBody> apiResponse =
tablesApiHandler.getAllTables(databaseId);

return new ResponseEntity<>(
apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus());
}

@Operation(
summary = "Search Tables in a Database",
description =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
*/
@Repository
public interface OpenHouseInternalRepository extends CrudRepository<TableDto, TableDtoPrimaryKey> {
List<TableDto> findAllByDatabaseId(String databaseId);

List<TableDtoPrimaryKey> findAllIds();

List<TableDto> searchTables(String databaseId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -476,21 +476,6 @@ public void deleteById(TableDtoPrimaryKey tableDtoPrimaryKey) {
true);
}

@Timed(metricKey = MetricsConstant.REPO_TABLES_FIND_BY_DATABASE_TIME)
@Override
public List<TableDto> findAllByDatabaseId(String databaseId) {
List<Table> tables =
catalog.listTables(Namespace.of(databaseId)).stream()
.map(tableIdentifier -> catalog.loadTable(tableIdentifier))
.collect(Collectors.toList());
return tables.stream()
.map(
table ->
convertToTableDto(
table, fileIOManager, partitionSpecMapper, policiesMapper, tableTypeMapper))
.collect(Collectors.toList());
}

@Timed(metricKey = MetricsConstant.REPO_TABLES_SEARCH_BY_DATABASE_TIME)
@Override
public List<TableDto> searchTables(String databaseId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ public interface TablesService {
*/
TableDto getTable(String databaseId, String tableId, String actingPrincipal);

/**
* Given a databaseId, prepare list of {@link TableDto}s that are part of database.
*
* @param databaseId
* @return list of {@link TableDto}
*/
List<TableDto> getAllTables(String databaseId);

/**
* Given a databaseId, prepare list of {@link TableDto}s.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ public TableDto getTable(String databaseId, String tableId, String actingPrincip
return tableDto;
}

@Override
public List<TableDto> getAllTables(String databaseId) {
return openHouseInternalRepository.findAllByDatabaseId(databaseId);
}

@Override
public List<TableDto> searchTables(String databaseId) {
return openHouseInternalRepository.searchTables(databaseId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Optional;
Expand Down Expand Up @@ -113,13 +112,6 @@ public void testOpenHouseRepository() {

Assertions.assertTrue(openHouseInternalRepository.existsById(key));

List<TableDto> tableDtos =
openHouseInternalRepository.findAllByDatabaseId(creationDTO.getDatabaseId());
Assertions.assertEquals(1, tableDtos.size());
for (TableDto tableDto : tableDtos) {
verifyTable(tableDto);
}

openHouseInternalRepository.deleteById(key);
Assertions.assertFalse(openHouseInternalRepository.existsById(key));
}
Expand Down Expand Up @@ -279,11 +271,6 @@ public void testOpenHouseInvalidTimePartitioningEvolution() {
Assertions.assertFalse(openHouseInternalRepository.existsById(key));
}

@Test
public void testEmptyDatabase() {
Assertions.assertEquals(0, openHouseInternalRepository.findAllByDatabaseId("NOT_FOUND").size());
}

@Test
public void testExistsByIdThatDoesNotExist() {

Expand All @@ -299,11 +286,6 @@ public void testExistsByIdThatDoesNotExist() {
"Cannot create a namespace with a null level", nullPointerException.getMessage());
}

@Test
public void testFindAllByDatabaseIdNotFound() {
Assertions.assertEquals(0, openHouseInternalRepository.findAllByDatabaseId("not_found").size());
}

@Test
public void testFindAllIds() {
openHouseInternalRepository.save(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,24 +237,6 @@ public void testDeleteNotFoundTable() throws Exception {
.andExpect(status().isNotFound());
}

@Test
public void testEmptyDatabase() throws Exception {
mvc.perform(
MockMvcRequestBuilders.get(
ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX
+ "/databases/not_found/tables/")
.accept(MediaType.APPLICATION_JSON))
.andExpect(status().isOk())
.andExpect(content().contentType(MediaType.APPLICATION_JSON))
.andExpect(
content()
.json(
GetAllTablesResponseBody.builder()
.results(new ArrayList<>())
.build()
.toJson()));
}

@Test
public void testCreateTableAlreadyExists() throws Exception {
RequestAndValidateHelper.createTableAndValidateResponse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION;
import static com.linkedin.openhouse.common.schema.IcebergSchemaHelper.*;
import static com.linkedin.openhouse.tables.model.TableModelConstants.*;
import static org.assertj.core.api.Assertions.*;

import com.google.common.collect.ImmutableMap;
import com.linkedin.openhouse.cluster.storage.StorageManager;
Expand All @@ -21,17 +20,13 @@
import com.linkedin.openhouse.tables.common.TableType;
import com.linkedin.openhouse.tables.model.DatabaseDto;
import com.linkedin.openhouse.tables.model.TableDto;
import com.linkedin.openhouse.tables.model.TableModelConstants;
import com.linkedin.openhouse.tables.repository.OpenHouseInternalRepository;
import com.linkedin.openhouse.tables.services.TablesService;
import com.linkedin.openhouse.tables.utils.AuthorizationUtils;
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.stream.Collectors;
import org.apache.iceberg.Schema;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Assertions;
Expand Down Expand Up @@ -236,35 +231,6 @@ public void testTableGetFailIfDoesntExist() {
() -> tablesService.getTable("DB_NOT_FOUND", "TBL_NOT_FOUND", TEST_USER));
}

@Test
public void testGetAll() {
verifyPutTableRequest(TABLE_DTO, null, true);
verifyPutTableRequest(TABLE_DTO_SAME_DB, null, true);

List<TableDto> tables = tablesService.getAllTables(TABLE_DTO.getDatabaseId());
List<TableDto> tableDtoList = new ArrayList<>();
tableDtoList.add(TableModelConstants.TABLE_DTO);
tableDtoList.add(TableModelConstants.TABLE_DTO_SAME_DB);

assertThat(tableDtoList.stream().map(TableDto::getTableId).collect(Collectors.toList()))
.hasSameElementsAs(tables.stream().map(TableDto::getTableId).collect(Collectors.toList()));
assertThat(tableDtoList.stream().map(TableDto::getDatabaseId).collect(Collectors.toList()))
.hasSameElementsAs(
tables.stream().map(TableDto::getDatabaseId).collect(Collectors.toList()));
assertThat(tableDtoList.stream().map(TableDto::getClusterId).collect(Collectors.toList()))
.hasSameElementsAs(
tables.stream().map(TableDto::getClusterId).collect(Collectors.toList()));
assertThat(tableDtoList.stream().map(TableDto::getTableUri).collect(Collectors.toList()))
.hasSameElementsAs(tables.stream().map(TableDto::getTableUri).collect(Collectors.toList()));

assertThat(new ArrayList<>()).hasSameElementsAs(tablesService.getAllTables("DB_NOT_FOUND"));

// Delete as clean up.
tablesService.deleteTable(TABLE_DTO.getDatabaseId(), TABLE_DTO.getTableId(), TEST_USER);
tablesService.deleteTable(
TABLE_DTO_SAME_DB.getDatabaseId(), TABLE_DTO_SAME_DB.getTableId(), TEST_USER);
}

@Test
public void testTableCreateFailsIfAlreadyExist() {
verifyPutTableRequest(TABLE_DTO, null, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,6 @@ public ApiResponse<GetTableResponseBody> getTable(
}
}

@Override
public ApiResponse<GetAllTablesResponseBody> getAllTables(String databaseId) {
switch (databaseId) {
case "d200":
return ApiResponse.<GetAllTablesResponseBody>builder()
.httpStatus(HttpStatus.OK)
.responseBody(RequestConstants.TEST_GET_ALL_TABLES_RESPONSE_BODY)
.build();
case "d404":
throw new NoSuchUserTableException(databaseId, "");
default:
return null;
}
}

@Override
public ApiResponse<GetAllTablesResponseBody> searchTables(String databaseId) {
switch (databaseId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,24 +74,6 @@ public void validateGetTableWithEmptyString() {
() -> tablesApiValidator.validateGetTable("d", ""));
}

@Test
public void validateGetAllTablesSuccess() {
assertDoesNotThrow(() -> tablesApiValidator.validateGetAllTables("d"));
}

@Test
public void validateGetAllTablesSpecialCharacter() {
assertThrows(
RequestValidationFailureException.class,
() -> tablesApiValidator.validateGetAllTables("%%"));
}

@Test
public void validateGetAllTablesEmptyString() {
assertThrows(
RequestValidationFailureException.class, () -> tablesApiValidator.validateGetAllTables(""));
}

@Test
public void validateCreateTableSuccess() {
assertDoesNotThrow(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,6 @@ public void testGetTableFailedPath() throws Exception {
.matches(actualEvent));
}

@Test
jiang95-dev marked this conversation as resolved.
Show resolved Hide resolved
public void testGetAllTablesSuccessfulPath() throws Exception {
mvc.perform(
MockMvcRequestBuilders.get(CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/tables")
.accept(MediaType.APPLICATION_JSON));
Mockito.verify(tableAuditHandler, atLeastOnce()).audit(argCaptor.capture());
TableAuditEvent actualEvent = argCaptor.getValue();
assertTrue(
new ReflectionEquals(TABLE_AUDIT_EVENT_GET_ALL_TABLES_SUCCESS, EXCLUDE_FIELDS)
.matches(actualEvent));
}

@Test
public void testGetAllTablesFailedPath() throws Exception {
mvc.perform(
MockMvcRequestBuilders.get(CURRENT_MAJOR_VERSION_PREFIX + "/databases/d404/tables")
.accept(MediaType.APPLICATION_JSON));
Mockito.verify(tableAuditHandler, atLeastOnce()).audit(argCaptor.capture());
TableAuditEvent actualEvent = argCaptor.getValue();
assertTrue(
new ReflectionEquals(TABLE_AUDIT_EVENT_GET_ALL_TABLES_FAILED, EXCLUDE_FIELDS)
.matches(actualEvent));
}

@Test
public void testCreateTableSuccessfulPath() throws Exception {
// test create table using POST api
Expand Down
Loading
Loading