yuqi1129 commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4229852117
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizationLookups.java:
##########
@@ -74,34 +67,28 @@ public JcasbinAuthorizationLookups(
* Optional#empty()} when the metadata object does not exist so callers can
deny authorization.
* Missing metadata objects are never cached as negative results: a later
create for the same name
* can be observed without waiting for cache eviction. Existing objects are
invalidated by local
- * name-id mapping hooks and by the change-log poller on peer nodes.
+ * name-id mapping hooks and by the change-log poller on peer nodes. Both
cache tiers use names
+ * normalized by catalog capability. Normalization is deduplicated per raw
name within a request;
+ * a fresh request still resolves current catalog rules before consulting
the shared cache.
*/
public Optional<Long> resolveMetadataId(
MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
try {
- MetadataObject cacheObject = metadataObject;
- // Hooks and change-log records use normalized parents. Cache the same
key for every
- // equivalent request spelling, while preserving the Gravitino-owned
model leaf.
- if (metadataObject.type() == MetadataObject.Type.SEMANTIC_MODEL) {
- NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Capability capability =
- CapabilityHelpers.getCapability(ident,
GravitinoEnv.getInstance().catalogManager());
- NameIdentifier normalized =
- NameIdentifier.of(
- CapabilityHelpers.applyCaseSensitive(
- ident.namespace(), Capability.Scope.SEMANTIC_MODEL,
capability),
- ident.name());
- cacheObject =
- NameIdentifierUtil.toMetadataObject(normalized,
Entity.EntityType.SEMANTIC_MODEL);
- }
+ // Use the same capability rules as ID resolution so hooks and peer
change-log replay
+ // evict every equivalent spelling from both cache tiers.
+ MetadataObject cacheObject =
+ requestContext.computeNormalizedMetadataObjectIfAbsent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake,
metadataObject),
+ ignored ->
MetadataIdConverter.normalizeMetadataObject(metadataObject, metalake));
Review Comment:
Fixed in c706a23259. A non-`NotFoundException` from capability resolution
now leaves that object unresolved and fails closed instead of escaping as a
500; entity-store failures still propagate. Plain `Optional.empty()` alone
would let an unresolved DENY drop out of role loading, so the lookup keeps a
separate normalization-failed outcome: the deny check rejects (using the
existing `unstablePolicyResult` fail-closed value), and role loading installs
the same DENY privileges on the object's catalog until the partial-role retry
resolves the precise object. Tests cover healthy catalogs next to a failing
one, the catalog guard and its removal after retry, and an already-loaded DENY;
removing the guard or failing open in the deny check fails them. I did not add
a capability TTL cache: it would not help a catalog that never initialized.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizationLookups.java:
##########
@@ -74,34 +67,28 @@ public JcasbinAuthorizationLookups(
* Optional#empty()} when the metadata object does not exist so callers can
deny authorization.
* Missing metadata objects are never cached as negative results: a later
create for the same name
* can be observed without waiting for cache eviction. Existing objects are
invalidated by local
- * name-id mapping hooks and by the change-log poller on peer nodes.
+ * name-id mapping hooks and by the change-log poller on peer nodes. Both
cache tiers use names
+ * normalized by catalog capability. Normalization is deduplicated per raw
name within a request;
+ * a fresh request still resolves current catalog rules before consulting
the shared cache.
*/
public Optional<Long> resolveMetadataId(
MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
try {
- MetadataObject cacheObject = metadataObject;
- // Hooks and change-log records use normalized parents. Cache the same
key for every
- // equivalent request spelling, while preserving the Gravitino-owned
model leaf.
- if (metadataObject.type() == MetadataObject.Type.SEMANTIC_MODEL) {
- NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Capability capability =
- CapabilityHelpers.getCapability(ident,
GravitinoEnv.getInstance().catalogManager());
- NameIdentifier normalized =
- NameIdentifier.of(
- CapabilityHelpers.applyCaseSensitive(
- ident.namespace(), Capability.Scope.SEMANTIC_MODEL,
capability),
- ident.name());
- cacheObject =
- NameIdentifierUtil.toMetadataObject(normalized,
Entity.EntityType.SEMANTIC_MODEL);
- }
+ // Use the same capability rules as ID resolution so hooks and peer
change-log replay
+ // evict every equivalent spelling from both cache tiers.
+ MetadataObject cacheObject =
+ requestContext.computeNormalizedMetadataObjectIfAbsent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake,
metadataObject),
+ ignored ->
MetadataIdConverter.normalizeMetadataObject(metadataObject, metalake));
String cacheKey =
JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake, cacheObject);
// Both cache tiers load atomically and forbid caching null, so a
missing object is signalled
// by throwing through the loaders and translated back to
Optional.empty() here. This caches
- // only positive results, never a negative one.
+ // only positive results, never a negative one. Load the same canonical
name as the key,
+ // rather than applying the request spelling again in the loader.
return Optional.of(
requestContext.computeMetadataIdIfAbsent(
cacheKey,
- k -> metadataIdCache.get(k, ignored ->
loadMetadataId(metadataObject, metalake))));
+ k -> metadataIdCache.get(k, ignored ->
loadMetadataId(cacheObject, metalake))));
Review Comment:
Fixed in c706a23259. The miss loader now calls
`MetadataIdConverter.getIdForNormalizedObject` with the same canonical object
as the cache key, which shares the entity-store loading with `getID` but does
not resolve capabilities again.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizationLookups.java:
##########
@@ -45,18 +45,17 @@ public class TestJcasbinAuthorizationLookups {
@Test
void testResolveMetadataIdUsesAtomicSharedCacheAndRequestDedup() {
- MetadataObject table =
- MetadataObjects.of(Arrays.asList("cat1", "sch1", "tbl1"),
MetadataObject.Type.TABLE);
+ MetadataObject catalog = MetadataObjects.of(Arrays.asList("cat1"),
MetadataObject.Type.CATALOG);
Review Comment:
Fixed in c706a23259. Kept the CATALOG fixture and added
`testNormalizedTableUsesAtomicSharedCacheAndRequestDedup`: `cat1.SCH1.TBL1` in
a case-insensitive catalog, asserting the canonical key, one shared-cache
`get`, no `getIfPresent`/`put`, and request dedup.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataIdConverter.java:
##########
@@ -95,14 +99,66 @@ public static Optional<Long> getID(MetadataObject
metadataObject, String metalak
return Optional.of(extractIdFromEntity(entity));
}
+ /**
+ * Normalizes a metadata object's name using the same catalog rules as ID
resolution.
+ *
+ * <p>Types without a catalog capability scope retain their names. Semantic
model leaves remain
+ * case sensitive, while column names and their table/schema parents follow
their own scopes.
+ *
+ * @param metadataObject the object whose name will be normalized
+ * @param metalake the metalake name
+ * @return the normalized object, suitable for name-to-ID cache keys
+ * @throws NotFoundException if the containing catalog does not exist
+ */
+ public static MetadataObject normalizeMetadataObject(
+ MetadataObject metadataObject, String metalake) {
+ Preconditions.checkArgument(metadataObject != null, "Metadata object
cannot be null");
+ Capability.Scope scope = METADATA_SCOPE_MAPPING.get(metadataObject.type());
Review Comment:
Included in this PR after all, in d3bda2475a. VIEW and FUNCTION now map to
their capability scopes, matching what
`ViewNormalizeDispatcher`/`FunctionNormalizeDispatcher` store.
`EntityClassMapper` lacked both types (the store ignores the class argument, so
production was unaffected), so I registered them. The mapping-derived suites
now cover both types, and an explicit test fails if either mapping is removed.
--
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]
