mchades commented on code in PR #12503:
URL: https://github.com/apache/gravitino/pull/12503#discussion_r3840561369
##########
docs/open-api/policies.yaml:
##########
@@ -497,6 +497,8 @@ components:
- "FILESET"
- "TOPIC"
- "MODEL"
+ - "VIEW"
Review Comment:
Please also add `"VIEW"` and `"FUNCTION"` to
`PolicyContentBase.properties.supportedObjectTypes.items.enum` (around line
393) to keep both enum definitions consistent:
```yaml
enum: [ "CATALOG", "SCHEMA", "TABLE", "FILESET", "TOPIC",
"MODEL", "VIEW", "FUNCTION" ]
```
Please run `./gradlew :docs:build` to ensure the OpenAPI specification
passes validation.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/PolicyIT.java:
##########
@@ -759,6 +827,380 @@ public void testAssociatePoliciesToModel() {
MetadataObject.Type.MODEL,
policy3.associatedObjects().objects()[0].type());
}
+ @Test
+ public void testAssociatePoliciesToView() {
+ PolicyContent content =
+ PolicyContents.custom(
+ ImmutableMap.of("rule1", "value1"),
ImmutableSet.of(MetadataObject.Type.VIEW), null);
+ Policy policy1 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy1"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy2 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy2"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy3 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy3"),
+ "custom",
+ null,
+ true,
+ content);
+
+ // Associate policies to catalog
+ relationalCatalog.supportsPolicies().associatePolicies(new String[]
{policy1.name()}, null);
+
+ // Associate policies to schema
+ schema.supportsPolicies().associatePolicies(new String[] {policy2.name()},
null);
+
+ // Test associate policies to view
+ String[] policies =
+ view.supportsPolicies().associatePolicies(new String[]
{policy3.name()}, null);
+
+ Assertions.assertEquals(1, policies.length);
+ Assertions.assertEquals(policy3.name(), policies[0]);
+
+ // Test list associated policies for view
+ String[] policies1 = view.supportsPolicies().listPolicies();
+ Assertions.assertEquals(3, policies1.length);
+ Set<String> policyNames = Sets.newHashSet(policies1);
+ Assertions.assertTrue(policyNames.contains(policy1.name()));
+ Assertions.assertTrue(policyNames.contains(policy2.name()));
+ Assertions.assertTrue(policyNames.contains(policy3.name()));
+
+ // Test list associated policies with details for view
+ Policy[] policies2 = view.supportsPolicies().listPolicyInfos();
+ Assertions.assertEquals(3, policies2.length);
+
+ Set<Policy> nonInheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> !policy.inherited().get())
+ .collect(Collectors.toSet());
+ Set<Policy> inheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> policy.inherited().get())
+ .collect(Collectors.toSet());
+
+ Assertions.assertEquals(1, nonInheritedPolicies.size());
+ Assertions.assertEquals(2, inheritedPolicies.size());
+ Assertions.assertTrue(nonInheritedPolicies.contains(policy3));
+ Assertions.assertTrue(inheritedPolicies.contains(policy1));
+ Assertions.assertTrue(inheritedPolicies.contains(policy2));
+
+ // Test get associated policy for view
+ Policy resultPolicy1 = view.supportsPolicies().getPolicy(policy1.name());
+ Assertions.assertEquals(policy1, resultPolicy1);
+ Assertions.assertTrue(resultPolicy1.inherited().get());
+
+ Policy resultPolicy2 = view.supportsPolicies().getPolicy(policy2.name());
+ Assertions.assertEquals(policy2, resultPolicy2);
+ Assertions.assertTrue(resultPolicy2.inherited().get());
+
+ Policy resultPolicy3 = view.supportsPolicies().getPolicy(policy3.name());
+ Assertions.assertEquals(policy3, resultPolicy3);
+ Assertions.assertFalse(resultPolicy3.inherited().get());
+
+ // Test get objects associated with policy
+ Assertions.assertEquals(1, policy1.associatedObjects().count());
+ Assertions.assertEquals(
+ relationalCatalog.name(),
policy1.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.CATALOG,
policy1.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy2.associatedObjects().count());
+ Assertions.assertEquals(schema.name(),
policy2.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.SCHEMA,
policy2.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy3.associatedObjects().count());
+ Assertions.assertEquals(view.name(),
policy3.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.VIEW,
policy3.associatedObjects().objects()[0].type());
+ }
+
+ @Test
+ public void testAssociatePoliciesToFunction() {
+ PolicyContent content =
+ PolicyContents.custom(
+ ImmutableMap.of("rule1", "value1"),
+ ImmutableSet.of(MetadataObject.Type.FUNCTION),
+ null);
+ Policy policy1 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy1"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy2 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy2"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy3 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy3"),
+ "custom",
+ null,
+ true,
+ content);
+
+ // Associate policies to catalog
+ relationalCatalog.supportsPolicies().associatePolicies(new String[]
{policy1.name()}, null);
+
+ // Associate policies to schema
+ schema.supportsPolicies().associatePolicies(new String[] {policy2.name()},
null);
+
+ // Test associate policies to function
+ String[] policies =
+ function.supportsPolicies().associatePolicies(new String[]
{policy3.name()}, null);
+
+ Assertions.assertEquals(1, policies.length);
+ Assertions.assertEquals(policy3.name(), policies[0]);
+
+ // Test list associated policies for function
+ String[] policies1 = function.supportsPolicies().listPolicies();
+ Assertions.assertEquals(3, policies1.length);
+ Set<String> policyNames = Sets.newHashSet(policies1);
+ Assertions.assertTrue(policyNames.contains(policy1.name()));
+ Assertions.assertTrue(policyNames.contains(policy2.name()));
+ Assertions.assertTrue(policyNames.contains(policy3.name()));
+
+ // Test list associated policies with details for function
+ Policy[] policies2 = function.supportsPolicies().listPolicyInfos();
+ Assertions.assertEquals(3, policies2.length);
+
+ Set<Policy> nonInheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> !policy.inherited().get())
+ .collect(Collectors.toSet());
+ Set<Policy> inheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> policy.inherited().get())
+ .collect(Collectors.toSet());
+
+ Assertions.assertEquals(1, nonInheritedPolicies.size());
+ Assertions.assertEquals(2, inheritedPolicies.size());
+ Assertions.assertTrue(nonInheritedPolicies.contains(policy3));
+ Assertions.assertTrue(inheritedPolicies.contains(policy1));
+ Assertions.assertTrue(inheritedPolicies.contains(policy2));
+
+ // Test get associated policy for function
+ Policy resultPolicy1 =
function.supportsPolicies().getPolicy(policy1.name());
+ Assertions.assertEquals(policy1, resultPolicy1);
+ Assertions.assertTrue(resultPolicy1.inherited().get());
+
+ Policy resultPolicy2 =
function.supportsPolicies().getPolicy(policy2.name());
+ Assertions.assertEquals(policy2, resultPolicy2);
+ Assertions.assertTrue(resultPolicy2.inherited().get());
+
+ Policy resultPolicy3 =
function.supportsPolicies().getPolicy(policy3.name());
+ Assertions.assertEquals(policy3, resultPolicy3);
+ Assertions.assertFalse(resultPolicy3.inherited().get());
+
+ // Test get objects associated with policy
+ Assertions.assertEquals(1, policy1.associatedObjects().count());
+ Assertions.assertEquals(
+ relationalCatalog.name(),
policy1.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.CATALOG,
policy1.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy2.associatedObjects().count());
+ Assertions.assertEquals(schema.name(),
policy2.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.SCHEMA,
policy2.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy3.associatedObjects().count());
+ Assertions.assertEquals(function.name(),
policy3.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.FUNCTION,
policy3.associatedObjects().objects()[0].type());
+ }
+
+ @Test
Review Comment:
The helper method `createCustomPolicy(String name)` hardcodes
`ImmutableSet.of(MetadataObject.Type.CATALOG)`, which does not match the
comment `// Create a policy that supports VIEW` or the intended test scope for
VIEW / FUNCTION.
Could you please update `createCustomPolicy` to accept the supported object
types (e.g., `createCustomPolicy(String name, MetadataObject.Type... types)`)
or define the policy explicitly with `MetadataObject.Type.VIEW` /
`MetadataObject.Type.FUNCTION` like in
`testCascadeDeleteSchemaWithViewAndFunction`?
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/PolicyIT.java:
##########
@@ -759,6 +827,380 @@ public void testAssociatePoliciesToModel() {
MetadataObject.Type.MODEL,
policy3.associatedObjects().objects()[0].type());
}
+ @Test
+ public void testAssociatePoliciesToView() {
+ PolicyContent content =
+ PolicyContents.custom(
+ ImmutableMap.of("rule1", "value1"),
ImmutableSet.of(MetadataObject.Type.VIEW), null);
+ Policy policy1 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy1"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy2 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy2"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy3 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_view_policy3"),
+ "custom",
+ null,
+ true,
+ content);
+
+ // Associate policies to catalog
+ relationalCatalog.supportsPolicies().associatePolicies(new String[]
{policy1.name()}, null);
+
+ // Associate policies to schema
+ schema.supportsPolicies().associatePolicies(new String[] {policy2.name()},
null);
+
+ // Test associate policies to view
+ String[] policies =
+ view.supportsPolicies().associatePolicies(new String[]
{policy3.name()}, null);
+
+ Assertions.assertEquals(1, policies.length);
+ Assertions.assertEquals(policy3.name(), policies[0]);
+
+ // Test list associated policies for view
+ String[] policies1 = view.supportsPolicies().listPolicies();
+ Assertions.assertEquals(3, policies1.length);
+ Set<String> policyNames = Sets.newHashSet(policies1);
+ Assertions.assertTrue(policyNames.contains(policy1.name()));
+ Assertions.assertTrue(policyNames.contains(policy2.name()));
+ Assertions.assertTrue(policyNames.contains(policy3.name()));
+
+ // Test list associated policies with details for view
+ Policy[] policies2 = view.supportsPolicies().listPolicyInfos();
+ Assertions.assertEquals(3, policies2.length);
+
+ Set<Policy> nonInheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> !policy.inherited().get())
+ .collect(Collectors.toSet());
+ Set<Policy> inheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> policy.inherited().get())
+ .collect(Collectors.toSet());
+
+ Assertions.assertEquals(1, nonInheritedPolicies.size());
+ Assertions.assertEquals(2, inheritedPolicies.size());
+ Assertions.assertTrue(nonInheritedPolicies.contains(policy3));
+ Assertions.assertTrue(inheritedPolicies.contains(policy1));
+ Assertions.assertTrue(inheritedPolicies.contains(policy2));
+
+ // Test get associated policy for view
+ Policy resultPolicy1 = view.supportsPolicies().getPolicy(policy1.name());
+ Assertions.assertEquals(policy1, resultPolicy1);
+ Assertions.assertTrue(resultPolicy1.inherited().get());
+
+ Policy resultPolicy2 = view.supportsPolicies().getPolicy(policy2.name());
+ Assertions.assertEquals(policy2, resultPolicy2);
+ Assertions.assertTrue(resultPolicy2.inherited().get());
+
+ Policy resultPolicy3 = view.supportsPolicies().getPolicy(policy3.name());
+ Assertions.assertEquals(policy3, resultPolicy3);
+ Assertions.assertFalse(resultPolicy3.inherited().get());
+
+ // Test get objects associated with policy
+ Assertions.assertEquals(1, policy1.associatedObjects().count());
+ Assertions.assertEquals(
+ relationalCatalog.name(),
policy1.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.CATALOG,
policy1.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy2.associatedObjects().count());
+ Assertions.assertEquals(schema.name(),
policy2.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.SCHEMA,
policy2.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy3.associatedObjects().count());
+ Assertions.assertEquals(view.name(),
policy3.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.VIEW,
policy3.associatedObjects().objects()[0].type());
+ }
+
+ @Test
+ public void testAssociatePoliciesToFunction() {
+ PolicyContent content =
+ PolicyContents.custom(
+ ImmutableMap.of("rule1", "value1"),
+ ImmutableSet.of(MetadataObject.Type.FUNCTION),
+ null);
+ Policy policy1 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy1"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy2 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy2"),
+ "custom",
+ null,
+ true,
+ content);
+ Policy policy3 =
+ metalake.createPolicy(
+ GravitinoITUtils.genRandomName("policy_it_func_policy3"),
+ "custom",
+ null,
+ true,
+ content);
+
+ // Associate policies to catalog
+ relationalCatalog.supportsPolicies().associatePolicies(new String[]
{policy1.name()}, null);
+
+ // Associate policies to schema
+ schema.supportsPolicies().associatePolicies(new String[] {policy2.name()},
null);
+
+ // Test associate policies to function
+ String[] policies =
+ function.supportsPolicies().associatePolicies(new String[]
{policy3.name()}, null);
+
+ Assertions.assertEquals(1, policies.length);
+ Assertions.assertEquals(policy3.name(), policies[0]);
+
+ // Test list associated policies for function
+ String[] policies1 = function.supportsPolicies().listPolicies();
+ Assertions.assertEquals(3, policies1.length);
+ Set<String> policyNames = Sets.newHashSet(policies1);
+ Assertions.assertTrue(policyNames.contains(policy1.name()));
+ Assertions.assertTrue(policyNames.contains(policy2.name()));
+ Assertions.assertTrue(policyNames.contains(policy3.name()));
+
+ // Test list associated policies with details for function
+ Policy[] policies2 = function.supportsPolicies().listPolicyInfos();
+ Assertions.assertEquals(3, policies2.length);
+
+ Set<Policy> nonInheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> !policy.inherited().get())
+ .collect(Collectors.toSet());
+ Set<Policy> inheritedPolicies =
+ Arrays.stream(policies2)
+ .filter(policy -> policy.inherited().get())
+ .collect(Collectors.toSet());
+
+ Assertions.assertEquals(1, nonInheritedPolicies.size());
+ Assertions.assertEquals(2, inheritedPolicies.size());
+ Assertions.assertTrue(nonInheritedPolicies.contains(policy3));
+ Assertions.assertTrue(inheritedPolicies.contains(policy1));
+ Assertions.assertTrue(inheritedPolicies.contains(policy2));
+
+ // Test get associated policy for function
+ Policy resultPolicy1 =
function.supportsPolicies().getPolicy(policy1.name());
+ Assertions.assertEquals(policy1, resultPolicy1);
+ Assertions.assertTrue(resultPolicy1.inherited().get());
+
+ Policy resultPolicy2 =
function.supportsPolicies().getPolicy(policy2.name());
+ Assertions.assertEquals(policy2, resultPolicy2);
+ Assertions.assertTrue(resultPolicy2.inherited().get());
+
+ Policy resultPolicy3 =
function.supportsPolicies().getPolicy(policy3.name());
+ Assertions.assertEquals(policy3, resultPolicy3);
+ Assertions.assertFalse(resultPolicy3.inherited().get());
+
+ // Test get objects associated with policy
+ Assertions.assertEquals(1, policy1.associatedObjects().count());
+ Assertions.assertEquals(
+ relationalCatalog.name(),
policy1.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.CATALOG,
policy1.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy2.associatedObjects().count());
+ Assertions.assertEquals(schema.name(),
policy2.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.SCHEMA,
policy2.associatedObjects().objects()[0].type());
+
+ Assertions.assertEquals(1, policy3.associatedObjects().count());
+ Assertions.assertEquals(function.name(),
policy3.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.FUNCTION,
policy3.associatedObjects().objects()[0].type());
+ }
+
+ @Test
+ public void testCascadeDeleteView() {
+ // Create a policy that supports VIEW
+ Policy policy =
createCustomPolicy(GravitinoITUtils.genRandomName("policy_it_cascade_view"));
+
+ // Create a view in the existing schema
+ String viewName = GravitinoITUtils.genRandomName("policy_it_cascade_view");
+ NameIdentifier viewIdent = NameIdentifier.of(schema.name(), viewName);
+
Assertions.assertFalse(relationalCatalog.asViewCatalog().viewExists(viewIdent));
+ View cascadeView =
+ relationalCatalog
+ .asViewCatalog()
+ .createView(
+ viewIdent,
+ "comment",
+ new Column[] {
+ Column.of("col1", Types.IntegerType.get()),
+ Column.of("col2", Types.StringType.get())
+ },
+ new SQLRepresentation[] {
+ SQLRepresentation.builder()
+ .withDialect(Dialects.HIVE)
+ .withSql("SELECT col1, col2 FROM " + table.name())
+ .build()
+ },
+ null,
+ null,
+ Collections.emptyMap());
+
+ // Associate the policy with the view
+ cascadeView.supportsPolicies().associatePolicies(new String[]
{policy.name()}, null);
+
+ // Verify the policy is associated with the view
+ Policy fetchedPolicy = metalake.getPolicy(policy.name());
+ Assertions.assertEquals(1, fetchedPolicy.associatedObjects().count());
+ Assertions.assertEquals(
+ cascadeView.name(),
fetchedPolicy.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.VIEW,
fetchedPolicy.associatedObjects().objects()[0].type());
+
+ // Delete the view — this should cascade-delete policy relations
+
Assertions.assertTrue(relationalCatalog.asViewCatalog().dropView(viewIdent));
+
+ // Verify the policy's associated objects are cleaned up
+ fetchedPolicy = metalake.getPolicy(policy.name());
+ Assertions.assertEquals(0, fetchedPolicy.associatedObjects().count());
+ }
+
+ @Test
+ public void testCascadeDeleteFunction() {
+ // Create a policy that supports FUNCTION
+ Policy policy =
createCustomPolicy(GravitinoITUtils.genRandomName("policy_it_cascade_func"));
+
+ // Create a function in the existing schema
+ String funcName = GravitinoITUtils.genRandomName("policy_it_cascade_func");
+ NameIdentifier funcIdent = NameIdentifier.of(schema.name(), funcName);
+
Assertions.assertFalse(relationalCatalog.asFunctionCatalog().functionExists(funcIdent));
+ FunctionParam param = FunctionParams.of("x", Types.IntegerType.get());
+ FunctionImpl impl = FunctionImpls.ofSql(FunctionImpl.RuntimeType.SPARK,
"SELECT x + 1");
+ FunctionDefinition definition =
+ FunctionDefinitions.of(
+ new FunctionParam[] {param}, Types.IntegerType.get(), new
FunctionImpl[] {impl});
+ Function cascadeFunc =
+ relationalCatalog
+ .asFunctionCatalog()
+ .registerFunction(
+ funcIdent,
+ "comment",
+ FunctionType.SCALAR,
+ true,
+ new FunctionDefinition[] {definition});
+
+ // Associate the policy with the function
+ cascadeFunc.supportsPolicies().associatePolicies(new String[]
{policy.name()}, null);
+
+ // Verify the policy is associated with the function
+ Policy fetchedPolicy = metalake.getPolicy(policy.name());
+ Assertions.assertEquals(1, fetchedPolicy.associatedObjects().count());
+ Assertions.assertEquals(
+ cascadeFunc.name(),
fetchedPolicy.associatedObjects().objects()[0].name());
+ Assertions.assertEquals(
+ MetadataObject.Type.FUNCTION,
fetchedPolicy.associatedObjects().objects()[0].type());
+
+ // Delete the function — this should cascade-delete policy relations
+
Assertions.assertTrue(relationalCatalog.asFunctionCatalog().dropFunction(funcIdent));
+
+ // Verify the policy's associated objects are cleaned up
+ fetchedPolicy = metalake.getPolicy(policy.name());
+ Assertions.assertEquals(0, fetchedPolicy.associatedObjects().count());
+ }
Review Comment:
Thanks for adding the schema cascade deletion test! Could we also add a test
case for catalog cascade drop (`metalake.dropCatalog(cascadeCatalogName,
true)`) to verify `softDeletePolicyMetadataObjectRelsByCatalogId` end-to-end
for `VIEW` and `FUNCTION`?
--
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]
