diff --git a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapper.java b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapper.java index 075538df2..5ddfea4a2 100644 --- a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapper.java +++ b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapper.java @@ -62,7 +62,16 @@ private static boolean isHtsField(String key) { && HouseTableSerdeUtils.HTS_FIELD_NAMES.contains(stripOhNamespace(key)); } + /** + * MapStruct picks this up as an implicit {@code String -> String} conversion for every String + * property on the generated mappers, so it must tolerate null. {@code entityType} is the first + * genuinely nullable String on {@link HouseTable} (it stays null for legacy tables), which is + * what surfaced this; a null {@code tableVersion} would have hit it too. + */ static String stripOhNamespace(String key) { + if (key == null) { + return null; + } return IS_OH_PREFIXED.test(key) ? key.substring(OPENHOUSE_NAMESPACE.length()) : key; } } diff --git a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableSerdeUtils.java b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableSerdeUtils.java index 6a303f797..61d62d95a 100644 --- a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableSerdeUtils.java +++ b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableSerdeUtils.java @@ -35,6 +35,8 @@ private HouseTableSerdeUtils() { // no-op for util class constructor } + @VisibleForTesting public static final String ENTITY_TYPE_FIELD_NAME = "entityType"; + @VisibleForTesting public static String getCanonicalFieldName(String htsField) { return OPENHOUSE_NAMESPACE + htsField; diff --git a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/model/HouseTable.java b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/model/HouseTable.java index dcc9acc80..70b55a92b 100644 --- a/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/model/HouseTable.java +++ b/iceberg/openhouse/internalcatalog/src/main/java/com/linkedin/openhouse/internal/catalog/model/HouseTable.java @@ -51,4 +51,11 @@ public class HouseTable { * with this table. */ private String storageType; + + /** + * As a private non-static field this is picked up automatically by {@link + * com.linkedin.openhouse.internal.catalog.mapper.HouseTableSerdeUtils#HTS_FIELD_NAMES}, so it + * serializes as the {@code openhouse.entityType} table property. + */ + private String entityType; } diff --git a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/OpenHouseInternalTableOperationsTest.java b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/OpenHouseInternalTableOperationsTest.java index 12092c160..3c0d92145 100644 --- a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/OpenHouseInternalTableOperationsTest.java +++ b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/OpenHouseInternalTableOperationsTest.java @@ -2128,4 +2128,66 @@ void testRefreshMetadataMissingFileThrowsInvalidTableMetadataException() { InvalidTableMetadataException.class, () -> openHouseInternalTableOperations.refreshMetadata(nonExistentPath)); } + + /** + * Backward compatibility: ordinary table commits must NOT start stamping the discriminator. + * Adding {@code openhouse.entityType=TABLE} to every table commit would rewrite every table's + * metadata.json content and mask the "NULL means table" compatibility contract that all the + * legacy-row assertions depend on. + */ + @Test + void normalTableCommitDoesNotStampEntityType() { + AtomicReference savedHouseTable = new AtomicReference<>(); + when(mockHouseTableMapper.toHouseTable(Mockito.any(TableMetadata.class), Mockito.any())) + .thenAnswer( + invocation -> { + TableMetadata tableMetadata = invocation.getArgument(0); + // Mirror the real mapper: the pointer's entityType comes solely from the + // openhouse.entityType property, so a null here proves the commit did not stamp it. + HouseTable mapped = + HouseTable.builder() + .databaseId(TEST_TABLE_IDENTIFIER.namespace().toString()) + .tableId(TEST_TABLE_IDENTIFIER.name()) + .tableLocation( + tableMetadata.properties().get(getCanonicalFieldName("tableLocation"))) + .entityType( + tableMetadata.properties().get(getCanonicalFieldName("entityType"))) + .build(); + savedHouseTable.set(mapped); + return mapped; + }); + when(mockHouseTableRepository.save(Mockito.any(HouseTable.class))) + .thenAnswer(invocation -> invocation.getArgument(0)); + + Map properties = new HashMap<>(BASE_TABLE_METADATA.properties()); + properties.put(getCanonicalFieldName("tableLocation"), TEST_LOCATION); + TableMetadata metadata = BASE_TABLE_METADATA.replaceProperties(properties); + + try (MockedStatic parserMock = + Mockito.mockStatic(TableMetadataParser.class, Mockito.CALLS_REAL_METHODS)) { + parserMock + .when( + () -> + TableMetadataParser.write( + Mockito.any(TableMetadata.class), + Mockito.any(org.apache.iceberg.io.OutputFile.class))) + .thenAnswer(invocation -> null); + + openHouseInternalTableOperations.doCommit(BASE_TABLE_METADATA, metadata); + + Mockito.verify(mockHouseTableMapper).toHouseTable(tblMetadataCaptor.capture(), Mockito.any()); + Map committedProperties = tblMetadataCaptor.getValue().properties(); + Assertions.assertFalse( + committedProperties.containsKey(getCanonicalFieldName("entityType")), + "Ordinary table commits must not write openhouse.entityType, but properties were: " + + committedProperties); + + Assertions.assertNull( + savedHouseTable.get().getEntityType(), + "The saved pointer for an ordinary table commit must keep a null discriminator"); + // NOTE: the captured-properties assertion above is the load-bearing one. This second + // assertion runs through a hand-rolled mirror of the real mapper, so it is null largely + // because the captured properties are; it guards the mapping wiring, not the commit itself. + } + } } diff --git a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapperTest.java b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapperTest.java index 83bfc56e4..9c5e78b6b 100644 --- a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapperTest.java +++ b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/mapper/HouseTableMapperTest.java @@ -7,6 +7,7 @@ import com.linkedin.openhouse.housetables.client.api.ToggleStatusApi; import com.linkedin.openhouse.housetables.client.api.UserTableApi; import com.linkedin.openhouse.housetables.client.invoker.ApiClient; +import com.linkedin.openhouse.housetables.client.model.UserTable; import com.linkedin.openhouse.internal.catalog.fileio.FileIOManager; import com.linkedin.openhouse.internal.catalog.model.HouseTable; import com.linkedin.openhouse.internal.catalog.repository.HouseTableRepository; @@ -70,4 +71,59 @@ public void simpleMapperTest() { Assertions.assertEquals("table", houseTable.getTableId()); Assertions.assertEquals("local", houseTable.getStorageType()); } + + /** + * The discriminator must survive the generated HTS client model in both directions, otherwise the + * pointer would be written or read back without its type. + */ + @Test + public void houseTableGeneratedUserTableRoundTripPreservesEntityType() { + HouseTable viewHouseTable = + HouseTable.builder() + .databaseId("d1") + .tableId("v1") + .tableLocation("/base/d1/v1-uuid/00001-x.metadata.json") + .tableVersion("INITIAL_VERSION") + .entityType("VIEW") + .build(); + + UserTable viewUserTable = houseTableMapper.toUserTable(viewHouseTable); + Assertions.assertEquals("VIEW", viewUserTable.getEntityType()); + Assertions.assertEquals( + "/base/d1/v1-uuid/00001-x.metadata.json", viewUserTable.getMetadataLocation()); + + HouseTable viewBack = houseTableMapper.toHouseTable(viewUserTable); + Assertions.assertEquals("VIEW", viewBack.getEntityType()); + Assertions.assertEquals("/base/d1/v1-uuid/00001-x.metadata.json", viewBack.getTableLocation()); + + HouseTable tableHouseTable = + HouseTable.builder() + .databaseId("d1") + .tableId("t1") + .tableLocation("/base/d1/t1-uuid/00001-y.metadata.json") + .entityType("TABLE") + .build(); + UserTable tableUserTable = houseTableMapper.toUserTable(tableHouseTable); + Assertions.assertEquals("TABLE", tableUserTable.getEntityType()); + Assertions.assertEquals("TABLE", houseTableMapper.toHouseTable(tableUserTable).getEntityType()); + } + + /** Backward compatibility: an absent discriminator maps to null, never "TABLE". */ + @Test + public void missingEntityTypeMapsToLegacyTableNull() { + HadoopFileIO fileIO = new HadoopFileIO(new Configuration()); + LocalStorage localStorage = mock(LocalStorage.class); + when(fileIOManager.getStorage(fileIO)).thenReturn(localStorage); + when(localStorage.getType()).thenReturn(StorageType.LOCAL); + + HouseTable fromProperties = + houseTableMapper.toHouseTable(ImmutableMap.of("databaseId", "d1", "tableId", "t1"), fileIO); + Assertions.assertNull(fromProperties.getEntityType()); + + UserTable legacyUserTable = + houseTableMapper.toUserTable( + HouseTable.builder().databaseId("d1").tableId("t1").tableLocation("loc").build()); + Assertions.assertNull(legacyUserTable.getEntityType()); + Assertions.assertNull(houseTableMapper.toHouseTable(legacyUserTable).getEntityType()); + } } diff --git a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/model/HouseTableTest.java b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/model/HouseTableTest.java index 063bcea88..8855f59db 100644 --- a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/model/HouseTableTest.java +++ b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/model/HouseTableTest.java @@ -60,4 +60,28 @@ public void testHouseTableDefaultValues() { Assertions.fail(e); } } + + /** + * {@code HTS_FIELD_NAMES} is derived reflectively from HouseTable's private fields, and {@code + * HouseTableMapper.extractRawHTSFields} only carries properties whose stripped key is in that + * set. So the discriminator is only serialized through Iceberg table properties if it is a real + * private field named exactly {@code entityType}, and its canonical property key must be {@code + * openhouse.entityType}. + */ + @Test + public void testEntityTypeDefaultAndSerdeRegistration() { + Assertions.assertNull( + HouseTable.builder().build().getEntityType(), + "entityType must default to null so ordinary table commits keep writing no discriminator"); + + Assertions.assertTrue( + HouseTableSerdeUtils.HTS_FIELD_NAMES.contains(HouseTableSerdeUtils.ENTITY_TYPE_FIELD_NAME), + "entityType must be reflected into HTS_FIELD_NAMES: " + + HouseTableSerdeUtils.HTS_FIELD_NAMES); + + Assertions.assertEquals("entityType", HouseTableSerdeUtils.ENTITY_TYPE_FIELD_NAME); + Assertions.assertEquals( + "openhouse.entityType", + HouseTableSerdeUtils.getCanonicalFieldName(HouseTableSerdeUtils.ENTITY_TYPE_FIELD_NAME)); + } } diff --git a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/repository/HouseTableRepositoryImplTest.java b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/repository/HouseTableRepositoryImplTest.java index a5c53d93a..627429318 100644 --- a/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/repository/HouseTableRepositoryImplTest.java +++ b/iceberg/openhouse/internalcatalog/src/test/java/com/linkedin/openhouse/internal/catalog/repository/HouseTableRepositoryImplTest.java @@ -754,4 +754,64 @@ public void testRestoreSoftDeletedTablesFailsWhenTableDoesNotExist() { htsRepo.restoreTable( HOUSE_TABLE.getDatabaseId(), HOUSE_TABLE.getTableId(), System.currentTimeMillis())); } + + /** + * This adapter is an HTTP client, not a query owner. It must faithfully carry the discriminator + * across the wire in both directions so the internal catalog's Java guards see the real stored + * value. It must NOT gain a local view filter of its own — filtering belongs in the HTS query so + * page counts stay correct. + */ + @Test + public void testRepoPointAndSavePreserveEntityType() { + HouseTable viewHouseTable = HOUSE_TABLE.toBuilder().entityType("VIEW").build(); + + EntityResponseBodyUserTable getResponse = new EntityResponseBodyUserTable(); + getResponse.entity(houseTableMapper.toUserTable(viewHouseTable)); + mockHtsServer.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody((new Gson()).toJson(getResponse)) + .addHeader("Content-Type", "application/json")); + + HouseTable found = + htsRepo + .findById( + HouseTablePrimaryKey.builder() + .tableId(HOUSE_TABLE.getTableId()) + .databaseId(HOUSE_TABLE.getDatabaseId()) + .build()) + .get(); + Assertions.assertEquals("VIEW", found.getEntityType()); + Assertions.assertEquals(HOUSE_TABLE.getTableLocation(), found.getTableLocation()); + + EntityResponseBodyUserTable putResponse = new EntityResponseBodyUserTable(); + putResponse.entity(houseTableMapper.toUserTable(viewHouseTable)); + mockHtsServer.enqueue( + new MockResponse() + .setResponseCode(201) + .setBody((new Gson()).toJson(putResponse)) + .addHeader("Content-Type", "application/json")); + + HouseTable saved = htsRepo.save(viewHouseTable); + Assertions.assertEquals("VIEW", saved.getEntityType()); + Assertions.assertEquals(HOUSE_TABLE.getTableLocation(), saved.getTableLocation()); + + // A legacy table pointer with no discriminator still round trips as null. + EntityResponseBodyUserTable legacyResponse = new EntityResponseBodyUserTable(); + legacyResponse.entity(houseTableMapper.toUserTable(HOUSE_TABLE)); + mockHtsServer.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody((new Gson()).toJson(legacyResponse)) + .addHeader("Content-Type", "application/json")); + Assertions.assertNull( + htsRepo + .findById( + HouseTablePrimaryKey.builder() + .tableId(HOUSE_TABLE.getTableId()) + .databaseId(HOUSE_TABLE.getDatabaseId()) + .build()) + .get() + .getEntityType()); + } } diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java index 3f11694b4..3c0616858 100644 --- a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java +++ b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java @@ -19,4 +19,10 @@ private ValidatorConstants() {} "Only alphanumerics, hyphen and underscore supported"; public static final int MAX_ALLOWED_CLUSTERING_COLUMNS = 4; public static final String INITIAL_TABLE_VERSION = "INITIAL_VERSION"; + + /** A null/absent value is allowed and means a legacy table row. */ + public static final String ENTITY_TYPE_REGEX = "(?i)^(TABLE|VIEW)$"; + + public static final String ENTITY_TYPE_ERROR_MSG = + "Only TABLE and VIEW are supported entity types (case-insensitive)"; } diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/UserTable.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/UserTable.java index 9369fddc2..f7854a0ca 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/UserTable.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/spec/model/UserTable.java @@ -2,6 +2,8 @@ import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_ERROR_MSG; import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_REGEX; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ENTITY_TYPE_ERROR_MSG; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ENTITY_TYPE_REGEX; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.gson.Gson; @@ -61,6 +63,15 @@ public class UserTable { @JsonProperty(value = "creationTime") private Long creationTime; + @Schema( + description = + "Type of the catalog object occupying this (databaseId, tableId) key. Null or 'TABLE' " + + "means a table; 'VIEW' means a view. Matched case-insensitively.", + example = "TABLE") + @JsonProperty(value = "entityType") + @Pattern(regexp = ENTITY_TYPE_REGEX, message = ENTITY_TYPE_ERROR_MSG) + private String entityType; + @Schema( description = "Timestamp in milliseconds when the table was soft-deleted. " diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/dto/model/UserTableDto.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/dto/model/UserTableDto.java index 68b603026..11e700a23 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/dto/model/UserTableDto.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/dto/model/UserTableDto.java @@ -23,6 +23,8 @@ public class UserTableDto { Long creationTime; + String entityType; + Long deletedAtMs; Long purgeAfterMs; diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/model/UserTableRow.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/model/UserTableRow.java index 7ee862d3e..01593f622 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/model/UserTableRow.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/model/UserTableRow.java @@ -32,4 +32,7 @@ public class UserTableRow { String storageType; Long creationTime; + + /** Nullable and without a default so existing rows need no backfill. */ + String entityType; } diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/repository/impl/jdbc/UserTableHtsJdbcRepository.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/repository/impl/jdbc/UserTableHtsJdbcRepository.java index 1c6ae8c06..099847976 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/repository/impl/jdbc/UserTableHtsJdbcRepository.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/repository/impl/jdbc/UserTableHtsJdbcRepository.java @@ -39,11 +39,37 @@ Optional findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase( void deleteByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(String databaseId, String tableId); + String COMMON_FILTER_CLAUSES = + "(:databaseId IS NULL OR lower(u.databaseId) = lower(:databaseId)) AND " + + "(:tableId IS NULL OR lower(u.tableId) = lower(:tableId)) AND " + + "(:tableVersion IS NULL OR u.version = :tableVersion) AND " + + "(:metadataLocation IS NULL OR u.metadataLocation = :metadataLocation) AND " + + "(:storageType IS NULL OR u.storageType = :storageType) AND " + + "(:creationTime IS NULL OR u.creationTime = :creationTime)"; + + String TABLE_ROW_PREDICATE = "(u.entityType IS NULL OR upper(u.entityType) = 'TABLE')"; + + String PATTERN_KEY_CLAUSES = + "lower(u.databaseId) = lower(:databaseId) AND " + + "lower(u.tableId) LIKE lower(:tableIdPattern)"; + + /** + * Table-scoped point read serving {@code getUserTable}, the single HTS endpoint behind every + * table point read in the tables service. The neutral {@link + * #findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase} above stays unfiltered because the writers + * must see a row of any type to detect a collision at a shared key. + */ + @Query( + "SELECT u FROM UserTableRow u WHERE " + + "lower(u.databaseId) = lower(:databaseId) AND " + + "lower(u.tableId) = lower(:tableId) AND " + + TABLE_ROW_PREDICATE) + Optional findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase( + @Param("databaseId") String databaseId, @Param("tableId") String tableId); + @Query("SELECT DISTINCT databaseId FROM UserTableRow") Iterable findAllDistinctDatabaseIds(); - Iterable findAllByDatabaseIdIgnoreCase(String databaseId); - Iterable findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( String databaseId, String tableIdPattern); @@ -52,19 +78,10 @@ Iterable findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + "(:databaseId IS NULL OR lower(u.databaseId) = lower(:databaseId))") Page findAllDistinctDatabaseIds(String databaseId, Pageable pageable); - Page findAllByDatabaseIdIgnoreCase(String databaseId, Pageable pageable); - Page findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( String databaseId, String tableIdPattern, Pageable pageable); - @Query( - "select DISTINCT u from UserTableRow u where " - + "(:databaseId IS NULL OR lower(u.databaseId) = lower(:databaseId)) AND " - + "(:tableId IS NULL OR lower(u.tableId) = lower(:tableId)) AND " - + "(:tableVersion IS NULL OR u.version = :tableVersion) AND " - + "(:metadataLocation IS NULL OR u.metadataLocation = :metadataLocation) AND " - + "(:storageType IS NULL OR u.storageType = :storageType) AND " - + "(:creationTime IS NULL OR u.creationTime = :creationTime)") + @Query("select DISTINCT u from UserTableRow u where " + COMMON_FILTER_CLAUSES) Page findAllByFilters( String databaseId, String tableId, @@ -74,14 +91,7 @@ Page findAllByFilters( Long creationTime, Pageable pageable); - @Query( - "select DISTINCT u from UserTableRow u where " - + "(:databaseId IS NULL OR lower(u.databaseId) = lower(:databaseId)) AND " - + "(:tableId IS NULL OR lower(u.tableId) = lower(:tableId)) AND " - + "(:tableVersion IS NULL OR u.version = :tableVersion) AND " - + "(:metadataLocation IS NULL OR u.metadataLocation = :metadataLocation) AND " - + "(:storageType IS NULL OR u.storageType = :storageType) AND " - + "(:creationTime IS NULL OR u.creationTime = :creationTime)") + @Query("select DISTINCT u from UserTableRow u where " + COMMON_FILTER_CLAUSES) Iterable findAllByFilters( String databaseId, String tableId, @@ -90,6 +100,60 @@ Iterable findAllByFilters( String storageType, Long creationTime); + @Query( + "SELECT u FROM UserTableRow u WHERE " + PATTERN_KEY_CLAUSES + " AND " + TABLE_ROW_PREDICATE) + Iterable findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + @Param("databaseId") String databaseId, @Param("tableIdPattern") String tableIdPattern); + + @Query( + value = + "SELECT u FROM UserTableRow u WHERE " + + PATTERN_KEY_CLAUSES + + " AND " + + TABLE_ROW_PREDICATE, + countQuery = + "SELECT COUNT(u) FROM UserTableRow u WHERE " + + PATTERN_KEY_CLAUSES + + " AND " + + TABLE_ROW_PREDICATE) + Page findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + @Param("databaseId") String databaseId, + @Param("tableIdPattern") String tableIdPattern, + Pageable pageable); + + @Query( + value = + "select DISTINCT u from UserTableRow u where " + + COMMON_FILTER_CLAUSES + + " AND " + + TABLE_ROW_PREDICATE, + countQuery = + "select COUNT(DISTINCT u) from UserTableRow u where " + + COMMON_FILTER_CLAUSES + + " AND " + + TABLE_ROW_PREDICATE) + Page findAllTablesByFilters( + @Param("databaseId") String databaseId, + @Param("tableId") String tableId, + @Param("tableVersion") String tableVersion, + @Param("metadataLocation") String metadataLocation, + @Param("storageType") String storageType, + @Param("creationTime") Long creationTime, + Pageable pageable); + + @Query( + "select DISTINCT u from UserTableRow u where " + + COMMON_FILTER_CLAUSES + + " AND " + + TABLE_ROW_PREDICATE) + Iterable findAllTablesByFilters( + @Param("databaseId") String databaseId, + @Param("tableId") String tableId, + @Param("tableVersion") String tableVersion, + @Param("metadataLocation") String metadataLocation, + @Param("storageType") String storageType, + @Param("creationTime") Long creationTime); + /* * The following methods are required to maintain the generality of the interface {@link com.linkedin.openhouse.housetables.repository.HtsRepository} */ diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/services/UserTablesServiceImpl.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/services/UserTablesServiceImpl.java index f9b150862..27f3f3227 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/services/UserTablesServiceImpl.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/services/UserTablesServiceImpl.java @@ -57,8 +57,7 @@ public UserTableDto getUserTable(String databaseId, String tableId) { try { userTableRow = htsJdbcRepository - .findById( - UserTableRowPrimaryKey.builder().databaseId(databaseId).tableId(tableId).build()) + .findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(databaseId, tableId) .orElseThrow(NoSuchElementException::new); } catch (NoSuchElementException ne) { throw new NoSuchUserTableException(databaseId, tableId, ne); @@ -288,7 +287,8 @@ private List listTables(UserTable userTable) { () -> StreamSupport.stream( htsJdbcRepository - .findAllByDatabaseIdIgnoreCase(userTable.getDatabaseId()) + .findAllTablesByFilters( + userTable.getDatabaseId(), null, null, null, null, null) .spliterator(), false) .map(userTableRow -> userTablesMapper.toUserTableDto(userTableRow)) @@ -302,7 +302,8 @@ private Page listTables(UserTable userTable, int page, int size, S return METRICS_REPORTER.executeWithStats( () -> htsJdbcRepository - .findAllByFilters(userTable.getDatabaseId(), null, null, null, null, null, pageable) + .findAllTablesByFilters( + userTable.getDatabaseId(), null, null, null, null, null, pageable) .map(userTableRow -> userTablesMapper.toUserTableDto(userTableRow)), MetricsConstant.HTS_PAGE_TABLES_TIME); } @@ -313,7 +314,7 @@ private List listTablesWithPattern(UserTable userTable) { () -> StreamSupport.stream( htsJdbcRepository - .findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + .findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( userTable.getDatabaseId(), userTable.getTableId()) .spliterator(), false) @@ -329,7 +330,7 @@ private Page listTablesWithPattern( return METRICS_REPORTER.executeWithStats( () -> htsJdbcRepository - .findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + .findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( userTable.getDatabaseId(), userTable.getTableId(), pageable) .map(userTableRow -> userTablesMapper.toUserTableDto(userTableRow)), MetricsConstant.HTS_PAGE_TABLES_TIME); @@ -343,7 +344,7 @@ private Page searchTables(UserTable userTable, int page, int size, return METRICS_REPORTER.executeWithStats( () -> htsJdbcRepository - .findAllByFilters( + .findAllTablesByFilters( userTable.getDatabaseId(), userTable.getTableId(), userTable.getTableVersion(), @@ -363,7 +364,7 @@ private List searchTables(UserTable userTable) { () -> StreamSupport.stream( htsJdbcRepository - .findAllByFilters( + .findAllTablesByFilters( userTable.getDatabaseId(), userTable.getTableId(), userTable.getTableVersion(), diff --git a/services/housetables/src/main/resources/schema.sql b/services/housetables/src/main/resources/schema.sql index 81317e873..eee3124b4 100644 --- a/services/housetables/src/main/resources/schema.sql +++ b/services/housetables/src/main/resources/schema.sql @@ -6,6 +6,7 @@ CREATE TABLE IF NOT EXISTS user_table_row ( metadata_location VARCHAR (512) , storage_type VARCHAR (128) DEFAULT 'hdfs' NOT NULL, creation_time BIGINT DEFAULT NULL, + entity_type VARCHAR (128) DEFAULT NULL, last_modified_time TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, ETL_TS DATETIME(6) DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), PRIMARY KEY (database_id, table_id) diff --git a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsControllerTest.java b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsControllerTest.java index d77a256f4..2969574d0 100644 --- a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsControllerTest.java +++ b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsControllerTest.java @@ -1,7 +1,9 @@ package com.linkedin.openhouse.housetables.e2e.usertable; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; import static com.linkedin.openhouse.housetables.model.TestHouseTableModelConstants.*; import static com.linkedin.openhouse.housetables.model.TestHtsApiConstants.*; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.*; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; @@ -28,6 +30,9 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; @@ -760,4 +765,324 @@ public void testPurgeAllSoftDeletedTables() throws Exception { .andExpect(status().isOk()) .andExpect(jsonPath("$.pageResults.content", hasSize(0))); } + + // --------------------------------------------------------------------------------------------- + // entityType discriminator over HTTP + // --------------------------------------------------------------------------------------------- + + /** + * Canonical interleaved fixture, seeded in its own database so it does not disturb the {@code + * test_db0} counts asserted by the tests above. + */ + private static final String ENTITY_TYPE_DB = "entity_type_db"; + + /** + * The HTTP contract the tables service actually consumes: a view at a table's key is a 404, the + * same response an absent row produces, so no client-side check is needed to hide it. + */ + @ParameterizedTest + @ValueSource(strings = {"VIEW", "view", "ViEw", "UNKNOWN"}) + public void testGetUserTableReturnsNotFoundForNonTableRow(String entityType) throws Exception { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "point_read", entityType)); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables") + .param("databaseId", ENTITY_TYPE_DB) + .param("tableId", "point_read") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.status", is(equalTo(HttpStatus.NOT_FOUND.name())))); + + assertThat( + htsRepository + .findById( + UserTableRowPrimaryKey.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("point_read") + .build()) + .isPresent()) + .isTrue(); + } + + @ParameterizedTest + @CsvSource( + nullValues = "NULL", + value = {"NULL", "TABLE", "table", "TaBlE"}) + public void testGetUserTableReturnsNullAndTableRows(String entityType) throws Exception { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "point_read", entityType)); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables") + .param("databaseId", ENTITY_TYPE_DB) + .param("tableId", "point_read") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.entity.tableId", is(equalTo("point_read")))); + } + + private UserTableRow entityTypeRow(String databaseId, String tableId, String entityType) { + return UserTableRow.builder() + .databaseId(databaseId) + .tableId(tableId) + .version(null) + .metadataLocation(String.format("/openhouse/%s/%s/v0_metadata.json", databaseId, tableId)) + .storageType(TEST_DEFAULT_STORAGE_TYPE) + .creationTime(TEST_CREATION_TIME) + .entityType(entityType) + .build(); + } + + private void seedCanonicalRows(String prefix) { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t00_legacy", null)); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t01_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t02_explicit", "TABLE")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t03_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t04_legacy", null)); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t05_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t06_explicit", "TABLE")); + } + + private static MultiValueMap queryParams(String... keyValues) { + Map> paramsInternal = new HashMap<>(); + for (int i = 0; i < keyValues.length; i += 2) { + paramsInternal.put(keyValues[i], Collections.singletonList(keyValues[i + 1])); + } + return new MultiValueMapAdapter(paramsInternal); + } + + /** Both v0 table query families exclude views and keep legacy NULL rows. */ + @Test + public void testTableQueriesExcludeViewsAndKeepLegacyRows() throws Exception { + seedCanonicalRows(""); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB)) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.results", hasSize(4))) + .andExpect( + jsonPath( + "$.results[*].tableId", + containsInAnyOrder("t00_legacy", "t02_explicit", "t04_legacy", "t06_explicit"))) + .andExpect(jsonPath("$.results[*].tableId", not(hasItem("t01_view")))) + .andExpect(jsonPath("$.results[*].tableId", not(hasItem("t03_view")))) + .andExpect(jsonPath("$.results[*].tableId", not(hasItem("t05_view")))); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB, "tableId", "t0%")) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.results", hasSize(4))) + .andExpect( + jsonPath( + "$.results[*].tableId", + containsInAnyOrder("t00_legacy", "t02_explicit", "t04_legacy", "t06_explicit"))); + } + + /** + * Anti-post-filter assertion over HTTP for the v1 paged query families: an implementation that + * filters the returned page would report totalElements=7/totalPages=4 and a 1-row first page. + */ + @Test + public void testPaginatedTableQueriesFilterBeforePaging() throws Exception { + seedCanonicalRows(""); + + mvc.perform( + MockMvcRequestBuilders.get("/v1/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB)) + .param("page", "0") + .param("size", "2") + .param("sortBy", "tableId") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.pageResults.totalElements", is(4))) + .andExpect(jsonPath("$.pageResults.totalPages", is(2))) + .andExpect(jsonPath("$.pageResults.content", hasSize(2))) + .andExpect(jsonPath("$.pageResults.content[0].tableId", is("t00_legacy"))) + .andExpect(jsonPath("$.pageResults.content[1].tableId", is("t02_explicit"))); + + mvc.perform( + MockMvcRequestBuilders.get("/v1/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB)) + .param("page", "1") + .param("size", "2") + .param("sortBy", "tableId") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.pageResults.totalElements", is(4))) + .andExpect(jsonPath("$.pageResults.totalPages", is(2))) + .andExpect(jsonPath("$.pageResults.content", hasSize(2))) + .andExpect(jsonPath("$.pageResults.content[0].tableId", is("t04_legacy"))) + .andExpect(jsonPath("$.pageResults.content[1].tableId", is("t06_explicit"))); + + // Same assertions on the pattern form. + mvc.perform( + MockMvcRequestBuilders.get("/v1/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB, "tableId", "t0%")) + .param("page", "0") + .param("size", "2") + .param("sortBy", "tableId") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.pageResults.totalElements", is(4))) + .andExpect(jsonPath("$.pageResults.totalPages", is(2))) + .andExpect(jsonPath("$.pageResults.content", hasSize(2))) + .andExpect(jsonPath("$.pageResults.content[0].tableId", is("t00_legacy"))) + .andExpect(jsonPath("$.pageResults.content[1].tableId", is("t02_explicit"))); + } + + /** + * The discriminator survives the HTTP write boundary, and legacy writers stay null. A view cannot + * be read back through {@code GET /hts/tables} because that read is table-scoped; the neutral + * entity read is deferred, so the PUT response and the persisted row are what pin the write. + */ + @Test + public void testEntityTypePutAndGetRoundTrip() throws Exception { + UserTable viewEntity = + UserTable.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("put_view") + .tableVersion(INITIAL_TABLE_VERSION) + .metadataLocation("/openhouse/entity_type_db/put_view/v0_metadata.json") + .entityType("VIEW") + .build(); + + mvc.perform( + MockMvcRequestBuilders.put("/hts/tables") + .contentType(MediaType.APPLICATION_JSON) + .content( + CreateUpdateEntityRequestBody.builder() + .entity(viewEntity) + .build() + .toJson()) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.entity.entityType", is("VIEW"))); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables") + .param("databaseId", ENTITY_TYPE_DB) + .param("tableId", "put_view") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isNotFound()); + + assertThat( + htsRepository + .findById( + UserTableRowPrimaryKey.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("put_view") + .build()) + .get() + .getEntityType()) + .isEqualTo("VIEW"); + + // A legacy PUT that omits the field must stay null end-to-end. + UserTable legacyEntity = + UserTable.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("put_legacy") + .tableVersion(INITIAL_TABLE_VERSION) + .metadataLocation("/openhouse/entity_type_db/put_legacy/v0_metadata.json") + .build(); + + mvc.perform( + MockMvcRequestBuilders.put("/hts/tables") + .contentType(MediaType.APPLICATION_JSON) + .content( + CreateUpdateEntityRequestBody.builder() + .entity(legacyEntity) + .build() + .toJson()) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.entity.entityType").doesNotExist()); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables") + .param("databaseId", ENTITY_TYPE_DB) + .param("tableId", "put_legacy") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.entity.entityType").doesNotExist()); + + assertThat( + htsRepository + .findById( + UserTableRowPrimaryKey.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("put_legacy") + .build()) + .get() + .getEntityType()) + .isNull(); + } + + /** + * {@code /hts/tables/query} is table-scoped by path, so {@code entityType} is not a supported + * query parameter. It is mapped onto the request object but never reaches a predicate, so a + * client that sends one is silently answered with tables. + */ + @Test + public void testEntityTypeQueryParameterIsIgnored() throws Exception { + seedCanonicalRows(""); + + mvc.perform( + MockMvcRequestBuilders.get("/hts/tables/query") + .params(queryParams("databaseId", ENTITY_TYPE_DB, "entityType", "VIEW")) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.results", hasSize(4))) + .andExpect( + jsonPath( + "$.results[*].tableId", + containsInAnyOrder("t00_legacy", "t02_explicit", "t04_legacy", "t06_explicit"))); + } + + /** + * Publish-boundary defense in depth. Issues the exact HTS PUT a table create would emit at a key + * already occupied by a VIEW pointer: tableVersion=INITIAL_VERSION, no entityType, and a + * different candidate metadataLocation. The pointer must be rejected with 409 and left + * byte-identical — same numeric JPA {@code version}, {@code entityType} and {@code + * metadataLocation}. + */ + @Test + public void testCreateTablePointerPublishCannotOverwriteView() throws Exception { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "occupied_by_view", "VIEW")); + + UserTableRowPrimaryKey key = + UserTableRowPrimaryKey.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("occupied_by_view") + .build(); + UserTableRow before = htsRepository.findById(key).get(); + + UserTable tableCreatePut = + UserTable.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("occupied_by_view") + .tableVersion(INITIAL_TABLE_VERSION) + .metadataLocation( + "/openhouse/entity_type_db/occupied_by_view-uuid/00001-candidate.metadata.json") + .build(); + + mvc.perform( + MockMvcRequestBuilders.put("/hts/tables") + .contentType(MediaType.APPLICATION_JSON) + .content( + CreateUpdateEntityRequestBody.builder() + .entity(tableCreatePut) + .build() + .toJson()) + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isConflict()); + + UserTableRow after = htsRepository.findById(key).get(); + assertThat(after.getEntityType()).isEqualTo("VIEW"); + assertThat(after.getEntityType()).isEqualTo(before.getEntityType()); + assertThat(after.getVersion()).isEqualTo(before.getVersion()); + assertThat(after.getMetadataLocation()).isEqualTo(before.getMetadataLocation()); + } } diff --git a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsRepositoryTest.java b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsRepositoryTest.java index 1a51289c3..5819e6e9d 100644 --- a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsRepositoryTest.java +++ b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/HtsRepositoryTest.java @@ -10,14 +10,23 @@ import com.linkedin.openhouse.housetables.model.UserTableRow; import com.linkedin.openhouse.housetables.model.UserTableRowPrimaryKey; import com.linkedin.openhouse.housetables.repository.impl.jdbc.UserTableHtsJdbcRepository; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.stream.Collectors; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; import org.springframework.orm.ObjectOptimisticLockingFailureException; import org.springframework.test.context.ContextConfiguration; @@ -25,6 +34,29 @@ @ContextConfiguration(initializers = PropertyOverrideContextInitializer.class) public class HtsRepositoryTest { + /** + * Canonical interleaved fixture. Four visible tables (two legacy NULL, two explicit TABLE) are + * interleaved with three VIEW rows so that a fetch-then-filter implementation returns a SHORT + * first page (1 row) and totalElements=7, while the correct pre-pagination predicate returns a + * full page (2 rows) and totalElements=4. + */ + private static final String ENTITY_TYPE_DB = "entity_type_db"; + + private static final String[] CANONICAL_TABLE_IDS = { + "t00_legacy", "t02_explicit", "t04_legacy", "t06_explicit" + }; + + private static final String[] CANONICAL_VIEW_IDS = {"t01_view", "t03_view", "t05_view"}; + + /** Case-normalization fixture; see {@code CASE_DB}. */ + private static final String CASE_DB = "entity_type_case_db"; + + private static final String[] CASE_VISIBLE_TABLE_IDS = { + "case00_null", "case01_upper_table", "case02_lower_table", "case03_mixed_table" + }; + + private static final String CASE_GARBAGE_ID = "case07_garbage"; + @Autowired UserTableHtsJdbcRepository htsRepository; @AfterEach @@ -32,6 +64,56 @@ public void tearDown() { htsRepository.deleteAll(); } + private UserTableRow row(String databaseId, String tableId, String entityType) { + return UserTableRow.builder() + .databaseId(databaseId) + .tableId(tableId) + .version(null) + .metadataLocation(String.format("/openhouse/%s/%s/v0_metadata.json", databaseId, tableId)) + .storageType(TEST_DEFAULT_STORAGE_TYPE) + .creationTime(TEST_CREATION_TIME) + .entityType(entityType) + .build(); + } + + /** Seeds the canonical 7-row interleaved fixture into {@code databaseId} under {@code prefix}. */ + private void seedCanonicalRows(String databaseId, String prefix) { + htsRepository.save(row(databaseId, prefix + "t00_legacy", null)); + htsRepository.save(row(databaseId, prefix + "t01_view", "VIEW")); + htsRepository.save(row(databaseId, prefix + "t02_explicit", "TABLE")); + htsRepository.save(row(databaseId, prefix + "t03_view", "VIEW")); + htsRepository.save(row(databaseId, prefix + "t04_legacy", null)); + htsRepository.save(row(databaseId, prefix + "t05_view", "VIEW")); + htsRepository.save(row(databaseId, prefix + "t06_explicit", "TABLE")); + } + + /** Seeds the 8-row case-normalization fixture into {@link #CASE_DB}. */ + private void seedCaseNormalizationRows() { + htsRepository.save(row(CASE_DB, "case00_null", null)); + htsRepository.save(row(CASE_DB, "case01_upper_table", "TABLE")); + htsRepository.save(row(CASE_DB, "case02_lower_table", "table")); + htsRepository.save(row(CASE_DB, "case03_mixed_table", "TaBlE")); + htsRepository.save(row(CASE_DB, "case04_upper_view", "VIEW")); + htsRepository.save(row(CASE_DB, "case05_lower_view", "view")); + htsRepository.save(row(CASE_DB, "case06_mixed_view", "ViEw")); + htsRepository.save(row(CASE_DB, CASE_GARBAGE_ID, "UNKNOWN")); + } + + private static List tableIds(Iterable rows) { + return Lists.newArrayList(rows).stream() + .map(UserTableRow::getTableId) + .sorted() + .collect(Collectors.toList()); + } + + private static List pageTableIds(Page page) { + return page.getContent().stream().map(UserTableRow::getTableId).collect(Collectors.toList()); + } + + private static Pageable sortedPage(int page) { + return PageRequest.of(page, 2, Sort.by("tableId")); + } + @Test public void testSaveFirstRecord() { UserTableRow testUserTableRow = @@ -57,7 +139,8 @@ public void testFindAllByDatabaseId() { htsRepository.save(TEST_TUPLE_1_1.get_userTableRow()); htsRepository.save(TEST_TUPLE_2_0.get_userTableRow()); List result = - Lists.newArrayList(htsRepository.findAllByDatabaseIdIgnoreCase("test_db0")); + Lists.newArrayList( + htsRepository.findAllTablesByFilters("test_db0", null, null, null, null, null)); Assertions.assertEquals( Lists.newArrayList("test_table1", "test_table2"), result.stream().map(UserTableRow::getTableId).collect(Collectors.toList())); @@ -70,7 +153,7 @@ public void testFindAllByTableIdPattern() { htsRepository.save(TEST_TUPLE_2_0.get_userTableRow()); List result = Lists.newArrayList( - htsRepository.findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( "test_db0", "test_table%")); Assertions.assertEquals( Lists.newArrayList("test_table1", "test_table2"), @@ -84,7 +167,7 @@ public void testFindAllByTableId() { htsRepository.save(TEST_TUPLE_2_0.get_userTableRow()); List result = Lists.newArrayList( - htsRepository.findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( "test_db0", "test_table1")); Assertions.assertEquals( Lists.newArrayList("test_table1"), @@ -246,4 +329,347 @@ public void testRenameCaseSensitivity() { // verify testTuple1_1 doesn't exist any more. assertThat(htsRepository.existsById(key)).isFalse(); } + + // --------------------------------------------------------------------------------------------- + // entityType discriminator + // --------------------------------------------------------------------------------------------- + + /** The discriminator must persist verbatim and must not perturb version/metadata behavior. */ + @Test + public void testEntityTypePersistenceRoundTrip() { + UserTableRow viewRow = htsRepository.save(row(ENTITY_TYPE_DB, "persist_view", "VIEW")); + UserTableRow tableRow = htsRepository.save(row(ENTITY_TYPE_DB, "persist_table", "TABLE")); + UserTableRow legacyRow = htsRepository.save(row(ENTITY_TYPE_DB, "persist_legacy", null)); + + // Insert still yields version 0 for all three; the discriminator is orthogonal to versioning. + assertThat(viewRow.getVersion()).isEqualTo(0L); + assertThat(tableRow.getVersion()).isEqualTo(0L); + assertThat(legacyRow.getVersion()).isEqualTo(0L); + + assertThat(findRow(ENTITY_TYPE_DB, "persist_view").getEntityType()).isEqualTo("VIEW"); + assertThat(findRow(ENTITY_TYPE_DB, "persist_table").getEntityType()).isEqualTo("TABLE"); + assertThat(findRow(ENTITY_TYPE_DB, "persist_legacy").getEntityType()).isNull(); + + assertThat(findRow(ENTITY_TYPE_DB, "persist_view").getMetadataLocation()) + .isEqualTo(viewRow.getMetadataLocation()); + + // An update at the correct version preserves the stored discriminator. + UserTableRow updated = + htsRepository.save( + findRow(ENTITY_TYPE_DB, "persist_view") + .toBuilder() + .metadataLocation("/openhouse/entity_type_db/persist_view/v1_metadata.json") + .build()); + assertThat(updated.getEntityType()).isEqualTo("VIEW"); + assertThat(findRow(ENTITY_TYPE_DB, "persist_view").getEntityType()).isEqualTo("VIEW"); + } + + /** SHOW-TABLES-equivalent plain listing hides views and keeps legacy NULL rows. */ + @Test + public void testFindAllByDatabaseIdFiltersViewsAndKeepsLegacyTables() { + seedCanonicalRows(ENTITY_TYPE_DB, ""); + // A table in another database must not leak in. + htsRepository.save(row("other_db", "t00_legacy", null)); + + List result = + Lists.newArrayList( + htsRepository.findAllTablesByFilters(ENTITY_TYPE_DB, null, null, null, null, null)); + + assertThat(tableIds(result)).containsExactly(CANONICAL_TABLE_IDS); + assertThat(result) + .allSatisfy(r -> assertThat(r.getEntityType()).isNotEqualToIgnoringCase("VIEW")); + } + + /** + * The canonical anti-post-filter assertion for the per-database page. A fetch-then-filter + * implementation returns [t00_legacy] on page 0 with totalElements=7/totalPages=4; the correct + * pre-pagination predicate returns a full 2-row page with totalElements=4/totalPages=2. + */ + @Test + public void testFindAllByDatabaseIdFiltersBeforePagination() { + seedCanonicalRows(ENTITY_TYPE_DB, ""); + + Page page0 = + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(0)); + assertThat(page0.getTotalElements()).isEqualTo(4); + assertThat(page0.getTotalPages()).isEqualTo(2); + assertThat(page0.getContent()).hasSize(2); + assertThat(pageTableIds(page0)).containsExactly("t00_legacy", "t02_explicit"); + + Page page1 = + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(1)); + assertThat(page1.getTotalElements()).isEqualTo(4); + assertThat(page1.getTotalPages()).isEqualTo(2); + assertThat(page1.getContent()).hasSize(2); + assertThat(pageTableIds(page1)).containsExactly("t04_legacy", "t06_explicit"); + + assertThat(pageTableIds(page0)).doesNotContainAnyElementsOf(Arrays.asList(CANONICAL_VIEW_IDS)); + assertThat(pageTableIds(page1)).doesNotContainAnyElementsOf(Arrays.asList(CANONICAL_VIEW_IDS)); + } + + /** The pattern (LIKE) listing family applies the same table-only predicate. */ + @Test + public void testFindAllByPatternFiltersViewsAndKeepsLegacyTables() { + seedCanonicalRows(ENTITY_TYPE_DB, "match_"); + // Non-matching table in the same database must be excluded by the pattern, not by type. + htsRepository.save(row(ENTITY_TYPE_DB, "nomatch_table", "TABLE")); + + List result = + Lists.newArrayList( + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%")); + + assertThat(tableIds(result)) + .containsExactly( + "match_t00_legacy", "match_t02_explicit", "match_t04_legacy", "match_t06_explicit"); + } + + /** Anti-post-filter assertion for the paged pattern listing. */ + @Test + public void testFindAllByPatternFiltersBeforePagination() { + seedCanonicalRows(ENTITY_TYPE_DB, "match_"); + htsRepository.save(row(ENTITY_TYPE_DB, "nomatch_table", "TABLE")); + + Page page0 = + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%", sortedPage(0)); + assertThat(page0.getTotalElements()).isEqualTo(4); + assertThat(page0.getTotalPages()).isEqualTo(2); + assertThat(page0.getContent()).hasSize(2); + assertThat(pageTableIds(page0)).containsExactly("match_t00_legacy", "match_t02_explicit"); + + Page page1 = + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%", sortedPage(1)); + assertThat(page1.getTotalElements()).isEqualTo(4); + assertThat(page1.getTotalPages()).isEqualTo(2); + assertThat(page1.getContent()).hasSize(2); + assertThat(pageTableIds(page1)).containsExactly("match_t04_legacy", "match_t06_explicit"); + } + + /** The general-filter query is table-scoped too: no overload can return a VIEW row. */ + @Test + public void testFindAllTablesByFiltersReturnsOnlyTables() { + seedCanonicalRows(ENTITY_TYPE_DB, ""); + + assertThat( + tableIds( + htsRepository.findAllTablesByFilters(ENTITY_TYPE_DB, null, null, null, null, null))) + .containsExactly(CANONICAL_TABLE_IDS); + + Page page0 = + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(0)); + assertThat(page0.getTotalElements()).isEqualTo(4); + assertThat(page0.getTotalPages()).isEqualTo(2); + assertThat(pageTableIds(page0)).containsExactly("t00_legacy", "t02_explicit"); + + Page page1 = + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(1)); + assertThat(page1.getTotalElements()).isEqualTo(4); + assertThat(pageTableIds(page1)).containsExactly("t04_legacy", "t06_explicit"); + + // A view is unreachable through this family, by tableId as well as by database. + assertThat( + Lists.newArrayList( + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, "t01_view", null, null, null, null))) + .isEmpty(); + } + + /** + * Case/garbage matrix at the SQL layer. + * + *

H2 runs in {@code MODE=MySQL} which is case-SENSITIVE for string comparison, whereas + * production MySQL's default collation is case-INSENSITIVE. This test therefore proves that the + * query normalizes explicitly (e.g. {@code upper(u.entityType) = 'TABLE'}) rather than leaning on + * a provider collation: an implementation using a bare {@code = 'TABLE'} comparison would hide + * {@code table}/{@code TaBlE} here and fail. It does NOT certify production MySQL behavior — a + * MySQL staging smoke test is still required before views are enabled. + */ + @Test + public void testEntityTypePredicatesAreCaseInsensitiveAndGarbageFailsClosed() { + seedCaseNormalizationRows(); + + assertThat( + tableIds(htsRepository.findAllTablesByFilters(CASE_DB, null, null, null, null, null))) + .containsExactly(CASE_VISIBLE_TABLE_IDS); + assertThat( + tableIds( + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + CASE_DB, "case%"))) + .containsExactly(CASE_VISIBLE_TABLE_IDS); + + Page dbPage0 = + htsRepository.findAllTablesByFilters(CASE_DB, null, null, null, null, null, sortedPage(0)); + assertThat(dbPage0.getTotalElements()).isEqualTo(4); + assertThat(dbPage0.getTotalPages()).isEqualTo(2); + assertThat(pageTableIds(dbPage0)).containsExactly("case00_null", "case01_upper_table"); + + Page patternPage0 = + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + CASE_DB, "case%", sortedPage(0)); + assertThat(patternPage0.getTotalElements()).isEqualTo(4); + assertThat(patternPage0.getTotalPages()).isEqualTo(2); + assertThat(pageTableIds(patternPage0)).containsExactly("case00_null", "case01_upper_table"); + + // The general filter family is table-scoped as well, so no view spelling leaks through it. + assertThat( + tableIds(htsRepository.findAllTablesByFilters(CASE_DB, null, null, null, null, null))) + .containsExactly(CASE_VISIBLE_TABLE_IDS); + + // Garbage fails closed everywhere: it is neither a table nor a view. + assertThat( + tableIds(htsRepository.findAllTablesByFilters(CASE_DB, null, null, null, null, null))) + .doesNotContain(CASE_GARBAGE_ID); + + // The garbage row is still stored — it is hidden, not dropped. + assertThat(findRow(CASE_DB, CASE_GARBAGE_ID).getEntityType()).isEqualTo("UNKNOWN"); + } + + /** + * Table-scoped point read: the query, not any caller, is what makes a view unreadable through the + * table path. NULL and every spelling of TABLE resolve; every spelling of VIEW and an + * unrecognized value resolve to empty. + */ + @ParameterizedTest + @CsvSource( + nullValues = "NULL", + value = { + "case00_null, true", + "case01_upper_table, true", + "case02_lower_table, true", + "case03_mixed_table, true", + "case04_upper_view, false", + "case05_lower_view, false", + "case06_mixed_view, false", + "case07_garbage, false" + }) + public void testFindTableByKeyResolvesOnlyTableRows(String tableId, boolean expectedVisible) { + seedCaseNormalizationRows(); + + assertThat( + htsRepository + .findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(CASE_DB, tableId) + .isPresent()) + .as("findTableBy... for %s", tableId) + .isEqualTo(expectedVisible); + + // Case-insensitive on the key itself, exactly like the neutral read. + assertThat( + htsRepository + .findTableByDatabaseIdIgnoreCaseAndTableIdIgnoreCase( + CASE_DB.toUpperCase(), tableId.toUpperCase()) + .isPresent()) + .isEqualTo(expectedVisible); + } + + /** + * The neutral read must keep seeing every row type. It backs {@code findById}/{@code existsById}, + * which the HTS writers use to detect a collision at a key held by another entity type; filtering + * it would make a view invisible to the very code that must refuse to overwrite it. + */ + @Test + public void testNeutralPointReadStillSeesEveryEntityType() { + seedCaseNormalizationRows(); + + for (String tableId : + new String[] { + "case00_null", + "case01_upper_table", + "case04_upper_view", + "case06_mixed_view", + CASE_GARBAGE_ID + }) { + assertThat( + htsRepository + .findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(CASE_DB, tableId) + .isPresent()) + .as("neutral read must still see %s", tableId) + .isTrue(); + assertThat(htsRepository.existsByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(CASE_DB, tableId)) + .as("neutral exists must still see %s", tableId) + .isTrue(); + } + } + + /** + * The type is chosen by which method you call, not by an argument. {@code findAllByFilters} is + * the general query and returns both types; its table-scoped sibling adds only the table + * predicate, which also matches a legacy stored null. + */ + @Test + public void testGeneralFiltersReturnBothTypesAndTableFiltersReturnOnlyTables() { + seedCanonicalRows(ENTITY_TYPE_DB, ""); + + List everything = new ArrayList<>(Arrays.asList(CANONICAL_TABLE_IDS)); + everything.addAll(Arrays.asList(CANONICAL_VIEW_IDS)); + Collections.sort(everything); + + assertThat( + tableIds(htsRepository.findAllByFilters(ENTITY_TYPE_DB, null, null, null, null, null))) + .as("the general query must return tables and views together") + .isEqualTo(everything); + + assertThat( + tableIds( + htsRepository.findAllTablesByFilters(ENTITY_TYPE_DB, null, null, null, null, null))) + .as("the table query must return tables and legacy nulls only") + .containsExactly(CANONICAL_TABLE_IDS); + + // A view is unreachable through the table family, by tableId as well as by database. + assertThat( + Lists.newArrayList( + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, "t01_view", null, null, null, null))) + .isEmpty(); + + // Paged overloads agree, counts included. + Page anyPage0 = + htsRepository.findAllByFilters(ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(0)); + assertThat(anyPage0.getTotalElements()).isEqualTo(7); + assertThat(pageTableIds(anyPage0)).containsExactly("t00_legacy", "t01_view"); + + Page tablePage0 = + htsRepository.findAllTablesByFilters( + ENTITY_TYPE_DB, null, null, null, null, null, sortedPage(0)); + assertThat(tablePage0.getTotalElements()).isEqualTo(4); + assertThat(tablePage0.getTotalPages()).isEqualTo(2); + assertThat(pageTableIds(tablePage0)).containsExactly("t00_legacy", "t02_explicit"); + } + + /** The pattern family splits the same way. */ + @Test + public void testGeneralPatternReturnsBothTypesAndTablePatternOnlyTables() { + seedCanonicalRows(ENTITY_TYPE_DB, "match_"); + + assertThat( + tableIds( + htsRepository.findAllByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%"))) + .hasSize(7); + + assertThat( + tableIds( + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%"))) + .containsExactly( + "match_t00_legacy", "match_t02_explicit", "match_t04_legacy", "match_t06_explicit"); + + Page tablePage0 = + htsRepository.findAllTablesByDatabaseIdAndTableIdLikeAllIgnoreCase( + ENTITY_TYPE_DB, "match_%", sortedPage(0)); + assertThat(tablePage0.getTotalElements()).isEqualTo(4); + assertThat(tablePage0.getTotalPages()).isEqualTo(2); + } + + private UserTableRow findRow(String databaseId, String tableId) { + return htsRepository + .findById(UserTableRowPrimaryKey.builder().databaseId(databaseId).tableId(tableId).build()) + .orElseThrow( + () -> new AssertionError("Expected row " + databaseId + "." + tableId + " to exist")); + } } diff --git a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/UserTablesServiceTest.java b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/UserTablesServiceTest.java index bf97095bd..98ea078cd 100644 --- a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/UserTablesServiceTest.java +++ b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/e2e/usertable/UserTablesServiceTest.java @@ -12,12 +12,14 @@ import com.linkedin.openhouse.housetables.e2e.SpringH2HtsApplication; import com.linkedin.openhouse.housetables.model.TestHouseTableModelConstants; import com.linkedin.openhouse.housetables.model.UserTableRow; +import com.linkedin.openhouse.housetables.model.UserTableRowPrimaryKey; import com.linkedin.openhouse.housetables.repository.impl.jdbc.SoftDeletedUserTableHtsJdbcRepository; import com.linkedin.openhouse.housetables.repository.impl.jdbc.UserTableHtsJdbcRepository; import com.linkedin.openhouse.housetables.services.UserTablesService; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -25,6 +27,9 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; @@ -637,4 +642,246 @@ private Boolean isUserTableDtoEqual(UserTableDto expected, UserTableDto actual) .build() .equals(actual.toBuilder().tableVersion("").build()); } + + // --------------------------------------------------------------------------------------------- + // entityType discriminator at the service call sites + // --------------------------------------------------------------------------------------------- + + /** + * Canonical interleaved fixture. Seeded into its own database so it never perturbs the + * pre-existing per-database counts asserted by the tests above. + */ + private static final String ENTITY_TYPE_DB = "entity_type_db"; + + private static final List CANONICAL_TABLE_IDS = + Arrays.asList("t00_legacy", "t02_explicit", "t04_legacy", "t06_explicit"); + + private static final List CANONICAL_VIEW_IDS = + Arrays.asList("t01_view", "t03_view", "t05_view"); + + /** + * {@code getUserTable} is the single HTS endpoint behind every table point read in the tables + * service, so filtering it here is what makes doRefresh, dropTable, the rename source and + * findTableRefById all treat a view as absent without a check of their own. + */ + @ParameterizedTest + @ValueSource(strings = {"VIEW", "view", "ViEw", "UNKNOWN"}) + public void testGetUserTableHidesNonTableRows(String entityType) { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "point_read", entityType)); + + Assertions.assertThrows( + NoSuchUserTableException.class, + () -> userTablesService.getUserTable(ENTITY_TYPE_DB, "point_read")); + + // Hidden from the table read, not deleted. + assertThat( + htsRepository + .findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(ENTITY_TYPE_DB, "point_read") + .isPresent()) + .isTrue(); + } + + @ParameterizedTest + @CsvSource( + nullValues = "NULL", + value = {"NULL", "TABLE", "table", "TaBlE"}) + public void testGetUserTableResolvesNullAndTableRows(String entityType) { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "point_read", entityType)); + + UserTableDto dto = userTablesService.getUserTable(ENTITY_TYPE_DB, "point_read"); + assertThat(dto.getTableId()).isEqualTo("point_read"); + assertThat(dto.getEntityType()).isEqualTo(entityType); + } + + /** + * The writers must still see a view at a shared key, otherwise a table create or delete would + * silently act on a name another entity already holds. + */ + @Test + public void testWritersStillSeeNonTableRowsAtTheSameKey() { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "shared_key", "VIEW")); + + UserTableRow seenByWriter = + htsRepository + .findById( + UserTableRowPrimaryKey.builder() + .databaseId(ENTITY_TYPE_DB) + .tableId("shared_key") + .build()) + .orElseThrow(() -> new AssertionError("writer read must see the view row")); + assertThat(seenByWriter.getEntityType()).isEqualTo("VIEW"); + + // deleteUserTable resolves through the same neutral read, so it can still remove the row. + userTablesService.deleteUserTable(ENTITY_TYPE_DB, "shared_key", false); + assertThat( + htsRepository + .findByDatabaseIdIgnoreCaseAndTableIdIgnoreCase(ENTITY_TYPE_DB, "shared_key") + .isPresent()) + .isFalse(); + } + + private UserTableRow entityTypeRow(String databaseId, String tableId, String entityType) { + return UserTableRow.builder() + .databaseId(databaseId) + .tableId(tableId) + .version(null) + .metadataLocation(String.format("/openhouse/%s/%s/v0_metadata.json", databaseId, tableId)) + .storageType(TEST_DEFAULT_STORAGE_TYPE) + .creationTime(TEST_CREATION_TIME) + .entityType(entityType) + .build(); + } + + private void seedCanonicalRows(String prefix) { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t00_legacy", null)); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t01_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t02_explicit", "TABLE")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t03_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t04_legacy", null)); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t05_view", "VIEW")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, prefix + "t06_explicit", "TABLE")); + } + + private static List sortedIds(List dtos) { + return dtos.stream().map(UserTableDto::getTableId).sorted().collect(Collectors.toList()); + } + + private static List pageIds(Page page) { + return page.getContent().stream().map(UserTableDto::getTableId).collect(Collectors.toList()); + } + + /** Plain per-database listing through the service hides views and keeps legacy NULL rows. */ + @Test + public void testListTablesCallSiteFiltersViewsAndKeepsNullRows() { + seedCanonicalRows(""); + + List result = + userTablesService.getAllUserTables(UserTable.builder().databaseId(ENTITY_TYPE_DB).build()); + + assertThat(sortedIds(result)).isEqualTo(CANONICAL_TABLE_IDS); + } + + /** + * Anti-post-filter assertion at the service layer: a fetch-then-filter implementation yields a + * 1-row page 0 with totalElements=7/totalPages=4. + */ + @Test + public void testListTablesCallSiteFiltersBeforePagination() { + seedCanonicalRows(""); + UserTable searchBy = UserTable.builder().databaseId(ENTITY_TYPE_DB).build(); + + Page page0 = userTablesService.getAllUserTables(searchBy, 0, 2, "tableId"); + Assertions.assertEquals(4, page0.getTotalElements()); + Assertions.assertEquals(2, page0.getTotalPages()); + Assertions.assertEquals(2, page0.getContent().size()); + assertThat(pageIds(page0)).containsExactly("t00_legacy", "t02_explicit"); + + Page page1 = userTablesService.getAllUserTables(searchBy, 1, 2, "tableId"); + Assertions.assertEquals(4, page1.getTotalElements()); + Assertions.assertEquals(2, page1.getTotalPages()); + Assertions.assertEquals(2, page1.getContent().size()); + assertThat(pageIds(page1)).containsExactly("t04_legacy", "t06_explicit"); + + assertThat(pageIds(page0)).doesNotContainAnyElementsOf(CANONICAL_VIEW_IDS); + assertThat(pageIds(page1)).doesNotContainAnyElementsOf(CANONICAL_VIEW_IDS); + } + + /** The pattern-listing call sites (plain and paged) apply the same predicate. */ + @Test + public void testPatternCallSitesFilterViewsPlainAndPaged() { + seedCanonicalRows("match_"); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "nomatch_table", "TABLE")); + UserTable searchBy = UserTable.builder().databaseId(ENTITY_TYPE_DB).tableId("match_%").build(); + + assertThat(sortedIds(userTablesService.getAllUserTables(searchBy))) + .containsExactly( + "match_t00_legacy", "match_t02_explicit", "match_t04_legacy", "match_t06_explicit"); + + Page page0 = userTablesService.getAllUserTables(searchBy, 0, 2, "tableId"); + Assertions.assertEquals(4, page0.getTotalElements()); + Assertions.assertEquals(2, page0.getTotalPages()); + assertThat(pageIds(page0)).containsExactly("match_t00_legacy", "match_t02_explicit"); + + Page page1 = userTablesService.getAllUserTables(searchBy, 1, 2, "tableId"); + Assertions.assertEquals(4, page1.getTotalElements()); + Assertions.assertEquals(2, page1.getTotalPages()); + assertThat(pageIds(page1)).containsExactly("match_t04_legacy", "match_t06_explicit"); + } + + /** + * The query endpoint is table-scoped by path, so {@code entityType} is bound onto the request but + * never reaches a predicate: an {@code entityType=VIEW} request is answered with tables, and + * routing is unaffected by the field. + */ + @Test + public void testEntityTypeOnQueryIsIgnoredAndAlwaysReturnsTables() { + seedCanonicalRows(""); + + for (String entityType : new String[] {"VIEW", "view", "TABLE", "TaBlE", null}) { + assertThat( + sortedIds( + userTablesService.getAllUserTables( + UserTable.builder() + .databaseId(ENTITY_TYPE_DB) + .entityType(entityType) + .build()))) + .as("entityType=%s must still resolve to the four visible tables", entityType) + .isEqualTo(CANONICAL_TABLE_IDS); + } + + Page page0 = + userTablesService.getAllUserTables( + UserTable.builder().databaseId(ENTITY_TYPE_DB).entityType("VIEW").build(), + 0, + 2, + "tableId"); + Assertions.assertEquals(4, page0.getTotalElements()); + Assertions.assertEquals(2, page0.getTotalPages()); + assertThat(pageIds(page0)).containsExactly("t00_legacy", "t02_explicit"); + } + + /** + * Defense in depth for the shared key space: if a rename ever reaches the HTS storage layer with + * an occupied destination, the primary-key violation must roll back cleanly and leave BOTH JPA + * rows byte-identical — same numeric {@code version}, {@code metadataLocation} and {@code + * entityType}. The table-service tests prove correct code never reaches this fallback; this pins + * that the fallback itself is non-mutating. + */ + @Test + public void testRenameCollisionLeavesJPARowsUnchanged() { + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "rename_src_table", "TABLE")); + htsRepository.save(entityTypeRow(ENTITY_TYPE_DB, "rename_dst_view", "VIEW")); + + UserTableRow sourceBefore = findRow(ENTITY_TYPE_DB, "rename_src_table"); + UserTableRow destinationBefore = findRow(ENTITY_TYPE_DB, "rename_dst_view"); + + Assertions.assertThrows( + AlreadyExistsException.class, + () -> + userTablesService.renameUserTable( + ENTITY_TYPE_DB, + "rename_src_table", + ENTITY_TYPE_DB, + "rename_dst_view", + "/openhouse/entity_type_db/rename_dst_view/v1_metadata.json")); + + UserTableRow sourceAfter = findRow(ENTITY_TYPE_DB, "rename_src_table"); + UserTableRow destinationAfter = findRow(ENTITY_TYPE_DB, "rename_dst_view"); + + Assertions.assertEquals(sourceBefore.getVersion(), sourceAfter.getVersion()); + Assertions.assertEquals(sourceBefore.getMetadataLocation(), sourceAfter.getMetadataLocation()); + Assertions.assertEquals(sourceBefore.getEntityType(), sourceAfter.getEntityType()); + + Assertions.assertEquals(destinationBefore.getVersion(), destinationAfter.getVersion()); + Assertions.assertEquals( + destinationBefore.getMetadataLocation(), destinationAfter.getMetadataLocation()); + Assertions.assertEquals("VIEW", destinationAfter.getEntityType()); + } + + private UserTableRow findRow(String databaseId, String tableId) { + return htsRepository + .findById(UserTableRowPrimaryKey.builder().databaseId(databaseId).tableId(tableId).build()) + .orElseThrow( + () -> new AssertionError("Expected row " + databaseId + "." + tableId + " to exist")); + } } diff --git a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/api/OpenHouseUserTablesValidatorTest.java b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/api/OpenHouseUserTablesValidatorTest.java index 53cfb3d95..8d038d2b8 100644 --- a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/api/OpenHouseUserTablesValidatorTest.java +++ b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/api/OpenHouseUserTablesValidatorTest.java @@ -147,4 +147,49 @@ public void validateRenameEntityInvalidInput() { RequestValidationFailureException.class, () -> userTablesHtsApiValidator.validateRenameEntity(fromKey, toKey)); } + + /** + * Load-bearing: the PUT path relies on Bean Validation on the transport model, so the pattern on + * {@code UserTable#entityType} must accept every TABLE/VIEW spelling and reject anything else. + */ + @Test + public void validatePutEntityTypeCaseInsensitivelyAndRejectsGarbage() { + for (String entityType : new String[] {"VIEW", "view", "ViEw", "TABLE", "table", "TaBlE"}) { + UserTable userTable = + UserTable.builder() + .tableId("tb1") + .databaseId("db1") + .tableVersion("/tmp/test/opt/metadata.json") + .metadataLocation("INITIAL_VERSION") + .entityType(entityType) + .build(); + + assertDoesNotThrow( + () -> userTablesHtsApiValidator.validatePutEntity(userTable), + "PUT with entityType=" + entityType + " should validate"); + } + + // Omitting the field entirely stays valid (legacy table writers). + assertDoesNotThrow( + () -> + userTablesHtsApiValidator.validatePutEntity( + UserTable.builder() + .tableId("tb1") + .databaseId("db1") + .tableVersion("/tmp/test/opt/metadata.json") + .metadataLocation("INITIAL_VERSION") + .build())); + + UserTable garbage = + UserTable.builder() + .tableId("tb1") + .databaseId("db1") + .tableVersion("/tmp/test/opt/metadata.json") + .metadataLocation("INITIAL_VERSION") + .entityType("UNKNOWN") + .build(); + assertThrows( + RequestValidationFailureException.class, + () -> userTablesHtsApiValidator.validatePutEntity(garbage)); + } } diff --git a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/mapper/UserTablesMapperTest.java b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/mapper/UserTablesMapperTest.java index 42103148b..f1b54ebef 100644 --- a/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/mapper/UserTablesMapperTest.java +++ b/services/housetables/src/test/java/com/linkedin/openhouse/housetables/mock/mapper/UserTablesMapperTest.java @@ -1,9 +1,12 @@ package com.linkedin.openhouse.housetables.mock.mapper; +import com.linkedin.openhouse.housetables.api.spec.model.UserTable; import com.linkedin.openhouse.housetables.dto.mapper.UserTablesMapper; import com.linkedin.openhouse.housetables.dto.model.UserTableDto; import com.linkedin.openhouse.housetables.model.TestHouseTableModelConstants; import com.linkedin.openhouse.housetables.model.UserTableRow; +import java.util.HashMap; +import java.util.Map; import java.util.Optional; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -64,4 +67,78 @@ void fromUserTable() { TestHouseTableModelConstants.TEST_USER_TABLE_DTO, userTablesMapper.fromUserTable(TestHouseTableModelConstants.TEST_USER_TABLE)); } + + /** + * The entityType discriminator must survive every hop of the HTS mapping chain (API -> JPA row -> + * DTO -> API). A mapper that silently drops it would let a view pointer be persisted as a legacy + * table row. + */ + @Test + void entityTypeRoundTripsAcrossUserTableRowDtoAndApi() { + UserTable viewUserTable = + TestHouseTableModelConstants.TEST_USER_TABLE.toBuilder().entityType("VIEW").build(); + + UserTableRow row = userTablesMapper.toUserTableRow(viewUserTable, Optional.empty()); + Assertions.assertEquals("VIEW", row.getEntityType()); + + UserTableDto dto = userTablesMapper.toUserTableDto(row); + Assertions.assertEquals("VIEW", dto.getEntityType()); + + UserTable roundTripped = userTablesMapper.toUserTable(dto); + Assertions.assertEquals("VIEW", roundTripped.getEntityType()); + + // Every other field is untouched by the new discriminator. + Assertions.assertEquals(viewUserTable.getTableId(), roundTripped.getTableId()); + Assertions.assertEquals(viewUserTable.getDatabaseId(), roundTripped.getDatabaseId()); + Assertions.assertEquals( + viewUserTable.getMetadataLocation(), roundTripped.getMetadataLocation()); + Assertions.assertEquals(viewUserTable.getStorageType(), roundTripped.getStorageType()); + Assertions.assertEquals(viewUserTable.getCreationTime(), roundTripped.getCreationTime()); + + // fromUserTable is the other API -> DTO direction and must carry it too. + Assertions.assertEquals("VIEW", userTablesMapper.fromUserTable(viewUserTable).getEntityType()); + } + + /** + * Backward compatibility: legacy writers omit the field entirely. No layer may default it to + * "TABLE", because that would start stamping a value on every existing table write and mask the + * null-means-table compatibility contract. + */ + @Test + void nullEntityTypeRemainsNullAcrossLegacyMappings() { + UserTable legacyUserTable = TestHouseTableModelConstants.TEST_USER_TABLE; + Assertions.assertNull(legacyUserTable.getEntityType()); + + UserTableRow row = userTablesMapper.toUserTableRow(legacyUserTable, Optional.empty()); + Assertions.assertNull(row.getEntityType()); + + UserTableDto dto = userTablesMapper.toUserTableDto(row); + Assertions.assertNull(dto.getEntityType()); + + Assertions.assertNull(userTablesMapper.toUserTable(dto).getEntityType()); + Assertions.assertNull(userTablesMapper.fromUserTable(legacyUserTable).getEntityType()); + + // The legacy fixture row (built without the field) must still map cleanly. + UserTableRow legacyRow = new TestHouseTableModelConstants.TestTuple(0).get_userTableRow(); + Assertions.assertNull(legacyRow.getEntityType()); + Assertions.assertNull(userTablesMapper.toUserTableDto(legacyRow).getEntityType()); + } + + /** + * {@code mapToUserTable} still binds an {@code entityType} request parameter onto the model, but + * the query endpoint is table-scoped by path so nothing consumes it. This pins where the value + * stops: bound here, never reaching a predicate. + */ + @Test + void mapToUserTableBindsButDoesNotConsumeEntityType() { + Map parameters = new HashMap<>(); + parameters.put("databaseId", "test_db0"); + parameters.put("entityType", "VIEW"); + + UserTable mapped = userTablesMapper.mapToUserTable(parameters); + + Assertions.assertEquals("test_db0", mapped.getDatabaseId()); + Assertions.assertEquals("VIEW", mapped.getEntityType()); + Assertions.assertNull(mapped.getTableId()); + } } 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 index 4ace198dd..ddcbf91bd 100644 --- 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 @@ -1236,6 +1236,11 @@ public void testRenameTablePreserveExistingCase() { Assertions.assertEquals( renamedTable.get().getTableProperties().get("openhouse.tableUri"), "local-cluster.d1.t1_renamed"); + + // Leaving d1.t1_renamed behind would leak into other tests in this class, which share one + // Spring context. + openHouseInternalRepository.deleteById( + TableDtoPrimaryKey.builder().databaseId("d1").tableId("t1_renamed").build()); } @Test