Copilot commented on code in PR #13527:
URL: https://github.com/apache/gravitino/pull/13527#discussion_r4115264819
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestMetalakeMetaService.java:
##########
@@ -68,6 +74,72 @@ public class TestMetalakeMetaService extends TestJDBCBackend
{
private static final String METALAKE_NAME = "metalake_for_metalake_test";
+ @TestTemplate
+ public void testCascadeDeleteCleansSecurableObjectsAndTagRels() throws
Exception {
+ BaseMetalake metalake = createAndInsertMakeLake(METALAKE_NAME);
+ long metalakeId = metalake.id();
+
+ CatalogEntity catalog = createAndInsertCatalog(METALAKE_NAME,
"cascade_cat");
+
+ // A role with a securable object under this metalake.
+ backend.insert(
+ RoleEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_role")
+ .withNamespace(Namespace.of(METALAKE_NAME, "system", "role"))
+ .withAuditInfo(AUDIT_INFO)
+ .withSecurableObjects(
+ Lists.newArrayList(
+ SecurableObjects.ofCatalog(
+ "cascade_cat",
Lists.newArrayList(Privileges.UseCatalog.allow()))))
+ .build(),
+ false);
+
+ // A tag assigned to a metadata object under this metalake.
+ TagEntity tag =
+ TagEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_tag")
+ .withNamespace(Namespace.of(METALAKE_NAME))
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ backend.insert(tag, false);
+ TagMetaService.getInstance()
+ .associateTagsWithMetadataObject(
+ catalog.nameIdentifier(),
+ catalog.type(),
+ new NameIdentifier[] {NameIdentifier.of(METALAKE_NAME,
"cascade_tag")},
+ new NameIdentifier[0]);
+
+ // The metalake cascade tombstones roles and tags BEFORE the relation
cleanups in
+ // one transaction; the relation cleanups must not filter on the parents'
live rows.
+ Assertions.assertTrue(
+ backend.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE,
true));
Review Comment:
The post-cascade assertions can pass even when either setup path failed to
create its relation, because `liveRows` would already be zero. Assert one
active row for each relation before deleting the metalake so this regression
test proves it exercised both cleanup statements.
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestMetalakeMetaService.java:
##########
@@ -68,6 +74,72 @@ public class TestMetalakeMetaService extends TestJDBCBackend
{
private static final String METALAKE_NAME = "metalake_for_metalake_test";
+ @TestTemplate
+ public void testCascadeDeleteCleansSecurableObjectsAndTagRels() throws
Exception {
+ BaseMetalake metalake = createAndInsertMakeLake(METALAKE_NAME);
+ long metalakeId = metalake.id();
+
+ CatalogEntity catalog = createAndInsertCatalog(METALAKE_NAME,
"cascade_cat");
+
+ // A role with a securable object under this metalake.
+ backend.insert(
+ RoleEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_role")
+ .withNamespace(Namespace.of(METALAKE_NAME, "system", "role"))
+ .withAuditInfo(AUDIT_INFO)
+ .withSecurableObjects(
+ Lists.newArrayList(
+ SecurableObjects.ofCatalog(
+ "cascade_cat",
Lists.newArrayList(Privileges.UseCatalog.allow()))))
+ .build(),
+ false);
+
+ // A tag assigned to a metadata object under this metalake.
+ TagEntity tag =
+ TagEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_tag")
+ .withNamespace(Namespace.of(METALAKE_NAME))
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ backend.insert(tag, false);
+ TagMetaService.getInstance()
+ .associateTagsWithMetadataObject(
+ catalog.nameIdentifier(),
+ catalog.type(),
+ new NameIdentifier[] {NameIdentifier.of(METALAKE_NAME,
"cascade_tag")},
+ new NameIdentifier[0]);
+
+ // The metalake cascade tombstones roles and tags BEFORE the relation
cleanups in
+ // one transaction; the relation cleanups must not filter on the parents'
live rows.
+ Assertions.assertTrue(
+ backend.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE,
true));
+
+ Assertions.assertEquals(
+ 0, liveRows("role_meta_securable_object", "role_id IN (SELECT role_id
FROM role_meta)"));
+ Assertions.assertEquals(
+ 0,
+ liveRows(
+ "tag_relation_meta",
+ "tag_id IN (SELECT tag_id FROM tag_meta WHERE metalake_id = " +
metalakeId + ")"));
+ }
+
+ private long liveRows(String table, String scope) throws Exception {
+ try (java.sql.Connection connection =
+ SqlSessionFactoryHelper.getInstance()
+ .getSqlSessionFactory()
+ .openSession(true)
+ .getConnection()) {
Review Comment:
This opens a MyBatis `SqlSession` only to obtain its connection, but the
session itself is never closed; closing the connection does not release the
session's resources. Keep the `SqlSession` in the try-with-resources alongside
the connection (and use the existing imports).
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestMetalakeMetaService.java:
##########
@@ -68,6 +74,72 @@ public class TestMetalakeMetaService extends TestJDBCBackend
{
private static final String METALAKE_NAME = "metalake_for_metalake_test";
+ @TestTemplate
+ public void testCascadeDeleteCleansSecurableObjectsAndTagRels() throws
Exception {
+ BaseMetalake metalake = createAndInsertMakeLake(METALAKE_NAME);
+ long metalakeId = metalake.id();
+
+ CatalogEntity catalog = createAndInsertCatalog(METALAKE_NAME,
"cascade_cat");
+
+ // A role with a securable object under this metalake.
+ backend.insert(
+ RoleEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_role")
+ .withNamespace(Namespace.of(METALAKE_NAME, "system", "role"))
+ .withAuditInfo(AUDIT_INFO)
+ .withSecurableObjects(
+ Lists.newArrayList(
+ SecurableObjects.ofCatalog(
+ "cascade_cat",
Lists.newArrayList(Privileges.UseCatalog.allow()))))
+ .build(),
+ false);
+
+ // A tag assigned to a metadata object under this metalake.
+ TagEntity tag =
+ TagEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("cascade_tag")
+ .withNamespace(Namespace.of(METALAKE_NAME))
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ backend.insert(tag, false);
+ TagMetaService.getInstance()
+ .associateTagsWithMetadataObject(
+ catalog.nameIdentifier(),
+ catalog.type(),
+ new NameIdentifier[] {NameIdentifier.of(METALAKE_NAME,
"cascade_tag")},
+ new NameIdentifier[0]);
+
+ // The metalake cascade tombstones roles and tags BEFORE the relation
cleanups in
+ // one transaction; the relation cleanups must not filter on the parents'
live rows.
+ Assertions.assertTrue(
+ backend.delete(metalake.nameIdentifier(), Entity.EntityType.METALAKE,
true));
+
+ Assertions.assertEquals(
+ 0, liveRows("role_meta_securable_object", "role_id IN (SELECT role_id
FROM role_meta)"));
+ Assertions.assertEquals(
+ 0,
+ liveRows(
+ "tag_relation_meta",
+ "tag_id IN (SELECT tag_id FROM tag_meta WHERE metalake_id = " +
metalakeId + ")"));
+ }
+
+ private long liveRows(String table, String scope) throws Exception {
+ try (java.sql.Connection connection =
+ SqlSessionFactoryHelper.getInstance()
+ .getSqlSessionFactory()
+ .openSession(true)
+ .getConnection()) {
+ try (java.sql.Statement st = connection.createStatement()) {
+ java.sql.ResultSet rs =
+ st.executeQuery("SELECT COUNT(*) FROM " + table + " WHERE
deleted_at = 0 AND " + scope);
Review Comment:
`Statement` and `ResultSet` are already imported in this file, so the new
fully qualified names are inconsistent with the repository's standard-import
rule. Use the existing imports in this helper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
