Copilot commented on code in PR #9372:
URL: https://github.com/apache/gravitino/pull/9372#discussion_r2587167624
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerAuthorizationIT.java:
##########
@@ -363,4 +384,113 @@ public void testGetTableOwner() {
ImmutableList.of(CATALOG, SCHEMA, "table1"),
MetadataObject.Type.TABLE));
});
}
+
+ @Test
+ @Order(9)
+ public void testGetMetalakeOwner() {
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of(METALAKE),
MetadataObject.Type.METALAKE));
+ GravitinoMetalake gravitinoMetalakeLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE);
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of(METALAKE),
MetadataObject.Type.METALAKE));
+ client.createMetalake("tempMetalake", "metalake1 comment",
Collections.emptyMap());
Review Comment:
The created metalake "tempMetalake" is not verified or used in any
assertions. Either add assertions to verify the creation succeeded and test
ownership of this metalake, or remove this line if it's not needed for the test.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java:
##########
@@ -118,4 +118,11 @@ public class AuthorizationExpressionConstants {
"""
METALAKE::OWNER || POLICY::OWNER || ANY_APPLY_POLICY
""";
+
Review Comment:
[nitpick] The constant name `loadMetalakeAuthorizationExpression` is
inconsistent with the naming pattern of other constants in this file. All other
authorization expression constants use lowercase_with_underscores for the
actual expression value (e.g., `METALAKE::OWNER || CATALOG::OWNER`), but this
one uses a special value `"METALAKE_USER"`. Consider either renaming the
expression value to match the pattern or adding a comment explaining why this
special case exists.
```suggestion
// Special case: "METALAKE_USER" is used here as a unique authorization
token,
// not a logical expression like other constants. This is intentional and
required
// for metalake-level user authorization checks.
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerAuthorizationIT.java:
##########
@@ -363,4 +384,113 @@ public void testGetTableOwner() {
ImmutableList.of(CATALOG, SCHEMA, "table1"),
MetadataObject.Type.TABLE));
});
}
+
+ @Test
+ @Order(9)
+ public void testGetMetalakeOwner() {
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of(METALAKE),
MetadataObject.Type.METALAKE));
+ GravitinoMetalake gravitinoMetalakeLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE);
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of(METALAKE),
MetadataObject.Type.METALAKE));
+ client.createMetalake("tempMetalake", "metalake1 comment",
Collections.emptyMap());
+ }
+
+ @Test
+ @Order(10)
+ public void testGetPolicyOwner() {
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ GravitinoMetalake gravitinoMetalakeLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE);
+ Set<MetadataObject.Type> supportedTypes =
ImmutableSet.of(MetadataObject.Type.TABLE);
+ String policyName = "policy1";
+ gravitinoMetalake.createPolicy(
+ policyName, "custom", "policy1", true, PolicyContents.custom(null,
supportedTypes, null));
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of(policyName),
MetadataObject.Type.POLICY));
+ assertThrows(
+ "Current user can not get owner",
+ ForbiddenException.class,
+ () -> {
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of(policyName),
MetadataObject.Type.POLICY));
+ });
+ }
+
+ @Test
+ @Order(11)
+ public void testGetTagOwner() {
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ GravitinoMetalake gravitinoMetalakeLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE);
+ String tagName = "tag1";
+ gravitinoMetalake.createTag("tag1", "tag1", Map.of());
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of(tagName),
MetadataObject.Type.TAG));
+ assertThrows(
+ "Current user can not get owner",
+ ForbiddenException.class,
+ () -> {
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of(tagName),
MetadataObject.Type.TAG));
+ });
+ }
+
+ @Test
+ @Order(12)
+ public void testGetJobOwner() throws IOException {
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ GravitinoMetalake gravitinoMetalakeLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE);
+ testStagingDir = Files.createTempDirectory("test_staging_dir").toFile();
+ String testEntryScriptPath = generateTestEntryScript();
+ String testLibScriptPath = generateTestLibScript();
+
+ builder =
+ ShellJobTemplate.builder()
+ .withComment("Test shell job template")
+ .withExecutable(testEntryScriptPath)
+ .withArguments(Lists.newArrayList("{{arg1}}", "{{arg2}}"))
+ .withEnvironments(ImmutableMap.of("ENV_VAR", "{{env_var}}"))
+ .withScripts(Lists.newArrayList(testLibScriptPath))
+ .withCustomFields(Collections.emptyMap());
+ JobTemplate template1 = builder.withName("test_1").build();
+ gravitinoMetalake.registerJobTemplate(template1);
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of("test_1"),
MetadataObject.Type.JOB_TEMPLATE));
+ assertThrows(
+ "Current user can not get owner",
+ ForbiddenException.class,
+ () -> {
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of("test_1"),
MetadataObject.Type.JOB_TEMPLATE));
+ });
+ JobHandle jobHandle =
+ gravitinoMetalake.runJob(
+ "test_1", ImmutableMap.of("arg1", "value1", "arg2", "success",
"env_var", "value2"));
+ gravitinoMetalake.getOwner(
+ MetadataObjects.of(ImmutableList.of(jobHandle.jobId()),
MetadataObject.Type.JOB));
+ assertThrows(
+ "Current user can not get owner",
+ ForbiddenException.class,
+ () -> {
+ gravitinoMetalakeLoadByNormalUser.getOwner(
+ MetadataObjects.of(ImmutableList.of(jobHandle.jobId()),
MetadataObject.Type.JOB));
+ });
+ }
+
+ @Test
+ @Order(13)
+ public void getRoleOwner() {
Review Comment:
The test method name `getRoleOwner()` is inconsistent with other test method
names in this file, which all use the `testGetXxxOwner()` pattern. It should be
renamed to `testGetRoleOwner()` for consistency.
```suggestion
public void testGetRoleOwner() {
```
--
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]
