diff --git a/metacat-main/src/main/java/com/netflix/metacat/main/api/RequestWrapper.java b/metacat-main/src/main/java/com/netflix/metacat/main/api/RequestWrapper.java index 55cee49ad..b0210cedb 100644 --- a/metacat-main/src/main/java/com/netflix/metacat/main/api/RequestWrapper.java +++ b/metacat-main/src/main/java/com/netflix/metacat/main/api/RequestWrapper.java @@ -38,8 +38,6 @@ import com.netflix.metacat.common.server.connectors.exception.TableAlreadyExistsException; import com.netflix.metacat.common.server.connectors.exception.TablePreconditionFailedException; import com.netflix.metacat.common.server.monitoring.Metrics; -import com.netflix.metacat.common.server.properties.Config; -import com.netflix.metacat.common.server.usermetadata.AliasService; import com.netflix.metacat.common.server.usermetadata.UserMetadataServiceException; import com.netflix.metacat.common.server.util.MetacatContextManager; import com.netflix.spectator.api.Id; @@ -68,8 +66,6 @@ @Component public final class RequestWrapper { private final Registry registry; - private final Config config; - private final AliasService aliasService; private final RequestGateway requestGateway; //Metrics @@ -82,18 +78,12 @@ public final class RequestWrapper { * Wrapper class for processing the request. * * @param registry registry - * @param config Config - * @param aliasService AliasService * @param requestGateway RequestGateway */ @Autowired public RequestWrapper(@NotNull @NonNull final Registry registry, - @NotNull @NonNull final Config config, - @NotNull @NonNull final AliasService aliasService, @NotNull @NonNull final RequestGateway requestGateway) { this.registry = registry; - this.config = config; - this.aliasService = aliasService; this.requestGateway = requestGateway; requestCounterId = registry.createId(Metrics.CounterRequestCount.getMetricName()); @@ -103,33 +93,19 @@ public RequestWrapper(@NotNull @NonNull final Registry registry, } /** - * Creates the qualified name, resolving a table alias to the table it points at. + * Creates the qualified name. * * @param nameSupplier supplier * @return name */ public QualifiedName qualifyName(final Supplier nameSupplier) { - final QualifiedName name = unresolvedQualifiedName(nameSupplier); - if (config.isTableAliasEnabled() && name.getType() == QualifiedName.Type.TABLE) { - return aliasService.getTableName(name); - } - return name; - } - - /** - * Creates the qualified name without resolving a table alias. Table endpoints use this so that the - * table service sees the name as it was requested, and can resolve or reject the alias itself. - * - * @param nameSupplier supplier - * @return name - */ - public QualifiedName unresolvedQualifiedName(final Supplier nameSupplier) { try { return nameSupplier.get(); } catch (Exception e) { log.error("Invalid qualified name", e); throw new MetacatBadRequestException(e.getMessage()); } + } /** diff --git a/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/MetacatController.java b/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/MetacatController.java index 3340f4c91..eb6387299 100644 --- a/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/MetacatController.java +++ b/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/MetacatController.java @@ -219,7 +219,7 @@ public TableDto createTable( @Parameter(description = "The table information", required = true) @Valid @RequestBody final TableDto table ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); if (MetacatServiceHelper.isIcebergTable(table)) { @@ -387,7 +387,7 @@ public TableDto deleteTable( @Parameter(description = "The name of the table", required = true) @PathVariable("table-name") final String tableName ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( @@ -642,7 +642,7 @@ public TableDto getTable( name = "includeMetadataLocationOnly", defaultValue = "false") final boolean includeMetadataLocationOnly ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( @@ -997,10 +997,10 @@ public void renameTable( @Parameter(description = "The name of the table", required = true) @RequestParam("newTableName") final String newTableName ) { - final QualifiedName oldName = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName oldName = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); - final QualifiedName newName = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName newName = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, newTableName) ); this.requestWrapper.processRequest( @@ -1208,7 +1208,7 @@ public TableDto updateTable( name = "shouldThrowExceptionOnMetadataSaveFailure", defaultValue = "false") final boolean shouldThrowExceptionOnMetadataSaveFailure ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( diff --git a/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/PartitionController.java b/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/PartitionController.java index c0d2efde3..fd9040f28 100644 --- a/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/PartitionController.java +++ b/metacat-main/src/main/java/com/netflix/metacat/main/api/v1/PartitionController.java @@ -117,7 +117,7 @@ public void deletePartitions( @Parameter(description = "partitionId of the partitions to be deleted from this table", required = true) @RequestBody final List partitionIds ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); this.requestWrapper.processRequest( @@ -179,7 +179,7 @@ public void deletePartitions( @Parameter(description = "partitionId of the partitions to be deleted from this table", required = true) @RequestBody final List partitionIds ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); this.requestWrapper.processRequest( @@ -251,7 +251,7 @@ public List getPartitions( @Parameter(description = "Whether to include user metadata information to the response") @RequestParam(name = "includeUserMetadata", defaultValue = "false") final boolean includeUserMetadata ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( @@ -327,7 +327,7 @@ public List getPartitions( @Parameter(description = "Whether to include user metadata information to the response") @RequestParam(name = "includeUserMetadata", defaultValue = "false") final boolean includeUserMetadata ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1123,7 +1123,7 @@ public PartitionsSaveResponseDto savePartitions( @Parameter(description = "Request containing the list of partitions", required = true) @RequestBody final PartitionsSaveRequestDto partitionsSaveRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1188,7 +1188,7 @@ public Integer getPartitionCount( @Parameter(description = "The name of the table", required = true) @PathVariable("table-name") final String tableName ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( @@ -1237,7 +1237,7 @@ public Integer getPartitionCount( @Parameter(description = "The name of the view", required = true) @PathVariable("view-name") final String viewName ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1258,7 +1258,7 @@ private List getPartitions( final boolean includeUserMetadata, @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); @@ -1291,7 +1291,7 @@ private List getPartitions( final boolean includeUserMetadata, @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1321,7 +1321,7 @@ private List _getPartitionUris( @Nullable final Integer limit, @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( @@ -1352,7 +1352,7 @@ private List _getMViewPartitionKeys( @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1382,7 +1382,7 @@ private List _getMViewPartitionUris( @Nullable final Integer limit, @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofView(catalogName, databaseName, tableName, viewName) ); return this.requestWrapper.processRequest( @@ -1412,7 +1412,7 @@ private List _getPartitionKeys( @Nullable final GetPartitionsRequestDto getPartitionsRequestDto ) { - final QualifiedName name = this.requestWrapper.unresolvedQualifiedName( + final QualifiedName name = this.requestWrapper.qualifyName( () -> QualifiedName.ofTable(catalogName, databaseName, tableName) ); return this.requestWrapper.processRequest( diff --git a/metacat-main/src/test/groovy/com/netflix/metacat/main/api/RequestWrapperSpec.groovy b/metacat-main/src/test/groovy/com/netflix/metacat/main/api/RequestWrapperSpec.groovy index df4efa1b2..72872ccae 100644 --- a/metacat-main/src/test/groovy/com/netflix/metacat/main/api/RequestWrapperSpec.groovy +++ b/metacat-main/src/test/groovy/com/netflix/metacat/main/api/RequestWrapperSpec.groovy @@ -21,7 +21,6 @@ import com.netflix.metacat.common.QualifiedName import com.netflix.metacat.common.exception.MetacatBadRequestException import com.netflix.metacat.common.server.api.traffic_control.RequestGateway import com.netflix.metacat.common.server.properties.Config -import com.netflix.metacat.common.server.usermetadata.AliasService import com.netflix.metacat.common.server.util.MetacatContextManager import com.netflix.spectator.api.* import spock.lang.Specification @@ -32,7 +31,6 @@ class RequestWrapperSpec extends Specification { def registry = new NoopRegistry() def config = Mock(Config) - def aliasService = Mock(AliasService) def requestGateway = Mock(RequestGateway) def counter = Mock(Counter) def supplier = Mock(Supplier) @@ -40,7 +38,7 @@ class RequestWrapperSpec extends Specification { def setup() { this.supplier.get() >> null - requestWrapper = new RequestWrapper(registry, config, aliasService, requestGateway) + requestWrapper = new RequestWrapper(registry, requestGateway) } def "gateway is invoked for each request"() { @@ -72,45 +70,6 @@ class RequestWrapperSpec extends Specification { MetacatContextManager.getContext().getRequestName() == "getTable" } - def "qualifyName resolves a table alias to its source table"() { - given: - def alias = QualifiedName.ofTable("c", "d", "the_alias") - def source = QualifiedName.ofTable("c", "d", "the_source") - - when: - def result = requestWrapper.qualifyName({ alias }) - - then: - 1 * config.isTableAliasEnabled() >> true - 1 * aliasService.getTableName(alias) >> source - result == source - } - - def "qualifyName does not consult the alias service when aliasing is disabled"() { - given: - def alias = QualifiedName.ofTable("c", "d", "the_alias") - - when: - def result = requestWrapper.qualifyName({ alias }) - - then: - 1 * config.isTableAliasEnabled() >> false - 0 * aliasService.getTableName(_) - result == alias - } - - def "qualifyName only resolves aliases for table names"() { - given: - def dbName = QualifiedName.ofDatabase("c", "d") - - when: - def result = requestWrapper.qualifyName({ dbName }) - - then: - 0 * aliasService.getTableName(_) - result == dbName - } - def "qualifyName converts a failing supplier into a bad request"() { when: requestWrapper.qualifyName({ throw new IllegalArgumentException("bad name") }) @@ -118,25 +77,4 @@ class RequestWrapperSpec extends Specification { then: thrown(MetacatBadRequestException) } - - def "unresolvedQualifiedName leaves an alias alone, so the table service can decide what to do with it"() { - given: - def alias = QualifiedName.ofTable("c", "d", "the_alias") - - when: - def result = requestWrapper.unresolvedQualifiedName({ alias }) - - then: - 0 * aliasService.getTableName(_) - 0 * aliasService.isAlias(_) - result == alias - } - - def "unresolvedQualifiedName converts a failing supplier into a bad request"() { - when: - requestWrapper.unresolvedQualifiedName({ throw new IllegalArgumentException("bad name") }) - - then: - thrown(MetacatBadRequestException) - } } diff --git a/metacat-main/src/test/groovy/com/netflix/metacat/main/api/v1/MetacatControllerAliasSpec.groovy b/metacat-main/src/test/groovy/com/netflix/metacat/main/api/v1/MetacatControllerAliasSpec.groovy index a78b0da39..5ced88ebf 100644 --- a/metacat-main/src/test/groovy/com/netflix/metacat/main/api/v1/MetacatControllerAliasSpec.groovy +++ b/metacat-main/src/test/groovy/com/netflix/metacat/main/api/v1/MetacatControllerAliasSpec.groovy @@ -40,7 +40,7 @@ class MetacatControllerAliasSpec extends Specification { def aliasService = Mock(AliasService) def requestGateway = Mock(RequestGateway) def tableService = Mock(TableService) - def requestWrapper = new RequestWrapper(new NoopRegistry(), config, aliasService, requestGateway) + def requestWrapper = new RequestWrapper(new NoopRegistry(), requestGateway) def controller = new MetacatController( Mock(CatalogService), Mock(DatabaseService), Mock(MViewService), tableService, requestWrapper, config)