Copilot commented on code in PR #10119:
URL: https://github.com/apache/gravitino/pull/10119#discussion_r2888978426
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -517,11 +494,8 @@ private void loadRolePrivilege(
});
}
- private void loadOwnerPolicy(String metalake, MetadataObject metadataObject,
Long metadataId) {
- if (ownerRel.getIfPresent(metadataId) != null) {
- LOG.debug("Metadata {} OWNER has been loaded.", metadataId);
- return;
- }
+ private boolean checkOwner(String metalake, MetadataObject metadataObject,
String userName) {
+
Review Comment:
There is an unnecessary blank line (line 498) between the opening brace of
`checkOwner` and its body. Google Java Style Guide does not allow blank lines
at the start of a method body. This line should be removed.
```suggestion
```
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -36,7 +36,6 @@
import java.io.IOException;
import java.lang.reflect.Field;
import java.security.Principal;
-import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
Review Comment:
The `import java.util.Optional;` import was only needed by the removed
`getOwnerRelCache` helper method and the deleted test methods
(`testOwnerCacheInvalidation`, `testAuthorizeByOwner`). Since all usages are
now gone, this import is unused and should be removed. Additionally, the
`getOwnerRelCache` helper method still exists in the file (around line 422–428)
even though the `ownerRel` field was removed from `JcasbinAuthorizer`. That
dead method should be removed as well to avoid a `NoSuchFieldException` if it
were ever accidentally invoked.
```suggestion
```
##########
docs/security/access-control.md:
##########
@@ -456,10 +455,8 @@ Gravitino uses Caffeine caches to improve authorization
performance by caching r
- **`roleCacheSize`**: Controls the maximum number of role entries that can be
cached. When the cache reaches this size, the least recently used entries are
evicted.
-- **`ownerCacheSize`**: Controls the maximum number of owner relationship
entries that can be cached. This cache maps metadata object IDs to their owner
IDs.
-
:::info
-When role privileges or ownership are changed through the Gravitino API, the
corresponding cache entries are automatically invalidated to ensure
authorization decisions reflect the latest state.
+When role privileges are changed through the Gravitino API, the corresponding
cache entries are automatically invalidated to ensure authorization decisions
reflect the latest state.
Review Comment:
The nearby description text on line 452 (which wasn't changed in this PR)
still reads: "Gravitino uses Caffeine caches to improve authorization
performance by caching role and **owner** information." Since the owner cache
has been removed by this PR, this sentence is now outdated and should be
updated to remove "and owner" so it reads: "by caching role information".
--
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]
