Copilot commented on code in PR #9290:
URL: https://github.com/apache/gravitino/pull/9290#discussion_r2571547713
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -285,7 +286,7 @@ public Response listMetadataObjectsForTag(
MetadataObjectDTO[] objectDTOs =
Arrays.stream(objects).map(DTOConverters::toDTO).toArray(MetadataObjectDTO[]::new);
- // TODO filter by can-access-metadata, MetadataFilterHelper can
not support now
+ objectDTOs = MetadataAuthzHelper.filterMetadataObject(metalake,
objectDTOs);
Review Comment:
The new metadata object filtering logic in `listMetadataObjectsForTag` lacks
direct unit test coverage. While integration tests exist in
`TagOperationsAuthorizationIT.java`, consider adding unit tests to verify the
filtering behavior in isolation, particularly for edge cases like empty arrays
or null handling.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java:
##########
@@ -314,9 +314,8 @@ public Response listMetadataObjectsForPolicy(
() -> {
MetadataObject[] objects =
policyDispatcher.listMetadataObjectsForPolicy(metalake,
policyName);
- // TODO filter by can-access-metadata, MetadataFilterHelper can
not support now
objects = objects == null ? new MetadataObject[0] : objects;
-
+ objects = MetadataAuthzHelper.filterMetadataObject(metalake,
objects);
Review Comment:
The filtering logic in `listMetadataObjectsForPolicy` is applied after
null-checking but before DTO conversion. Add unit tests to verify this ordering
and ensure filtering correctly handles the pre-DTO conversion objects array.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -213,6 +237,26 @@ private static <E> E[] doFilter(
GravitinoAuthorizer authorizer,
AuthorizationRequestContext authorizationRequestContext,
Function<E, Map<Entity.EntityType, NameIdentifier>>
extractMetadataNamesMap) {
+ return doFilter(
+ expression,
+ entities,
+ authorizer,
+ authorizationRequestContext,
+ extractMetadataNamesMap,
+ (any) -> null);
Review Comment:
[nitpick] The lambda parameter name 'any' is vague and doesn't clearly
indicate its purpose. Consider renaming it to 'entity' or 'unused' to better
convey that this is a no-op function for extracting entity type.
```suggestion
(unused) -> null);
```
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -108,6 +105,37 @@ public static Metalake[] filterMetalakes(Metalake[]
metalakes, String expression
NameIdentifierUtil.ofMetalake(metalakeName));
});
}
+
+ public static MetadataObjectDTO[] filterMetadataObject(
+ String metalake, MetadataObjectDTO[] metadataObjects) {
+ return doFilter(
+ AuthorizationExpressionConstants.CAN_ACCESS_METADATA,
+ metadataObjects,
+ GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer(),
+ new AuthorizationRequestContext(),
+ metadataObject ->
+ splitMetadataNames(
+ metalake,
+ MetadataObjectUtil.toEntityType(metadataObject.type()),
+ MetadataObjectUtil.toEntityIdent(metalake, metadataObject)),
+ metadataObject ->
MetadataObjectUtil.toEntityType(metadataObject.type()));
+ }
Review Comment:
The two `filterMetadataObject` methods (lines 109-122 and 124-137) contain
nearly identical logic with only the input type differing. Consider extracting
the common logic into a shared private method or using generics to reduce code
duplication and improve maintainability.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -213,6 +237,26 @@ private static <E> E[] doFilter(
GravitinoAuthorizer authorizer,
AuthorizationRequestContext authorizationRequestContext,
Function<E, Map<Entity.EntityType, NameIdentifier>>
extractMetadataNamesMap) {
+ return doFilter(
+ expression,
+ entities,
+ authorizer,
+ authorizationRequestContext,
+ extractMetadataNamesMap,
+ (any) -> null);
+ }
+
+ private static <E> E[] doFilter(
+ String expression,
+ E[] entities,
+ GravitinoAuthorizer authorizer,
+ AuthorizationRequestContext authorizationRequestContext,
+ Function<E, Map<Entity.EntityType, NameIdentifier>>
extractMetadataNamesMap,
+ Function<E, Entity.EntityType> extractEntityType) {
Review Comment:
The overloaded `doFilter` method with the new `extractEntityType` parameter
lacks documentation explaining its purpose and when it should be used versus
the simpler version. Add a Javadoc comment describing the purpose of the
`extractEntityType` parameter and how it differs from the existing method.
--
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]
