Skip to content

Commit

Permalink
Add validaton to prevent creating another new name with a different uuid
Browse files Browse the repository at this point in the history
  • Loading branch information
Yingjian Wu committed Jul 5, 2024
1 parent 8317b70 commit c91c57e
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 14 deletions.
42 changes: 42 additions & 0 deletions ...ts/src/functionalTest/groovy/com/netflix/metacat/ParentChildRelMetadataServiceSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}
}
45 changes: 33 additions & 12 deletions ...t-main/src/test/groovy/com/netflix/metacat/main/services/impl/TableServiceImplSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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")}
Expand All @@ -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
Expand All @@ -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(_, _)
Expand Down Expand Up @@ -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")
Expand All @@ -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(_)
Expand All @@ -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(_)
Expand All @@ -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(_)
Expand All @@ -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(_)
Expand All @@ -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(_)
Expand Down
63 changes: 61 additions & 2 deletions .../src/main/java/com/netflix/metacat/metadata/mysql/MySqlParentChildRelMetaDataService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
);
}
}

}

0 comments on commit c91c57e

Please sign in to comment.