Copilot commented on code in PR #12793:
URL: https://github.com/apache/gravitino/pull/12793#discussion_r3905254754
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -507,18 +505,19 @@ public static void authorizationPluginRemovePrivileges(
}
}
- public static void removeCatalogPrivileges(Catalog catalog, List<String>
locations) {
+ public static void removeCatalogPrivileges(NameIdentifier catalogIdent,
List<String> locations) {
// If we enable authorization, we should remove the privileges about the
entity in the
// authorization plugin.
MetadataObject metadataObject =
- MetadataObjects.of(null, catalog.name(), MetadataObject.Type.CATALOG);
+ MetadataObjects.of(null, catalogIdent.name(),
MetadataObject.Type.CATALOG);
MetadataObjectChange removeObject =
MetadataObjectChange.remove(metadataObject, locations);
callAuthorizationPluginImpl(
authorizationPlugin -> {
authorizationPlugin.onMetadataUpdated(removeObject);
},
- catalog);
+ GravitinoEnv.getInstance().catalogManager(),
+ catalogIdent);
}
Review Comment:
`removeCatalogPrivileges` now builds the `MetadataObject` from
`catalogIdent.name()`, which can differ from the effective/normalized catalog
name (`BaseCatalog.name()`). This can cause privilege removals to target the
wrong external-authorization resource (e.g., case normalization) compared to
the prior behavior that used `catalog.name()`. Consider constructing the
`MetadataObjectChange` inside the `doWithCatalog` callback (or otherwise using
the leased catalog’s `catalog.name()`) so the external update uses the
canonical name.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -645,26 +664,21 @@ private static void checkCatalogType(
}
}
- private static List<Catalog> loadMetadataObjectCatalog(
+ private static List<NameIdentifier> getMetadataObjectCatalogs(
String metalake, MetadataObject metadataObject) {
CatalogManager catalogManager =
GravitinoEnv.getInstance().catalogManager();
- List<Catalog> loadedCatalogs = Lists.newArrayList();
+ List<NameIdentifier> catalogIdents = Lists.newArrayList();
if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
NameIdentifier[] catalogs =
catalogManager.listCatalogs(Namespace.of(metalake));
- // ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we
need `BaseCatalog` to
- // call authorization plugin method.
- for (NameIdentifier catalog : catalogs) {
- loadedCatalogs.add(catalogManager.loadCatalog(catalog));
- }
+ catalogIdents.addAll(Arrays.asList(catalogs));
} else if (needApplyAuthorization(metadataObject.type())) {
NameIdentifier catalogIdent =
NameIdentifierUtil.getCatalogIdentifier(
MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
- Catalog catalog = catalogManager.loadCatalog(catalogIdent);
- loadedCatalogs.add(catalog);
+ catalogIdents.add(catalogIdent);
}
- return loadedCatalogs;
+ return catalogIdents;
}
Review Comment:
`getMetadataObjectCatalogs` reaches into `GravitinoEnv` to fetch a
`CatalogManager`, but it only uses it to call `listCatalogs` for the \"all
catalogs\" branch. Since the caller
(`callAuthorizationPluginForMetadataObject`) already has a `CatalogManager`,
consider passing it in as a parameter and removing the internal `GravitinoEnv`
lookup. This reduces hidden dependencies and makes the helper easier to unit
test in isolation.
--
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]
