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 7b6cf6b58c [#12475] improvement(common): make CallerContext an
immutable value object (#12476)
7b6cf6b58c is described below
commit 7b6cf6b58c314216c8f815c18c4177b0e00d238a
Author: YangJie <[email protected]>
AuthorDate: Tue Aug 18 21:48:59 2026 +0800
[#12475] improvement(common): make CallerContext an immutable value object
(#12476)
### What changes are proposed in this pull request?
Make `CallerContext` a truly immutable value object:
- `withContext` now stores a defensive unmodifiable copy
(`Collections.unmodifiableMap(new HashMap<>(context))`) instead of the
caller's map by reference.
- The `context` field is now `final`, assigned once via a private
constructor; the `Builder` accumulates the map and `build()` returns a
distinct `new CallerContext(...)`.
`CallerContext` is published into a `ThreadLocal` and read across
modules (server fileset/credential ops, event dispatcher, GVFS clients).
It is forked from Hadoop's `CallerContext`, which is fully immutable,
but the Gravitino copy had lost that: the map was exposed by reference
and a retained `Builder` could mutate an already-built instance.
### Why are the changes needed?
Defense-in-depth for an audit/credential context: prevent a caller's
later map mutation or a reader's `put`/`remove` from silently corrupting
the published context, and restore parity with the immutable Hadoop
origin. No live bug today (all call sites pass fresh maps and only
read), but a latent footgun on a shared, ThreadLocal-published object.
Fix: #12475
### Does this PR introduce _any_ user-facing change?
`context()` now returns an unmodifiable map. Within Gravitino nothing
breaks (all readers are read-only). External GVFS/client code that
mutated the returned map would now get `UnsupportedOperationException`.
### How was this patch tested?
Extended `TestCallerContext` (1 → 7 tests): defensive copy,
unmodifiability, null-rejection contract, empty map, builder-reuse
isolation, equals/hashCode across a mutated source map. `./gradlew
:common:test --tests "*.audit.TestCallerContext"` passes 7/7.
---
.../org/apache/gravitino/audit/CallerContext.java | 26 ++++----
.../apache/gravitino/audit/TestCallerContext.java | 70 ++++++++++++++++++++++
2 files changed, 85 insertions(+), 11 deletions(-)
diff --git a/common/src/main/java/org/apache/gravitino/audit/CallerContext.java
b/common/src/main/java/org/apache/gravitino/audit/CallerContext.java
index 0ae2250d07..d1324f6014 100644
--- a/common/src/main/java/org/apache/gravitino/audit/CallerContext.java
+++ b/common/src/main/java/org/apache/gravitino/audit/CallerContext.java
@@ -21,6 +21,8 @@ package org.apache.gravitino.audit;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.Map;
/**
@@ -31,12 +33,14 @@ import java.util.Map;
*
<p>hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/CallerContext.java
*/
public class CallerContext {
- private Map<String, String> context;
+ private final Map<String, String> context;
- private CallerContext() {}
+ private CallerContext(Map<String, String> context) {
+ this.context = context;
+ }
/**
- * Returns the context map in the caller context.
+ * Returns the context map in the caller context. The returned map is
unmodifiable.
*
* @return the context map
*/
@@ -59,26 +63,26 @@ public class CallerContext {
/** Builder to create a caller context. */
public static class Builder {
- private final CallerContext callerContext;
+ private Map<String, String> context;
- private Builder() {
- callerContext = new CallerContext();
- }
+ private Builder() {}
/**
- * Sets the context for CallerContext
+ * Sets the context for CallerContext. The provided map is defensively
copied into an
+ * unmodifiable map, so later mutations to the caller's map do not affect
this context and the
+ * map returned by {@link CallerContext#context()} cannot be modified.
*
* @param context The context to set.
* @return This Builder instance for method chaining.
*/
public CallerContext.Builder withContext(Map<String, String> context) {
- callerContext.context = context;
+ this.context = context == null ? null : Collections.unmodifiableMap(new
HashMap<>(context));
return this;
}
/** Validate the variables in the CallerContext. */
private void validate() {
- Preconditions.checkArgument(callerContext.context != null, "context
cannot be null");
+ Preconditions.checkArgument(context != null, "context cannot be null");
}
/**
@@ -88,7 +92,7 @@ public class CallerContext {
*/
public CallerContext build() {
validate();
- return callerContext;
+ return new CallerContext(context);
}
}
diff --git
a/common/src/test/java/org/apache/gravitino/audit/TestCallerContext.java
b/common/src/test/java/org/apache/gravitino/audit/TestCallerContext.java
index e0f3d4d56c..a350d11a94 100644
--- a/common/src/test/java/org/apache/gravitino/audit/TestCallerContext.java
+++ b/common/src/test/java/org/apache/gravitino/audit/TestCallerContext.java
@@ -19,6 +19,7 @@
package org.apache.gravitino.audit;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import org.junit.jupiter.api.Assertions;
@@ -42,4 +43,73 @@ public class TestCallerContext {
CallerContext.CallerContextHolder.remove();
}
}
+
+ @Test
+ public void testContextIsDefensivelyCopied() {
+ Map<String, String> contextMap = new HashMap<>();
+ contextMap.put("test", "test");
+ CallerContext callerContext =
CallerContext.builder().withContext(contextMap).build();
+
+ // Mutating the original map after build must not affect the stored
context.
+ contextMap.put("added-after-build", "value");
+ contextMap.remove("test");
+
+ Assertions.assertEquals(1, callerContext.context().size());
+ Assertions.assertEquals("test", callerContext.context().get("test"));
+
Assertions.assertFalse(callerContext.context().containsKey("added-after-build"));
+ }
+
+ @Test
+ public void testContextIsUnmodifiable() {
+ Map<String, String> contextMap = new HashMap<>();
+ contextMap.put("test", "test");
+ CallerContext callerContext =
CallerContext.builder().withContext(contextMap).build();
+
+ Assertions.assertThrows(
+ UnsupportedOperationException.class, () ->
callerContext.context().put("k", "v"));
+ }
+
+ @Test
+ public void testNullContextStillRejectedOnBuild() {
+ CallerContext.Builder builder = CallerContext.builder().withContext(null);
+ Assertions.assertThrows(IllegalArgumentException.class, builder::build);
+ }
+
+ @Test
+ public void testEmptyContextIsAllowed() {
+ CallerContext callerContext = CallerContext.builder().withContext(new
HashMap<>()).build();
+ Assertions.assertTrue(callerContext.context().isEmpty());
+ }
+
+ @Test
+ public void testReusingBuilderDoesNotMutateAlreadyBuiltContext() {
+ Map<String, String> first = new HashMap<>();
+ first.put("k", "v1");
+ CallerContext.Builder builder = CallerContext.builder().withContext(first);
+ CallerContext built = builder.build();
+
+ // Reusing the same builder to set a different context must not affect the
already-built
+ // instance, which is now an independent immutable value object.
+ Map<String, String> second = new HashMap<>();
+ second.put("k", "v2");
+ builder.withContext(second);
+
+ Assertions.assertEquals("v1", built.context().get("k"));
+ }
+
+ @Test
+ public void testContextEqualsAcrossMutatedSourceMap() {
+ Map<String, String> source = new HashMap<>();
+ source.put("k", "v");
+ CallerContext expected =
CallerContext.builder().withContext(source).build();
+
+ // Mutating the source after building must not change equality against a
context built from the
+ // original contents.
+ source.put("k2", "v2");
+ CallerContext actual =
+ CallerContext.builder().withContext(Collections.singletonMap("k",
"v")).build();
+
+ Assertions.assertEquals(expected, actual);
+ Assertions.assertEquals(expected.hashCode(), actual.hashCode());
+ }
}
