This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 50c5b9531e [#11135] improvement(authz): Cache parsed OGNL AST to avoid
re-parsing per authorization (#12253)
50c5b9531e is described below
commit 50c5b9531e2138eb3e394aeb73aff63f6e1256b2
Author: YangJie <[email protected]>
AuthorDate: Fri Jul 31 21:32:47 2026 +0800
[#11135] improvement(authz): Cache parsed OGNL AST to avoid re-parsing per
authorization (#12253)
### What changes were proposed in this pull request?
`AuthorizationExpressionEvaluator` stored the converted OGNL expression
as a `String` and evaluated it via `Ognl.getValue(String, ctx)`, which
re-parses the expression into an AST on every call. This PR parses the
expression once at construction and caches the AST in a process-level
`ConcurrentHashMap` keyed by the converted OGNL string, then evaluates
with `Ognl.getValue(tree, ctx)`.
- The cache keys all originate from compile-time constants in
`AuthorizationExpressionConstants` and `@AuthorizationExpression`
annotations, so the map is naturally bounded and needs no eviction.
- Semantics are preserved: `getValue(String, ctx)` already parses and
then evaluates against the same context-as-root, so passing the
pre-parsed tree only skips the re-parse. Verified against the OGNL 3.4.7
bytecode.
- The shared tree is evaluated concurrently
(`MetadataAuthzHelper.doFilter` runs on a thread pool). This is safe:
OGNL only memoizes context-independent constant nodes during evaluation,
every thread writes the same immutable value, publication is guarded by
a volatile flag, and each evaluation builds its own `OgnlContext`.
- Invalid expressions now fail fast at construction instead of on first
evaluation.
### Why are the changes needed?
On every authorized request the evaluator is constructed and evaluated,
and in the list-filter fallback (`MetadataAuthzHelper.doFilter`) once
per listed object, so the repeated parse cost scales with request volume
and namespace size. Caching the parsed AST removes the redundant parse
from these hot paths.
Fix: #11135
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added unit tests in `TestAuthorizationExpressionEvaluator`:
- parsed AST is reused across evaluators for the same expression, and
differs for different expressions;
- concurrent evaluation on a shared AST (16 threads x 200 iterations)
produces correct allow/deny results with no error;
- an invalid expression fails fast at construction with the
`OgnlException` cause preserved.
Existing allow/deny/owner regression tests continue to guard evaluation
semantics. Ran `./gradlew :server-common:test --tests
"*AuthorizationExpressionEvaluator*"`, plus `spotlessApply` and
`javadoc`.
---
.../AuthorizationExpressionEvaluator.java | 67 ++++++++++-
.../TestAuthorizationExpressionEvaluator.java | 127 +++++++++++++++++++++
2 files changed, 188 insertions(+), 6 deletions(-)
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
index 59d5375774..0311de01f0 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionEvaluator.java
@@ -17,12 +17,14 @@
package org.apache.gravitino.server.authorization.expression;
+import com.google.common.annotations.VisibleForTesting;
import java.security.Principal;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
import ognl.Ognl;
import ognl.OgnlContext;
import ognl.OgnlException;
@@ -40,12 +42,28 @@ import org.slf4j.LoggerFactory;
/** Evaluate the runtime result of the AuthorizationExpression. */
public class AuthorizationExpressionEvaluator {
- private final String ognlAuthorizationExpression;
- private final GravitinoAuthorizer authorizer;
-
private static final Logger LOGGER =
LoggerFactory.getLogger(AuthorizationExpressionEvaluator.class);
+ /**
+ * Caches the OGNL AST parsed from an authorization expression string, so a
single parsed tree can
+ * be reused across evaluators and threads instead of re-parsing on every
{@link #evaluate} call.
+ *
+ * <p>The shared tree is evaluated concurrently (see {@code
MetadataAuthzHelper.doFilter}, which
+ * evaluates on a thread pool). This is safe even though OGNL nodes lazily
memoize constant
+ * subexpressions during evaluation: only context-independent nodes are
folded, every thread
+ * writes the same immutable value, the write is published through a
volatile flag, and each
+ * evaluation builds its own {@link OgnlContext}, so no per-request state is
shared.
+ *
+ * <p>The keys are the converted OGNL expression strings, which all
originate from compile-time
+ * constants in {@link AuthorizationExpressionConstants} and {@code
@AuthorizationExpression}
+ * annotations, so the cache is naturally bounded.
+ */
+ private static final Map<String, Object> PARSED_EXPRESSION_CACHE = new
ConcurrentHashMap<>();
+
+ private final Object ognlAuthorizationExpressionTree;
+ private final GravitinoAuthorizer authorizer;
+
/**
* Use {@link AuthorizationExpressionConverter} to convert the authorization
expression into an
* OGNL expression, and then call {@link GravitinoAuthorizer} to perform
permission verification.
@@ -64,11 +82,31 @@ public class AuthorizationExpressionEvaluator {
* @param authorizer GravitinoAuthorizer instance
*/
public AuthorizationExpressionEvaluator(String expression,
GravitinoAuthorizer authorizer) {
- this.ognlAuthorizationExpression =
- AuthorizationExpressionConverter.convertToOgnlExpression(expression);
+ this.ognlAuthorizationExpressionTree =
+
parseExpression(AuthorizationExpressionConverter.convertToOgnlExpression(expression));
this.authorizer = authorizer;
}
+ /**
+ * Parses the OGNL expression into an AST, reusing the cached tree when the
same expression was
+ * parsed before.
+ *
+ * @param ognlExpression the converted OGNL expression string
+ * @return the parsed OGNL AST, shared across evaluators for the same
expression
+ */
+ private static Object parseExpression(String ognlExpression) {
+ return PARSED_EXPRESSION_CACHE.computeIfAbsent(
+ ognlExpression,
+ expression -> {
+ try {
+ return Ognl.parseExpression(expression);
+ } catch (OgnlException e) {
+ throw new RuntimeException(
+ "Failed to parse OGNL authorization expression: " +
expression, e);
+ }
+ });
+ }
+
/**
* Use OGNL expressions to invoke GravitinoAuthorizer for authorizing
multiple types of metadata
* IDs.
@@ -155,7 +193,7 @@ public class AuthorizationExpressionEvaluator {
ognlContext.put(
"METALAKE_NAME",
Optional.ofNullable(nameIdentifier).map(NameIdentifier::name).orElse(""));
try {
- return (boolean) Ognl.getValue(ognlAuthorizationExpression, ognlContext);
+ return (boolean) Ognl.getValue(ognlAuthorizationExpressionTree,
ognlContext);
} catch (OgnlException e) {
throw new RuntimeException(e);
}
@@ -165,4 +203,21 @@ public class AuthorizationExpressionEvaluator {
return Arrays.stream(MetadataObject.Type.values())
.anyMatch(e -> Objects.equals(e.name(), type.name()));
}
+
+ /**
+ * Returns the parsed OGNL AST backing this evaluator, for tests asserting
that the same
+ * expression reuses one cached tree.
+ *
+ * @return the parsed OGNL AST
+ */
+ @VisibleForTesting
+ Object getOgnlAuthorizationExpressionTree() {
+ return ognlAuthorizationExpressionTree;
+ }
+
+ /** Clears the parsed-expression cache so tests can run in isolation. */
+ @VisibleForTesting
+ static void clearParsedExpressionCache() {
+ PARSED_EXPRESSION_CACHE.clear();
+ }
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
index ed3f291dc8..03f2549933 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/expression/TestAuthorizationExpressionEvaluator.java
@@ -23,6 +23,15 @@ import static org.mockito.Mockito.when;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import ognl.OgnlException;
import org.apache.gravitino.Entity;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.UserPrincipal;
@@ -32,12 +41,18 @@ import
org.apache.gravitino.server.authorization.MockGravitinoAuthorizer;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
/** Test for {@link AuthorizationExpressionEvaluator} */
public class TestAuthorizationExpressionEvaluator {
+ @BeforeEach
+ public void clearCache() {
+ AuthorizationExpressionEvaluator.clearParsedExpressionCache();
+ }
+
@Test
public void testEvaluator() {
String expression =
@@ -109,4 +124,116 @@ public class TestAuthorizationExpressionEvaluator {
metadataNames, new AuthorizationRequestContext()));
}
}
+
+ @Test
+ public void testParsedExpressionTreeIsReused() {
+ String expression = "CATALOG::USE_CATALOG && SCHEMA::USE_SCHEMA";
+ try (MockedStatic<GravitinoAuthorizerProvider> mockStatic =
+ mockStatic(GravitinoAuthorizerProvider.class)) {
+ GravitinoAuthorizerProvider mockedProvider =
mock(GravitinoAuthorizerProvider.class);
+
mockStatic.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider);
+ when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new
MockGravitinoAuthorizer());
+
+ AuthorizationExpressionEvaluator first = new
AuthorizationExpressionEvaluator(expression);
+ AuthorizationExpressionEvaluator second = new
AuthorizationExpressionEvaluator(expression);
+ Assertions.assertSame(
+ first.getOgnlAuthorizationExpressionTree(),
+ second.getOgnlAuthorizationExpressionTree(),
+ "The same expression should reuse one cached parsed AST");
+
+ AuthorizationExpressionEvaluator other =
+ new AuthorizationExpressionEvaluator("CATALOG::USE_CATALOG");
+ Assertions.assertNotSame(
+ first.getOgnlAuthorizationExpressionTree(),
+ other.getOgnlAuthorizationExpressionTree(),
+ "Different expressions should not share a parsed AST");
+ }
+ }
+
+ @Test
+ public void testConcurrentEvaluationOnSharedTree() throws Exception {
+ String expression =
+ "CATALOG::USE_CATALOG && SCHEMA::USE_SCHEMA && (TABLE::SELECT_TABLE ||
TABLE::MODIFY_TABLE)";
+ // No static mocks here: the evaluate overload takes an explicit principal
and the constructor
+ // an
+ // explicit authorizer, so worker threads never depend on thread-confined
Mockito static mocks.
+ UserPrincipal principal = new UserPrincipal("tester");
+ AuthorizationExpressionEvaluator evaluator =
+ new AuthorizationExpressionEvaluator(expression, new
MockGravitinoAuthorizer());
+
+ int threads = 16;
+ int iterations = 200;
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+ CountDownLatch startGate = new CountDownLatch(1);
+ AtomicInteger failures = new AtomicInteger();
+ AtomicReference<Throwable> firstError = new AtomicReference<>();
+ Future<?>[] results = new Future<?>[threads];
+ for (int t = 0; t < threads; t++) {
+ boolean expectAuthorized = t % 2 == 0;
+ results[t] =
+ pool.submit(
+ () -> {
+ try {
+ startGate.await();
+ for (int i = 0; i < iterations; i++) {
+ boolean actual =
+ evaluator.evaluate(
+ metadataNames(expectAuthorized),
+ new AuthorizationRequestContext(),
+ principal,
+ Optional.empty());
+ if (actual != expectAuthorized) {
+ failures.incrementAndGet();
+ }
+ }
+ } catch (Throwable e) {
+ failures.incrementAndGet();
+ firstError.compareAndSet(null, e);
+ }
+ });
+ }
+ startGate.countDown();
+ for (Future<?> result : results) {
+ result.get(30, TimeUnit.SECONDS);
+ }
+ pool.shutdownNow();
+ Assertions.assertEquals(
+ 0,
+ failures.get(),
+ "Concurrent evaluation on a shared AST produced wrong or failed
results; first error: "
+ + firstError.get());
+ }
+
+ @Test
+ public void testConstructorFailsFastOnInvalidExpression() {
+ try (MockedStatic<AuthorizationExpressionConverter> converterMocked =
+ mockStatic(AuthorizationExpressionConverter.class)) {
+ converterMocked
+ .when(() ->
AuthorizationExpressionConverter.convertToOgnlExpression("BAD"))
+ .thenReturn("a b c ((");
+ RuntimeException e =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> new AuthorizationExpressionEvaluator("BAD", new
MockGravitinoAuthorizer()));
+ Assertions.assertInstanceOf(OgnlException.class, e.getCause());
+ }
+ }
+
+ private static Map<Entity.EntityType, NameIdentifier> metadataNames(boolean
authorized) {
+ Map<Entity.EntityType, NameIdentifier> metadataNames = new HashMap<>();
+ metadataNames.put(Entity.EntityType.METALAKE,
NameIdentifierUtil.ofMetalake("testMetalake"));
+ metadataNames.put(
+ Entity.EntityType.CATALOG,
NameIdentifierUtil.ofCatalog("testMetalake", "testCatalog"));
+ metadataNames.put(
+ Entity.EntityType.SCHEMA,
+ NameIdentifierUtil.ofSchema("testMetalake", "testCatalog",
"testSchema"));
+ metadataNames.put(
+ Entity.EntityType.TABLE,
+ NameIdentifierUtil.ofTable(
+ "testMetalake",
+ "testCatalog",
+ "testSchema",
+ authorized ? "testTable" : "testTableHasNotPermission"));
+ return metadataNames;
+ }
}
