diff --git a/metacat-common/src/main/java/com/netflix/metacat/common/dto/TableDto.java b/metacat-common/src/main/java/com/netflix/metacat/common/dto/TableDto.java index c00d3d79f..776911ffd 100644 --- a/metacat-common/src/main/java/com/netflix/metacat/common/dto/TableDto.java +++ b/metacat-common/src/main/java/com/netflix/metacat/common/dto/TableDto.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; import com.netflix.metacat.common.QualifiedName; import io.swagger.annotations.ApiModel; @@ -37,6 +38,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Optional; /** * Table DTO. @@ -89,6 +91,14 @@ public QualifiedName getDefinitionName() { return name; } + @JsonIgnore + public Optional getTableOwner() { + return Optional.ofNullable(definitionMetadata) + .map(definitionMetadataJson -> definitionMetadataJson.get("owner")) + .map(ownerJson -> ownerJson.get("userId")) + .map(JsonNode::textValue); + } + /** * Returns the list of partition keys. * @return list of partition keys diff --git a/metacat-main/src/main/java/com/netflix/metacat/main/configs/ServicesConfig.java b/metacat-main/src/main/java/com/netflix/metacat/main/configs/ServicesConfig.java index 40a2d0e39..42aa13106 100644 --- a/metacat-main/src/main/java/com/netflix/metacat/main/configs/ServicesConfig.java +++ b/metacat-main/src/main/java/com/netflix/metacat/main/configs/ServicesConfig.java @@ -17,6 +17,7 @@ */ package com.netflix.metacat.main.configs; +import com.netflix.metacat.common.json.MetacatJson; import com.netflix.metacat.common.server.api.ratelimiter.DefaultRateLimiter; import com.netflix.metacat.common.server.api.ratelimiter.RateLimiter; import com.netflix.metacat.common.server.converter.ConverterUtil; @@ -191,6 +192,7 @@ public DatabaseService databaseService( * @param databaseService database service * @param tagService tag service * @param userMetadataService user metadata service + * @param metacatJson metacat json utility * @param eventBus Internal event bus * @param registry registry handle * @param config configurations @@ -205,6 +207,7 @@ public TableService tableService( final DatabaseService databaseService, final TagService tagService, final UserMetadataService userMetadataService, + final MetacatJson metacatJson, final MetacatEventBus eventBus, final Registry registry, final Config config, @@ -217,6 +220,7 @@ public TableService tableService( databaseService, tagService, userMetadataService, + metacatJson, eventBus, registry, config, diff --git a/metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java b/metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java index e3df4edaf..a4bc61ae5 100644 --- a/metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java +++ b/metacat-main/src/main/java/com/netflix/metacat/main/services/impl/TableServiceImpl.java @@ -13,7 +13,6 @@ package com.netflix.metacat.main.services.impl; -import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -27,6 +26,7 @@ import com.netflix.metacat.common.dto.TableDto; import com.netflix.metacat.common.exception.MetacatBadRequestException; import com.netflix.metacat.common.exception.MetacatNotSupportedException; +import com.netflix.metacat.common.json.MetacatJson; import com.netflix.metacat.common.server.connectors.exception.NotFoundException; import com.netflix.metacat.common.server.connectors.exception.TableMigrationInProgressException; import com.netflix.metacat.common.server.connectors.exception.TableNotFoundException; @@ -59,6 +59,7 @@ import com.netflix.metacat.main.services.TableService; import com.netflix.spectator.api.Registry; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang.StringUtils; import org.springframework.web.context.request.RequestAttributes; @@ -80,56 +81,20 @@ * Table service implementation. */ @Slf4j +@RequiredArgsConstructor public class TableServiceImpl implements TableService { private final ConnectorManager connectorManager; + private final ConnectorTableServiceProxy connectorTableServiceProxy; private final DatabaseService databaseService; private final TagService tagService; private final UserMetadataService userMetadataService; + private final MetacatJson metacatJson; private final MetacatEventBus eventBus; private final Registry registry; private final Config config; private final ConverterUtil converterUtil; - private final ConnectorTableServiceProxy connectorTableServiceProxy; private final AuthorizationService authorizationService; - /** - * Constructor. - * - * @param connectorManager Connector manager to use - * @param connectorTableServiceProxy connector table service proxy - * @param databaseService database service - * @param tagService tag service - * @param userMetadataService user metadata service - * @param eventBus Internal event bus - * @param registry registry handle - * @param config configurations - * @param converterUtil utility to convert to/from Dto to connector resources - * @param authorizationService authorization service - */ - public TableServiceImpl( - final ConnectorManager connectorManager, - final ConnectorTableServiceProxy connectorTableServiceProxy, - final DatabaseService databaseService, - final TagService tagService, - final UserMetadataService userMetadataService, - final MetacatEventBus eventBus, - final Registry registry, - final Config config, - final ConverterUtil converterUtil, - final AuthorizationService authorizationService - ) { - this.connectorManager = connectorManager; - this.connectorTableServiceProxy = connectorTableServiceProxy; - this.databaseService = databaseService; - this.tagService = tagService; - this.userMetadataService = userMetadataService; - this.eventBus = eventBus; - this.registry = registry; - this.config = config; - this.authorizationService = authorizationService; - this.converterUtil = converterUtil; - } - /** * {@inheritDoc} */ @@ -139,10 +104,8 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) { validate(name); this.authorizationService.checkPermission(metacatRequestContext.getUserName(), tableDto.getName(), MetacatOperation.CREATE); - // - // Set the owner,if null, with the session user name. - // - setOwnerIfNull(tableDto, metacatRequestContext.getUserName()); + + setDefaultAttributes(tableDto); logOwnershipDiagnosticDetails(name, tableDto); log.info("Creating table {}", name); @@ -181,6 +144,12 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) { return dto; } + private void setDefaultAttributes(final TableDto tableDto) { + setDefaultSerdeIfNull(tableDto); + setDefaultDefinitionMetadataIfNull(tableDto); + setOwnerIfNull(tableDto); + } + /** * Logs diagnostic data for debugging invalid owners. * @@ -189,13 +158,9 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) { */ private void logOwnershipDiagnosticDetails(final QualifiedName name, final TableDto tableDto) { try { - final String tableOwner = Optional.ofNullable(tableDto.getDefinitionMetadata()) - .map(node -> node.get("owner")) - .map(owner -> owner.get("userId")) - .map(JsonNode::textValue) - .orElse(null); + final String tableOwner = tableDto.getTableOwner().orElse(null); - if (StringUtils.isBlank(tableOwner) || "metacat".equals(tableOwner) || "root".equals(tableOwner)) { + if (!isOwnerValid(tableOwner)) { registry.counter( "unauth.user.create.table", "catalog", name.getCatalogName(), @@ -241,24 +206,75 @@ private Map getHttpHeaders() { return requestHeaders; } - private void setOwnerIfNull(final TableDto tableDto, final String user) { - String owner = user; + private void setDefaultDefinitionMetadataIfNull(final TableDto tableDto) { + ObjectNode definitionMetadata = tableDto.getDefinitionMetadata(); + if (definitionMetadata == null) { + definitionMetadata = metacatJson.emptyObjectNode(); + tableDto.setDefinitionMetadata(definitionMetadata); + } + } + + private void setDefaultSerdeIfNull(final TableDto tableDto) { StorageDto serde = tableDto.getSerde(); if (serde == null) { serde = new StorageDto(); tableDto.setSerde(serde); } - final String serdeOwner = serde.getOwner(); - if (Strings.isNullOrEmpty(serdeOwner)) { - serde.setOwner(user); - } else { - owner = serdeOwner; + } + + /** + * Sets the owner of the table. The order of priority of selecting the owner is: + * 
+     *     1. Explicitly set in the table dto
+     *     2. Username from the request headers
+     *     3. Owner set in the serde
+     *
+ * + * @param tableDto the table DTO + */ + private void setOwnerIfNull(final TableDto tableDto) { + final String ownerInDto = tableDto.getTableOwner().orElse(null); + if (isOwnerValid(ownerInDto)) { + return; + } + + final String userInContext = MetacatContextManager.getContext().getUserName(); + if (isOwnerValid(userInContext)) { + updateTableOwner(tableDto, userInContext); + return; + } + + final String serdeOwner = tableDto.getSerde().getOwner(); + if (isOwnerValid(serdeOwner)) { + updateTableOwner(tableDto, serdeOwner); + } + + // At this point, if we still have not found a valid user, + // cycle through the list again and use the first non-null value + + if (ownerInDto != null) { + return; } - if (!Strings.isNullOrEmpty(owner)) { - userMetadataService.populateOwnerIfMissing(tableDto, owner); + + if (userInContext != null) { + updateTableOwner(tableDto, userInContext); + return; + } + + if (serdeOwner != null) { + updateTableOwner(tableDto, serdeOwner); } } + void updateTableOwner(final TableDto tableDto, final String userId) { + final ObjectNode ownerNode = tableDto.getDefinitionMetadata().with("owner"); + ownerNode.put("userId", userId); + } + + private boolean isOwnerValid(@Nullable final String userId) { + return StringUtils.isNotBlank(userId) && !"metacat".equals(userId) && !"root".equals(userId); + } + @SuppressFBWarnings private void tag(final QualifiedName name, final ObjectNode definitionMetadata) { final Set tags = MetacatUtils.getTableTags(definitionMetadata); diff --git a/metacat-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy b/metacat-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy index 8b27ddd4f..043350acf 100644 --- a/metacat-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy +++ b/metacat-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy @@ -26,6 +26,7 @@ import com.netflix.metacat.common.QualifiedName import com.netflix.metacat.common.dto.AuditDto import com.netflix.metacat.common.dto.StorageDto import com.netflix.metacat.common.dto.TableDto +import com.netflix.metacat.common.json.MetacatJsonLocator import com.netflix.metacat.common.server.connectors.ConnectorRequestContext import com.netflix.metacat.common.server.connectors.ConnectorTableService import com.netflix.metacat.common.server.connectors.exception.InvalidMetadataException @@ -39,6 +40,7 @@ import com.netflix.metacat.common.server.spi.MetacatCatalogConfig import com.netflix.metacat.common.server.usermetadata.DefaultAuthorizationService import com.netflix.metacat.common.server.usermetadata.TagService import com.netflix.metacat.common.server.usermetadata.UserMetadataService +import com.netflix.metacat.common.server.util.MetacatContextManager import com.netflix.metacat.main.manager.ConnectorManager import com.netflix.metacat.main.services.DatabaseService import com.netflix.metacat.main.services.GetTableServiceParameters @@ -90,7 +92,8 @@ class TableServiceImplSpec extends Specification { connectorTableServiceProxy = new ConnectorTableServiceProxy(connectorManager, converterUtil) authorizationService = new DefaultAuthorizationService(config) service = new TableServiceImpl(connectorManager, connectorTableServiceProxy, databaseService, tagService, - usermetadataService, eventBus, registry, config, converterUtil, authorizationService) + usermetadataService, new MetacatJsonLocator(), + eventBus, registry, config, converterUtil, authorizationService) } def testTableGet() { @@ -283,8 +286,7 @@ class TableServiceImplSpec extends Specification { @Unroll def "test ownership diagnostic logging"() { given: - def definitionMetadataJson = definitionMetadata == null - ? null : (objectMapper.readTree(definitionMetadata as String) as ObjectNode) + def definitionMetadataJson = toObjectNode(definitionMetadata) tableDto = new TableDto( name: name, definitionMetadata: definitionMetadataJson @@ -292,7 +294,7 @@ class TableServiceImplSpec extends Specification { registry = new DefaultRegistry() TableServiceImpl tableService = new TableServiceImpl( connectorManager, connectorTableServiceProxy, databaseService, tagService, - usermetadataService, eventBus, registry, config, converterUtil, authorizationService) + usermetadataService, new MetacatJsonLocator(), eventBus, registry, config, converterUtil, authorizationService) when: tableService.logOwnershipDiagnosticDetails(name, tableDto) @@ -316,4 +318,43 @@ class TableServiceImplSpec extends Specification { "{\"owner\":{\"userId\":\"metacat\"}}" | "metacat" "{\"owner\":{\"userId\":\"root\"}}" | "root" } + + @Unroll + def "test default attributes"() { + given: + def tableService = new TableServiceImpl( + connectorManager, connectorTableServiceProxy, databaseService, tagService, + usermetadataService, new MetacatJsonLocator(), + eventBus, new DefaultRegistry(), config, converterUtil, authorizationService) + + def initialDefinitionMetadataJson = toObjectNode(initialDefinitionMetadata) + tableDto = new TableDto( + name: name, + definitionMetadata: initialDefinitionMetadataJson, + serde: initialSerde + ) + + MetacatContextManager.getContext().setUserName(sessionUser) + + when: + tableService.setDefaultAttributes(tableDto) + + then: + tableDto.getDefinitionMetadata() == toObjectNode(expectedDefMetadata) + tableDto.getSerde() == expectedSerde + + where: + initialDefinitionMetadata | sessionUser | initialSerde || expectedDefMetadata | expectedSerde + null | null | null || "{}" | new StorageDto() + null | "ssarma" | null || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto() + "{\"owner\":{\"userId\":\"ssarma\"}}" | "asdf" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga") + "{\"owner\":{\"userId\":\"metacat\"}}" | "ssarma" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga") + "{\"owner\":{\"userId\":\"root\"}}" | "ssarma" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"ssarma\"}}" | new StorageDto(owner: "swaranga") + "{\"owner\":{\"userId\":\"root\"}}" | "metacat" | new StorageDto(owner: "swaranga") || "{\"owner\":{\"userId\":\"swaranga\"}}" | new StorageDto(owner: "swaranga") + "{\"owner\":{\"userId\":\"root\"}}" | "metacat" | new StorageDto(owner: "metacat") || "{\"owner\":{\"userId\":\"root\"}}" | new StorageDto(owner: "metacat") + } + + ObjectNode toObjectNode(jsonString) { + return jsonString == null ? null : (objectMapper.readTree(jsonString as String) as ObjectNode) + } }