Copilot commented on code in PR #9384:
URL: https://github.com/apache/gravitino/pull/9384#discussion_r2596842833
##########
api/src/main/java/org/apache/gravitino/authorization/Privilege.java:
##########
@@ -89,9 +89,15 @@ enum Name {
CREATE_ROLE(0L, 1L << 16),
/** The privilege to grant or revoke a role for the user or the group. */
MANAGE_GRANTS(0L, 1L << 17),
- /** The privilege to create a model */
+ /** The privilege to register a model */
+ REGISTER_MODEL(0L, 1L << 18),
+ /** The privilege to create a model. This is deprecated. Please use
REGISTER_MODEL */
CREATE_MODEL(0L, 1L << 18),
- /** The privilege to create a model version */
+ /** The privilege to link a model version */
+ LINK_MODEL_VERSION(0L, 1L << 19),
+ /**
+ * The privilege to create a model version. This is deprecated. Please use
LINK_MODEL_VERSION
+ */
CREATE_MODEL_VERSION(0L, 1L << 19),
Review Comment:
[nitpick] The enum values `CREATE_MODEL` and `CREATE_MODEL_VERSION` are
marked as deprecated in their Javadoc comments but lack the `@Deprecated`
annotation. Adding the annotation would provide compile-time warnings when
these deprecated values are used, improving the deprecation signal.
Example:
```java
/** The privilege to create a model. This is deprecated. Please use
REGISTER_MODEL */
@Deprecated
CREATE_MODEL(0L, 1L << 18),
```
##########
api/src/main/java/org/apache/gravitino/authorization/Privilege.java:
##########
@@ -89,9 +89,15 @@ enum Name {
CREATE_ROLE(0L, 1L << 16),
/** The privilege to grant or revoke a role for the user or the group. */
MANAGE_GRANTS(0L, 1L << 17),
- /** The privilege to create a model */
+ /** The privilege to register a model */
+ REGISTER_MODEL(0L, 1L << 18),
+ /** The privilege to create a model. This is deprecated. Please use
REGISTER_MODEL */
CREATE_MODEL(0L, 1L << 18),
- /** The privilege to create a model version */
+ /** The privilege to link a model version */
+ LINK_MODEL_VERSION(0L, 1L << 19),
+ /**
+ * The privilege to create a model version. This is deprecated. Please use
LINK_MODEL_VERSION
+ */
CREATE_MODEL_VERSION(0L, 1L << 19),
Review Comment:
Both `REGISTER_MODEL` and `CREATE_MODEL` share the same bit value `1L <<
18`, and `LINK_MODEL_VERSION` and `CREATE_MODEL_VERSION` share `1L << 19`.
While this appears intentional for backward compatibility (treating the legacy
and new names as equivalent), this design is problematic:
1. It violates the typical enum pattern where each enum value has unique
identifying characteristics
2. The bit fields (highBits/lowBits) become ambiguous - they don't uniquely
identify an enum value
3. If any code attempts to convert from bits to enum name, it will be
ambiguous which enum to return
4. This unconventional approach should be documented with clear comments
explaining the design choice
Consider documenting this design decision inline, or restructure to avoid
enum duplication (e.g., handle legacy names purely through the compatibility
layer without separate enum entries).
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java:
##########
@@ -277,19 +278,45 @@ void testManageRoles() {
SecurableObjects.ofModel(
schemaObjectWithModelPrivs,
"model",
- Lists.newArrayList(Privileges.CreateModelVersion.allow(),
Privileges.UseModel.allow()));
+ Lists.newArrayList(Privileges.LinkModelVersion.allow(),
Privileges.UseModel.allow()));
role =
metalake.createRole(
"model_name", properties,
Lists.newArrayList(modelObjectWithCorrectPriv));
Assertions.assertEquals("model_name", role.name());
Assertions.assertEquals(properties, role.properties());
metalake.deleteRole("model_name");
+ // Test legacy privilege name
+ SecurableObject modelObjectWithLegacyPriv =
+ SecurableObjects.ofModel(
+ schemaObjectWithModelPrivs,
+ "model",
+ Lists.newArrayList(
+ Privileges.CreateModel.allow(),
Privileges.CreateModelVersion.allow()));
Review Comment:
This test incorrectly uses `Privileges.CreateModel.allow()` on a MODEL
object. According to the documentation, `CREATE_MODEL` (now `REGISTER_MODEL`)
should only be applicable to Metalake, Catalog, and Schema - not Model.
Only `CREATE_MODEL_VERSION` (now `LINK_MODEL_VERSION`) should be usable on
MODEL objects. This test currently passes due to the bug in
`CreateModel.canBindTo()` which uses `MODEL_SUPPORTED_TYPES` instead of
`SCHEMA_SUPPORTED_TYPES`.
The test should be updated to only use
`Privileges.CreateModelVersion.allow()` on the MODEL object, not both
`CreateModel` and `CreateModelVersion`.
##########
api/src/main/java/org/apache/gravitino/authorization/Privileges.java:
##########
@@ -899,7 +907,71 @@ public boolean canBindTo(MetadataObject.Type type) {
}
}
+ /** The privilege to link a model version */
+ public static class LinkModelVersion extends
GenericPrivilege<LinkModelVersion> {
+ private static final LinkModelVersion ALLOW_INSTANCE =
+ new LinkModelVersion(Condition.ALLOW, Name.LINK_MODEL_VERSION);
+ private static final LinkModelVersion DENY_INSTANCE =
+ new LinkModelVersion(Condition.DENY, Name.LINK_MODEL_VERSION);
+
+ private LinkModelVersion(Condition condition, Name name) {
+ super(condition, name);
+ }
+
+ /**
+ * @return The instance with allow condition of the privilege.
+ */
+ public static LinkModelVersion allow() {
+ return ALLOW_INSTANCE;
+ }
+
+ /**
+ * @return The instance with deny condition of the privilege.
+ */
+ public static LinkModelVersion deny() {
+ return DENY_INSTANCE;
+ }
+
+ @Override
+ public boolean canBindTo(MetadataObject.Type type) {
+ return MODEL_SUPPORTED_TYPES.contains(type);
+ }
+ }
+
+ /** The privilege to create a model. */
+ @Deprecated
+ public static class CreateModel extends GenericPrivilege<CreateModel> {
+ private static final CreateModel ALLOW_INSTANCE =
+ new CreateModel(Condition.ALLOW, Name.CREATE_MODEL);
+ private static final CreateModel DENY_INSTANCE =
+ new CreateModel(Condition.DENY, Name.CREATE_MODEL);
+
+ private CreateModel(Condition condition, Name name) {
+ super(condition, name);
+ }
+
+ /**
+ * @return The instance with allow condition of the privilege.
+ */
+ public static CreateModel allow() {
+ return ALLOW_INSTANCE;
+ }
+
+ /**
+ * @return The instance with deny condition of the privilege.
+ */
+ public static CreateModel deny() {
+ return DENY_INSTANCE;
+ }
+
+ @Override
+ public boolean canBindTo(MetadataObject.Type type) {
+ return MODEL_SUPPORTED_TYPES.contains(type);
Review Comment:
The deprecated `CreateModel` class uses `MODEL_SUPPORTED_TYPES` in its
`canBindTo()` method, but it should use `SCHEMA_SUPPORTED_TYPES` to match the
behavior of its replacement `RegisterModel`.
According to the documentation, `REGISTER_MODEL` (and thus the legacy
`CREATE_MODEL`) should support Metalake, Catalog, and Schema - not Model. The
new `RegisterModel` class correctly uses `SCHEMA_SUPPORTED_TYPES` at line 877.
```suggestion
return SCHEMA_SUPPORTED_TYPES.contains(type);
```
--
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]
