Copilot commented on code in PR #6744:
URL: https://github.com/apache/hive/pull/6744#discussion_r4003719746
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java:
##########
@@ -140,10 +140,9 @@ public final List<String> filterDatabases(String catName,
List<String> list) thr
@Override
public final Database filterDatabase(Database database) throws
MetaException, NoSuchObjectException {
if (database != null) {
- String dbName = database.getName();
- List<String> databases = filterDatabases(database.getCatalogName(),
Collections.singletonList(dbName));
+ List<Database> databases =
filterDatabaseObjects(Collections.singletonList(database));
if (databases.isEmpty()) {
- throw new NoSuchObjectException(String.format("Database %s does not
exist", dbName));
+ throw new NoSuchObjectException(String.format("Database %s does not
exist", database.getName()));
}
}
return database;
Review Comment:
The new recursion-avoidance path is in `filterDatabase(...)`, but the added
test only exercises `getAllDatabases()` (i.e., `filterDatabases(...)`). Add a
focused test that calls `client.getDatabase(\"default\")` (or similar) with the
filter hook enabled to cover `filterDatabase(...)` and prevent regressions
related to the `StackOverflowError` scenario described in the PR.
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/metastore/filtercontext/DatabaseFilterContext.java:
##########
@@ -89,7 +89,7 @@ private List<HivePrivilegeObject> getInputHObjs() {
if (db != null) {
ret.add(getHivePrivilegeObject(db));
} else {
- HivePrivilegeObject hivePrivilegeObject = new
HivePrivilegeObject(type, dbName);
+ HivePrivilegeObject hivePrivilegeObject = new
HivePrivilegeObject(type, catName, dbName, dbName);
Review Comment:
For a DATABASE privilege object, passing `dbName` as both the database name
and the object name is likely incorrect and can confuse downstream authorizers
(treating it like a table/object within the DB). Prefer constructing the
`HivePrivilegeObject` so that the DB name is populated, but the object/table
component is unset (null/empty) or use a database-specific constructor/factory
if available.
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java:
##########
@@ -140,10 +140,9 @@ public final List<String> filterDatabases(String catName,
List<String> list) thr
@Override
public final Database filterDatabase(Database database) throws
MetaException, NoSuchObjectException {
if (database != null) {
- String dbName = database.getName();
- List<String> databases = filterDatabases(database.getCatalogName(),
Collections.singletonList(dbName));
+ List<Database> databases =
filterDatabaseObjects(Collections.singletonList(database));
if (databases.isEmpty()) {
Review Comment:
The variable name `databases` is ambiguous here because it represents the
*filtered result* of `filterDatabaseObjects(...)`. Renaming it to something
like `filteredDatabases` would make the intent clearer and reduce confusion
with other `databases` variables/lists in the authorizer.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
