Add validaton to prevent creating another new name with a different uuid

Author: Yingjian Wu

metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/ParentChildRelMetadataServiceSpec.groovy

@@ -29,6 +29,16 @@ class ParentChildRelMetadataServiceSpec extends Specification{
     @Shared
     private String database = "testpc"
 
+    @Shared
+    static final String SQL_CREATE_PARENT_CHILD_RELATIONS =
+        "INSERT INTO parent_child_relation (parent, parent_uuid, child, child_uuid, relation_type) " +
+            "VALUES (?, ?, ?, ?, ?)"
+
+    private void insertNewParentChildRecord(final String pName, final String pUUID,
+                                            final String child, final String childUUID, final String type) {
+        jdbcTemplate.update(SQL_CREATE_PARENT_CHILD_RELATIONS, pName, pUUID, child, childUUID, type)
+    }
+
     def setupSpec() {
         String jdbcUrl = "jdbc:mysql://localhost:3306/metacat"
         String username = "metacat_user"
@@ -481,4 +491,36 @@ class ParentChildRelMetadataServiceSpec extends Specification{
         assert service.isChildTable(child2)
         assert !service.isParentTable(child2)
     }
+
+    // This could happen in 2 cases:
+    // 1. Fail to create the table but did not clean up the parent child relationship
+    // 2: Successfully Drop the table but fail to clean up the parent child relationship
+    def "simulate a record is not cleaned up and a same parent or child name is created"() {
+        given:
+        def parent = QualifiedName.ofTable(catalog, database, "p")
+        def parentUUID = "parent_uuid"
+        def child = QualifiedName.ofTable(catalog, database, "c")
+        def childUUID = "child_uuid"
+        def type = "clone"
+        def randomParent = QualifiedName.ofTable(catalog, database, "rp")
+        def randomParentUUID = "random_parent_uuid"
+        def randomChild = QualifiedName.ofTable(catalog, database, "rc")
+        def randomChildUUID = "random_child_uuid"
+
+        insertNewParentChildRecord(parent.toString(), parentUUID, child.toString(), childUUID, type)
+
+        // Same parent name is created with a different uuid should fail
+        when:
+        service.createParentChildRelation(parent, randomParentUUID, randomChild, randomChildUUID, type)
+        then:
+        def e = thrown(RuntimeException)
+        assert e.message.contains("This normally means table prodhive/testpc/p already exists")
+
+        // Same childName with a different uuid should fail
+        when:
+        service.createParentChildRelation(randomParent, randomParentUUID, child, randomChildUUID, type)
+        then:
+        e = thrown(RuntimeException)
+        assert e.message.contains("Cannot have a child table having more than one parent")
+    }
 }

metacat-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy

@@ -36,7 +36,6 @@ import com.netflix.metacat.common.server.events.MetacatEventBus
 import com.netflix.metacat.common.server.events.MetacatUpdateTablePostEvent
 import com.netflix.metacat.common.server.events.MetacatUpdateTablePreEvent
 import com.netflix.metacat.common.server.model.ChildInfo
-import com.netflix.metacat.common.server.model.ParentInfo
 import com.netflix.metacat.common.server.properties.Config
 import com.netflix.metacat.common.server.spi.MetacatCatalogConfig
 import com.netflix.metacat.common.server.usermetadata.DefaultAuthorizationService
@@ -292,6 +291,23 @@ class TableServiceImplSpec extends Specification {
             definitionMetadata: outerNode,
             serde: new StorageDto(uri: 's3:/clone/clone/c')
         )
+        // mock case where create Table Fail as parent table does not exist
+        when:
+        service.create(childTableName, createTableDto)
+        then:
+        1 * config.isParentChildCreateEnabled() >> true
+        1 * ownerValidationService.extractPotentialOwners(_) >> ["cloneClient"]
+        1 * ownerValidationService.isUserValid(_) >> true
+        1 * ownerValidationService.extractPotentialOwnerGroups(_) >> ["cloneClientGroup"]
+        1 * ownerValidationService.isGroupValid(_) >> true
+        1 * connectorTableServiceProxy.exists(_) >> false
+
+        0 * parentChildRelSvc.createParentChildRelation(parentTableName, "p_uuid", childTableName, "child_uuid", "CLONE")
+        0 * connectorTableServiceProxy.create(_, _) >> {throw new RuntimeException("Fail to create")}
+        0 * parentChildRelSvc.deleteParentChildRelation(parentTableName, "p_uuid", childTableName, "child_uuid", "CLONE")
+        def e = thrown(RuntimeException)
+        assert e.message.contains("does not exist")
+
         // mock case where create Table Fail and revert function is triggerred
         when:
         service.create(childTableName, createTableDto)
@@ -301,6 +317,7 @@ class TableServiceImplSpec extends Specification {
         1 * ownerValidationService.isUserValid(_) >> true
         1 * ownerValidationService.extractPotentialOwnerGroups(_) >> ["cloneClientGroup"]
         1 * ownerValidationService.isGroupValid(_) >> true
+        1 * connectorTableServiceProxy.exists(_) >> true
 
         1 * parentChildRelSvc.createParentChildRelation(parentTableName, "p_uuid", childTableName, "child_uuid", "CLONE")
         1 * connectorTableServiceProxy.create(_, _) >> {throw new RuntimeException("Fail to create")}
@@ -317,10 +334,11 @@ class TableServiceImplSpec extends Specification {
         1 * ownerValidationService.isUserValid(_) >> true
         1 * ownerValidationService.extractPotentialOwnerGroups(_) >> ["cloneClientGroup"]
         1 * ownerValidationService.isGroupValid(_) >> true
+        0 * connectorTableServiceProxy.exists(_)
 
         0 * parentChildRelSvc.createParentChildRelation(parentTableName, "p_uuid", childTableName, "child_uuid", "CLONE")
         0 * connectorTableServiceProxy.create(_, _)
-        def e = thrown(RuntimeException)
+        e = thrown(RuntimeException)
         assert e.message.contains("is currently disabled")
 
         // mock successful case
@@ -332,6 +350,7 @@ class TableServiceImplSpec extends Specification {
         1 * ownerValidationService.isUserValid(_) >> true
         1 * ownerValidationService.extractPotentialOwnerGroups(_) >> ["cloneClientGroup"]
         1 * ownerValidationService.isGroupValid(_) >> true
+        1 * connectorTableServiceProxy.exists(_) >> true
 
         1 * parentChildRelSvc.createParentChildRelation(parentTableName, "p_uuid", childTableName, "child_uuid", "CLONE")
         1 * connectorTableServiceProxy.create(_, _)
@@ -417,7 +436,8 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> true
         1 * parentChildRelSvc.getParents(name) >> {[] as Set}
-        2 * parentChildRelSvc.getChildren(name) >> {[new ChildInfo("child", "clone", "child_uuid")] as Set}
+        1 * parentChildRelSvc.isParentTable(name) >> true
+        1 * parentChildRelSvc.getChildren(name) >> {[new ChildInfo("child", "clone", "child_uuid")] as Set}
         1 * config.getNoTableDeleteOnTags() >> []
         def e = thrown(RuntimeException)
         assert e.message.contains("because it still has")
@@ -429,7 +449,8 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> true
         1 * parentChildRelSvc.getParents(name)
-        2 * parentChildRelSvc.getChildren(name)
+        1 * parentChildRelSvc.isParentTable(name) >> true
+        1 * parentChildRelSvc.getChildren(name)
         1 * config.getNoTableDeleteOnTags() >> []
         1 * connectorTableServiceProxy.delete(_) >> {throw new RuntimeException("Fail to drop")}
         0 * parentChildRelSvc.drop(_)
@@ -442,9 +463,9 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> false
         1 * parentChildRelSvc.isChildTable(name) >> true
-        0 * parentChildRelSvc.isParentTable(name)
+        1 * parentChildRelSvc.isParentTable(name) >> false
         1 * parentChildRelSvc.getParents(name)
-        1 * parentChildRelSvc.getChildren(name)
+        0 * parentChildRelSvc.getChildren(name)
         1 * config.getNoTableDeleteOnTags() >> []
         0 * connectorTableServiceProxy.delete(_)
         0 * parentChildRelSvc.drop(_)
@@ -458,9 +479,9 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> false
         1 * parentChildRelSvc.isChildTable(name) >> false
-        1 * parentChildRelSvc.isParentTable(name) >> true
+        2 * parentChildRelSvc.isParentTable(name) >> true
         1 * parentChildRelSvc.getParents(name)
-        1 * parentChildRelSvc.getChildren(name)
+        0 * parentChildRelSvc.getChildren(name)
         1 * config.getNoTableDeleteOnTags() >> []
         0 * connectorTableServiceProxy.delete(_)
         0 * parentChildRelSvc.drop(_)
@@ -475,9 +496,9 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> false
         1 * parentChildRelSvc.isChildTable(name) >> false
-        1 * parentChildRelSvc.isParentTable(name) >> false
+        2 * parentChildRelSvc.isParentTable(name) >> false
         1 * parentChildRelSvc.getParents(name) >> {[] as Set}
-        2 * parentChildRelSvc.getChildren(name) >> {[] as Set}
+        1 * parentChildRelSvc.getChildren(name) >> {[] as Set}
         1 * config.getNoTableDeleteOnTags() >> []
         1 * connectorTableServiceProxy.delete(_)
         1 * parentChildRelSvc.drop(_)
@@ -491,9 +512,9 @@ class TableServiceImplSpec extends Specification {
         1 * config.isParentChildGetEnabled() >> true
         1 * config.isParentChildDropEnabled() >> true
         0 * parentChildRelSvc.isChildTable(name)
-        0 * parentChildRelSvc.isParentTable(name)
+        1 * parentChildRelSvc.isParentTable(name)
         1 * parentChildRelSvc.getParents(name) >> {[] as Set}
-        2 * parentChildRelSvc.getChildren(name) >> {[] as Set}
+        1 * parentChildRelSvc.getChildren(name) >> {[] as Set}
         1 * config.getNoTableDeleteOnTags() >> []
         1 * connectorTableServiceProxy.delete(_)
         1 * parentChildRelSvc.drop(_)

metacat-metadata-mysql/src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java

@@ -17,9 +17,10 @@
 import org.springframework.transaction.annotation.Transactional;
 
 import java.sql.PreparedStatement;
-import java.util.Set;
-import java.util.List;
 import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.StringJoiner;
 import java.util.HashSet;
 import java.util.stream.Collectors;
 import java.sql.ResultSet;
@@ -60,6 +61,12 @@ public class MySqlParentChildRelMetaDataService implements ParentChildRelMetadat
     static final String SQL_IS_PARENT_TABLE = "SELECT 1 FROM parent_child_relation WHERE parent = ?";
     static final String SQL_IS_CHILD_TABLE = "SELECT 1 FROM parent_child_relation WHERE child = ?";
 
+    static final String SQL_GET_PARENT_UUIDS = "SELECT DISTINCT parent_uuid FROM parent_child_relation "
+        + "where parent = ?";
+
+    static final String SQL_GET_CHILDREN_UUIDS = "SELECT DISTINCT child_uuid FROM parent_child_relation "
+        + "where child = ?";
+
     private final JdbcTemplate jdbcTemplate;
     private final ConverterUtil converterUtil;
 
@@ -104,6 +111,14 @@ public void createParentChildRelation(final QualifiedName parentName,
                 + "- child table: " + childName + " already have child");
         }
 
+        // Validation to prevent creating a parent with an uuid that is different from the existing parent uuids
+        final Set<String> existingParentUuids = getExistingUUIDS(parentName.toString());
+        validateUUIDs(parentName.toString(), existingParentUuids, parentUUID, "Parent");
+
+        // Validation to prevent creating a child with an uuid that is different from the existing child uuids
+        final Set<String> existingChildUuids = getExistingUUIDS(childName.toString());
+        validateUUIDs(childName.toString(), existingChildUuids, childUUID, "Child");
+
         try {
             jdbcTemplate.update(connection -> {
                 final PreparedStatement ps = connection.prepareStatement(SQL_CREATE_PARENT_CHILD_RELATIONS);
@@ -304,4 +319,48 @@ public Boolean extractData(final ResultSet rs) throws SQLException {
             }
         );
     }
+
+    private Set<String> getExistingUUIDS(final String tableName) {
+        final Set<String> existingUUIDs = new HashSet<>();
+        existingUUIDs.addAll(getParentUuidsForParent(tableName));
+        existingUUIDs.addAll(getChildUuidsForChild(tableName));
+        return existingUUIDs;
+    }
+
+    private Set<String> getParentUuidsForParent(final String parentName) {
+        return new HashSet<>(jdbcTemplate.query(connection -> {
+            final PreparedStatement ps = connection.prepareStatement(SQL_GET_PARENT_UUIDS);
+            ps.setString(1, parentName);
+            return ps;
+        }, (rs, rowNum) -> rs.getString("parent_uuid")));
+    }
+
+    private Set<String> getChildUuidsForChild(final String childName) {
+        return new HashSet<>(jdbcTemplate.query(connection -> {
+            final PreparedStatement ps = connection.prepareStatement(SQL_GET_CHILDREN_UUIDS);
+            ps.setString(1, childName);
+            return ps;
+        }, (rs, rowNum) -> rs.getString("child_uuid")));
+    }
+
+    private void validateUUIDs(final String name,
+                               final Set<String> existingUUIDs,
+                               final String inputUUID,
+                               final String entity
+    ) throws ParentChildRelServiceException {
+        if (existingUUIDs.size() > 1 || (!existingUUIDs.isEmpty() && !existingUUIDs.contains(inputUUID))) {
+            final StringJoiner uuidJoiner = new StringJoiner(", ");
+            for (String uuid : existingUUIDs) {
+                uuidJoiner.add(uuid);
+            }
+            final String existingUuidsString = uuidJoiner.toString();
+
+            throw new ParentChildRelServiceException(
+                String.format("Cannot create parent-child relation: %s '%s' has existing UUIDs [%s] "
+                        + "that differ from the input %s UUID '%s'. This normally means table %s already exists",
+                    entity, name, existingUuidsString, entity, inputUUID, name)
+            );
+        }
+    }
+
 }