jerryshao commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4228006823
##########
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:
[Nit] Switching this fixture from `TABLE` to `CATALOG` removes normalization
from the one test that asserts shared-cache atomicity and request dedup.
`CATALOG` is not in `METADATA_SCOPE_MAPPING`, so `normalizeMetadataObject`
returns at its `scope == null` early exit and the test never reaches a
capability lookup — which is presumably why the change was made, since this
test has no `CatalogManager` mock. The effect is that `getCount == 1` /
`getIfPresentCount == 0` is now only proven for a type that bypasses the new
code. The realistic shape (a `TABLE` whose raw and canonical keys differ, where
one must confirm the shared cache is still consulted exactly once under the
canonical key) is no longer covered here.
`TestJcasbinMetadataIdCacheNormalization` covers normalization and
`testLoaderUsesTheCanonicalNameOfItsCacheKey` covers the loader input, so the
gap is narrow — but keeping a `TABLE` case with the same `CatalogManager` stub
the new suite already builds would restore it cheaply.
Verified by: reading this test before and after via `git diff
origin/main...HEAD` on this file, and confirming `CATALOG` is absent from
`METADATA_SCOPE_MAPPING` (`MetadataIdConverter.java:50-58`) so
`normalizeMetadataObject` returns at line 117-119.
##########
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:
[Important] A capability lookup now runs on every metadata-ID lookup,
including shared-cache hits, so a catalog whose connector cannot initialize
will fail authorization requests that previously served from cache.
Not the throughput point Copilot raised in #discussion_r4217217323
(addressed by the request-local memo) but the failure mode, which memoization
cannot help: the failure recurs every fresh request.
Chain I traced:
- `CapabilityHelpers.getCapability`
(`core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:55-64`)
rethrows `NoSuchCatalogException` but wraps *every other* `RuntimeException`
as a bare `RuntimeException("Failed to get capabilities for catalog: ...")`.
- `resolveMetadataId` only catches `NotFoundException` (line 92 of this
file). `NoSuchCatalogException extends NotFoundException`; the wrapped one does
not, so it propagates.
- `doWithCatalog` leases through `catalogCache.get(ident,
this::loadCatalogInternal)` (`CatalogManager.java:815-826`, `:1644`), so a
catalog whose connector constructor fails (metastore unreachable, bad
credentials) throws on each attempt.
Before this PR, the non-`SEMANTIC_MODEL` path reached
`metadataIdCache.get(key, loader)` with a raw key and touched `CatalogManager`
only on a miss, so a long-lived cached ID kept authorizing through a sick
catalog. Now every call needs the capability first.
Worst case is role policy loading: `resolveRolePolicies`
(`JcasbinAuthorizer.java:1631-1633`) has an explicit `unresolvedObjects`
channel so a vanished object degrades gracefully, but this exception bypasses
it and escapes `loadRole`, which rethrows as `RuntimeException`
(`AuthorizationRequestContext.java:162-168`). One unhealthy catalog referenced
by a role therefore aborts the whole request, including objects in healthy
catalogs.
Suggested fix: treat a non-`NotFoundException` capability failure as a
normalization failure here and fail closed for that object (`Optional.empty()`)
instead of letting it escape as a 500. A short-TTL capability cache keyed by
`(metalake, catalog)` would also cover it, since `Capability` depends on the
catalog's provider rather than connector liveness.
Verified by: reading `CapabilityHelpers.getCapability` and
`CatalogManager.doWithCatalog`/`loadCatalogAndWrapInternal` in this checkout;
`git show origin/main` of this file calls `getCapability` only for
`SEMANTIC_MODEL`; `TestJcasbinMetadataIdCacheNormalization:392` only exercises
`NoSuchCatalogException`.
##########
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:
[Nit] On a shared-cache miss the capability is resolved twice for the same
object.
`cacheObject` is already canonical, but `loadMetadataId` calls
`MetadataIdConverter.getID`, which runs `normalizeCaseSensitive(ident, scope,
catalogManager)` again (`MetadataIdConverter.java:79-80`) and so issues a
second `getCapability` -> `doWithCatalog` round trip. Normalizing an
already-canonical name is idempotent, so this is correctness-neutral; it just
doubles the catalog work on the miss path, which is the path the new comment
above is trying to tighten.
If worth tidying, an overload of `getID` that takes a pre-normalized
identifier (or a package-private `getIdForNormalizedObject`) would skip it. Not
blocking.
Verified by: reading `getID` lines 69-100 of `MetadataIdConverter.java` in
this checkout and following `normalizeCaseSensitive` -> `normalizeIdentifier`
-> `CapabilityHelpers.getCapability`.
##########
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:
[Question] `METADATA_SCOPE_MAPPING` has no entry for `VIEW` or `FUNCTION`,
so those two types still key and load on the raw spelling. Is that deliberate
for this PR?
Both are first-class authorization types, not virtual ones:
`metadataIdCacheKey` gives `VIEW` and `FUNCTION` their own branch
(`JcasbinAuthorizationCacheKeys.java:61-68`),
`MetadataObjectUtil.toEntityIdent` maps both (`:129-140`), `Capability.Scope`
has both, `hasSchemaParent` returns `true` for both
(`CapabilityHelpers.java:249-256`), and `VIEW` appears in
`AuthorizationUtils.SKIP_APPLY_TYPES` (`:102`) only because there is no plugin
push-down — it is still a securable type.
`JcasbinChangeListener.VIRTUAL_NAMESPACE_TYPES` (`:70-75`) excludes neither.
With `scope == null` the method returns early, so for a case-insensitive
catalog a request for `cat.DB.MY_VIEW` keys raw and `getID` loads raw. The
symptom differs from the `TABLE` bug in #13694: nothing positive gets cached,
so there is no stale ID — instead the lookup consistently misses the store and
denies, and the listener's eviction of `cat.db.my_view` never matches the raw
key.
This predates the PR (`getID` used the same map), so it may well be out of
scope. Raising it because the PR describes keys as using "the same
normalization implementation as ID resolution" and the test suite derives its
type coverage from this map, so the two types that the key factory handles but
the map omits are invisible to both the claim and the tests. A line in the
description or a follow-up issue would settle it.
Verified by: reading `METADATA_SCOPE_MAPPING` (lines 50-58) against the
`VIEW`/`FUNCTION` branches in
`JcasbinAuthorizationCacheKeys.metadataIdCacheKey`,
`MetadataObjectUtil.toEntityIdent`, `CapabilityHelpers.hasSchemaParent` and
`JcasbinChangeListener.VIRTUAL_NAMESPACE_TYPES` in this checkout.
--
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]
