You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by ch...@apache.org on 2026/08/24 05:54:26 UTC
(kyuubi) branch branch-1.12 updated: [KYUUBI #7651] Fix NPE binding the AlwaysNull sentinel field in DynFields
This is an automated email from the ASF dual-hosted git repository.
pan3793 pushed a commit to branch branch-1.12
in repository https://gitbox.apache.org/repos/asf/kyuubi.git
The following commit(s) were added to refs/heads/branch-1.12 by this push:
new 1fa6eafea6 [KYUUBI #7651] Fix NPE binding the AlwaysNull sentinel field in DynFields
1fa6eafea6 is described below
commit 1fa6eafea698c6c9ad7c84a055c2e7d65f5cb24b
Author: YangJie <ya...@baidu.com>
AuthorDate: Mon Aug 24 13:54:02 2026 +0800
[KYUUBI #7651] Fix NPE binding the AlwaysNull sentinel field in DynFields
### Why are the changes needed?
Closes #7651.
`DynFields.AlwaysNull` is a singleton with a `null` backing `Field`. It did not override `bind(Object)`, so `build(target)` / `buildChecked(target)` fell through to `UnboundField.bind`, which exempts the sentinel from the static-field check and then dereferences the null field on the next check, throwing `NullPointerException` instead of returning a field that reads `null`.
The fix mirrors `DynMethods.UnboundMethod.NOOP`, the sentinel in the sibling class, which already overrides `bind` to return a bound no-op. With that override in place the `this != AlwaysNull.INSTANCE` term in `UnboundField.bind` is unreachable, so it is narrowed to `if (isStatic())` and the sentinel case lives in one place only. `DynMethods.UnboundMethod.bind` has exactly that shape.
Two things stated so the change is not read as more than it is:
- Nothing in Kyuubi calls `defaultAlwaysNull()` today, so the NPE is currently unreachable. This hardens a vendored utility rather than fixing a reported failure.
- The sentinel is a no-op for reads and for `set(null)` only. Because `AlwaysNull` is an `UnboundField<Void>`, the bridge generated for `set(Object, Void)` casts the argument to `Void`, so `BoundField.set(nonNull)` throws `ClassCastException`. That is pre-existing, reachable before this patch through `buildStatic().set(x)`, and not addressed here: making writes a true no-op would mean changing the sentinel's type parameter and its `set` contract, which is a separate concern and belong [...]
`kyuubi-util`'s `DynFields` is copied from `iceberg-common`, which carries the same defect. It is tracked upstream as apache/iceberg#17044 and unfixed as of iceberg 1.11.0. The proposed upstream patch, apache/iceberg#17045, adds the same override but keeps the sentinel exemption in `UnboundField.bind`, so that one line stays different in this copy even after upstream lands.
### How was this patch tested?
New `DynFieldsTest`, 11 cases, covering the sentinel through every entry point (`build`, `build(target)`, `buildChecked(target)`, `buildStatic`, `buildStaticChecked`, direct `bind`) and the ordinary static/instance field paths:
```
build/mvn test -pl kyuubi-util -am -Dtest=DynFieldsTest -DwildcardSuites=none
```
```
Tests run: 11, Failures: 0, Errors: 0, Skipped: 0
```
Mutation-checked that the new cases have teeth:
- restoring `DynFields.java` to its state before this patch: 3 cases fail with `NullPointerException: Cannot invoke "java.lang.reflect.Field.getDeclaringClass()" because "this.field" is null`
- removing the `AlwaysNull.toString()` override: `testAlwaysNullToStringReportsSentinelName` fails with the same NPE, and only that case
`build/mvn spotless:check -pl kyuubi-util` passes.
### Was this patch assisted by generative AI tooling?
Assisted-by: Claude Opus 5
Closes #7652 from LuciferYang/kyuubi-util-dynfields-alwaysnull-bind.
Closes #7651
4587501ba [yangjie01] Narrow the AlwaysNull exemption in bind and cover the sentinel's toString
ac6432ddf [yangjie01] Fix NPE binding the AlwaysNull sentinel field in DynFields
Authored-by: YangJie <ya...@baidu.com>
Signed-off-by: Cheng Pan <ch...@apache.org>
(cherry picked from commit 7519db18e6239ac045000fff042fd9b662920a5f)
Signed-off-by: Cheng Pan <ch...@apache.org>
---
.../org/apache/kyuubi/util/reflect/DynFields.java | 24 ++-
.../apache/kyuubi/util/reflect/DynFieldsTest.java | 169 +++++++++++++++++++++
2 files changed, 191 insertions(+), 2 deletions(-)
diff --git a/kyuubi-util/src/main/java/org/apache/kyuubi/util/reflect/DynFields.java b/kyuubi-util/src/main/java/org/apache/kyuubi/util/reflect/DynFields.java
index 4c7af9036c..1ab64b4221 100644
--- a/kyuubi-util/src/main/java/org/apache/kyuubi/util/reflect/DynFields.java
+++ b/kyuubi-util/src/main/java/org/apache/kyuubi/util/reflect/DynFields.java
@@ -78,7 +78,8 @@ public class DynFields {
* @throws IllegalArgumentException if the receiver's class is incompatible
*/
public BoundField<T> bind(Object target) {
- if (isStatic() && this != AlwaysNull.INSTANCE) {
+ // AlwaysNull overrides bind, so this check needs no sentinel exemption.
+ if (isStatic()) {
throw new IllegalStateException("Cannot bind static field " + name);
}
if (!field.getDeclaringClass().isAssignableFrom(target.getClass())) {
@@ -125,6 +126,11 @@ public class DynFields {
return null;
}
+ /**
+ * Ignores the write. In practice only null gets this far: the bridge generated for this
+ * override casts the value to {@link Void} first, so a real value fails with {@link
+ * ClassCastException}.
+ */
@Override
public void set(Object target, Void value) {}
@@ -133,6 +139,19 @@ public class DynFields {
return "Field(AlwaysNull)";
}
+ /**
+ * Binds to any target, skipping both checks in {@link UnboundField#bind}. The sentinel wraps no
+ * field, so there is nothing to validate, and its {@code isStatic()} is a sentinel flag rather
+ * than a real modifier. {@code DynMethods.UnboundMethod.NOOP} binds the same way.
+ *
+ * <p>iceberg-common has no such override, so there the sentinel reaches {@link
+ * UnboundField#bind} and throws {@link NullPointerException} on its null field.
+ */
+ @Override
+ public BoundField<Void> bind(Object target) {
+ return new BoundField<>(this, target);
+ }
+
@Override
public boolean isStatic() {
return true;
@@ -204,7 +223,8 @@ public class DynFields {
}
/**
- * Instructs this builder to return AlwaysNull if no implementation is found.
+ * Instructs this builder to return AlwaysNull if no implementation is found. That sentinel
+ * reads as null and binds to any target.
*
* @return this Builder for method chaining
*/
diff --git a/kyuubi-util/src/test/java/org/apache/kyuubi/util/reflect/DynFieldsTest.java b/kyuubi-util/src/test/java/org/apache/kyuubi/util/reflect/DynFieldsTest.java
new file mode 100644
index 0000000000..ecf8ea2567
--- /dev/null
+++ b/kyuubi-util/src/test/java/org/apache/kyuubi/util/reflect/DynFieldsTest.java
@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.kyuubi.util.reflect;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+public class DynFieldsTest {
+
+ private static class ReflectionTarget {
+ public static String staticField = "static-value";
+
+ public String instanceField = "instance-value";
+ }
+
+ private static class OtherTarget {}
+
+ @Test
+ public void testAlwaysNullBindsToAnyTarget() {
+ DynFields.BoundField<String> bound =
+ DynFields.builder()
+ .impl(ReflectionTarget.class, "noSuchField")
+ .defaultAlwaysNull()
+ .build(new OtherTarget());
+
+ assertNull(bound.get());
+ // AlwaysNull is an UnboundField<Void>, so the bridge generated for its set(Object, Void)
+ // override casts the erased argument to Void: a side effect of the signature, not a contract.
+ assertThrows(ClassCastException.class, () -> bound.set("any-value"));
+ bound.set(null);
+ assertNull(bound.get());
+ }
+
+ @Test
+ public void testAlwaysNullBindsThroughEveryEntryPoint() throws NoSuchFieldException {
+ DynFields.Builder builder =
+ DynFields.builder().impl(ReflectionTarget.class, "noSuchField").defaultAlwaysNull();
+
+ DynFields.UnboundField<String> alwaysNull = builder.build();
+ assertTrue(alwaysNull.isAlwaysNull());
+ assertTrue(alwaysNull.isStatic());
+
+ DynFields.BoundField<String> viaBind = alwaysNull.bind(new OtherTarget());
+ assertNull(viaBind.get());
+
+ DynFields.BoundField<String> viaBuild = builder.build(new OtherTarget());
+ assertNull(viaBuild.get());
+
+ DynFields.BoundField<String> viaBuildChecked = builder.buildChecked(new ReflectionTarget());
+ assertNull(viaBuildChecked.get());
+ }
+
+ @Test
+ public void testAlwaysNullFromMissingClassNameBindsToAnyTarget() {
+ DynFields.BoundField<String> bound =
+ DynFields.builder()
+ .impl("a.b.MissingClass", "noSuchField")
+ .defaultAlwaysNull()
+ .build(new ReflectionTarget());
+
+ assertNull(bound.get());
+ }
+
+ @Test
+ public void testAlwaysNullAsStaticReturnsNull() throws NoSuchFieldException {
+ DynFields.StaticField<String> viaBuildStatic =
+ DynFields.builder()
+ .impl(ReflectionTarget.class, "noSuchField")
+ .defaultAlwaysNull()
+ .buildStatic();
+ assertNull(viaBuildStatic.get());
+
+ DynFields.StaticField<String> viaBuildStaticChecked =
+ DynFields.builder()
+ .impl(ReflectionTarget.class, "noSuchField")
+ .defaultAlwaysNull()
+ .buildStaticChecked();
+ assertNull(viaBuildStaticChecked.get());
+ }
+
+ @Test
+ public void testAlwaysNullToStringReportsSentinelName() {
+ DynFields.UnboundField<String> alwaysNull =
+ DynFields.builder().impl(ReflectionTarget.class, "noSuchField").defaultAlwaysNull().build();
+
+ assertEquals("Field(AlwaysNull)", alwaysNull.toString());
+ }
+
+ @Test
+ public void testRealFieldIsNotAlwaysNull() {
+ DynFields.UnboundField<String> realField =
+ DynFields.builder().impl(ReflectionTarget.class, "instanceField").build();
+
+ assertFalse(realField.isAlwaysNull());
+ assertFalse(realField.isStatic());
+ }
+
+ @Test
+ public void testBuildStaticReadsRealStaticField() {
+ DynFields.StaticField<String> staticField =
+ DynFields.builder().impl(ReflectionTarget.class, "staticField").buildStatic();
+
+ assertEquals("static-value", staticField.get());
+ }
+
+ @Test
+ public void testBuildWithoutAlwaysNullFallbackThrows() {
+ RuntimeException thrown =
+ assertThrows(
+ RuntimeException.class,
+ () -> DynFields.builder().impl(ReflectionTarget.class, "noSuchField").build());
+
+ assertTrue(thrown.getMessage().contains("Cannot find field from candidates"));
+ }
+
+ @Test
+ public void testBindRejectsStaticField() {
+ DynFields.UnboundField<String> staticField =
+ DynFields.builder().impl(ReflectionTarget.class, "staticField").build();
+
+ IllegalStateException thrown =
+ assertThrows(IllegalStateException.class, () -> staticField.bind(new ReflectionTarget()));
+ assertEquals("Cannot bind static field staticField", thrown.getMessage());
+ }
+
+ @Test
+ public void testBindRejectsIncompatibleTarget() {
+ DynFields.UnboundField<String> instanceField =
+ DynFields.builder().impl(ReflectionTarget.class, "instanceField").build();
+
+ IllegalArgumentException thrown =
+ assertThrows(IllegalArgumentException.class, () -> instanceField.bind(new OtherTarget()));
+ assertTrue(thrown.getMessage().contains("Cannot bind field instanceField to instance of"));
+ }
+
+ @Test
+ public void testBoundInstanceFieldGetAndSet() {
+ ReflectionTarget target = new ReflectionTarget();
+ DynFields.BoundField<String> bound =
+ DynFields.builder().impl(ReflectionTarget.class, "instanceField").build(target);
+
+ assertEquals("instance-value", bound.get());
+ bound.set("new-value");
+ assertEquals("new-value", bound.get());
+ assertEquals("new-value", target.instanceField);
+ }
+}