Deprecate getAllTables api (#127)

## Summary Deprecate getAllTables api in favor of searchTables api. The getAllTables api causes latency issues. It will come back with paging support. ## Changes - [x] Client-facing API Changes - [ ] Internal API Changes - [ ] Bug Fixes - [ ] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [ ] Tests For all the boxes checked, please include additional details of the changes made in this pull request. ## Testing Done  - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [ ] Added new tests for the changes made. - [x] Updated existing tests to reflect the changes made. - [ ] No tests added or updated. Please explain why. If unsure, please feel free to ask for help. - [ ] Some other form of testing like staging or soak time in production. Please explain. For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. For all the boxes checked, include additional details of the changes made in this pull request.
linkedin · Jul 10, 2024 · 5ba28ac · 5ba28ac
1 parent 36aba11
commit 5ba28ac
Show file tree

Hide file tree

Showing 16 changed files with 1 addition and 240 deletions.
diff --git a/...ices/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/TablesApiHandler.java b/...ices/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/TablesApiHandler.java
@@ -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.

diff --git a/...c/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java b/...c/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java
@@ -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);

diff --git a/.../tables/src/main/java/com/linkedin/openhouse/tables/api/validator/TablesApiValidator.java b/.../tables/src/main/java/com/linkedin/openhouse/tables/api/validator/TablesApiValidator.java
@@ -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.

diff --git a/...in/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java b/...in/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java
@@ -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(

diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/controller/TablesController.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/controller/TablesController.java
@@ -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 =

diff --git a/...s/src/main/java/com/linkedin/openhouse/tables/repository/OpenHouseInternalRepository.java b/...s/src/main/java/com/linkedin/openhouse/tables/repository/OpenHouseInternalRepository.java
@@ -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);

diff --git a/...n/java/com/linkedin/openhouse/tables/repository/impl/OpenHouseInternalRepositoryImpl.java b/...n/java/com/linkedin/openhouse/tables/repository/impl/OpenHouseInternalRepositoryImpl.java
@@ -477,21 +477,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) {

diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/services/TablesService.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/TablesService.java
@@ -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.
    *

diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/services/TablesServiceImpl.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/TablesServiceImpl.java
@@ -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);

diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/RepositoryTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/RepositoryTest.java
@@ -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;
@@ -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));
   }
@@ -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() {
 
@@ -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(

diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesControllerTest.java
@@ -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(

diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesServiceTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/TablesServiceTest.java
@@ -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;
@@ -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;
@@ -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);

diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/MockTablesApiHandler.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/MockTablesApiHandler.java
@@ -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) {

diff --git a/...ices/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java b/...ices/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/TablesValidatorTest.java
@@ -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(

diff --git a/...les/src/test/java/com/linkedin/openhouse/tables/mock/audit/TablesApiHandlerAuditTest.java b/...les/src/test/java/com/linkedin/openhouse/tables/mock/audit/TablesApiHandlerAuditTest.java
@@ -58,30 +58,6 @@ public void testGetTableFailedPath() throws Exception {
             .matches(actualEvent));
   }
 
-  @Test
-  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