yuqi1129 commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4228543959
##########
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:
Confirmed: this is still unresolved on the published head 764d6b75d. The
request-local memo does not isolate a RuntimeException from capability
resolution on a fresh request, and the current catch only handles
NotFoundException.
A local candidate handles normalization failures without falling back to raw
cache keys, while preserving entity-store failures. It also retains a distinct
failure outcome for DENY consumers: simply skipping an unresolved DENY could
allow an ancestor or another role's ALLOW to bypass it. The candidate covers
healthy/failing catalogs, already-loaded DENY policies, conservative protection
during partial role loading, and recovery.
That candidate has not been committed or pushed, so this comment remains
open; I am not claiming the current PR head fixes it.
##########
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:
Yes, VIEW and FUNCTION retain their pre-existing raw-name behavior. I will
not add those scope mappings in this PR: that would expand ID-resolution
behavior beyond the existing catalog-scoped mapping addressed by #13694 and
needs its own behavior and invalidation coverage. This does not mean they are
virtual or non-securable types.
I have updated the PR description to name both exclusions explicitly. The
mapped-type tests cover the seven existing mapped types; they do not establish
VIEW/FUNCTION normalization coverage. No follow-up issue has been created.
##########
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:
Confirmed. On published head 764d6b75d the miss loader still calls getID,
which repeats capability resolution for the already-canonical object. This is
not yet fixed in the published PR.
The local candidate adds a documented getIdForNormalizedObject entry point
and shares the entity-store loading implementation with getID, so the canonical
cache key and loader use the same object without a second capability lookup.
Lookup-count regression tests cover the miss path. The candidate is not
committed or pushed yet; leaving this thread open until it is published.
##########
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:
Agreed. The CATALOG fixture verifies the cache API behavior but bypasses
normalization, so it does not prove the TABLE case you described. This remains
a coverage gap on published head 764d6b75d.
The local candidate keeps the existing fixture and adds a case-insensitive
TABLE test using cat1.SCH1.TBL1 and its canonical spelling. It checks the
actual canonical cache key, one shared-cache get, no getIfPresent/put calls,
and request deduplication. That test is not committed or pushed yet, so I am
not marking this comment resolved.
--
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]
