Description
DynConstructors.builder() creates a Builder whose baseClass is null, and the zero-argument overload forwards that field:
public Builder hiddenImpl(Class<?>... types) {
hiddenImpl(baseClass, types);
return this;
}hiddenImpl(Class<T> targetClass, Class<?>... types) then calls targetClass.getDeclaredConstructor(types) and catches only SecurityException and NoSuchMethodException, so the null dereference leaves the builder chain as a bare NPE:
java.lang.NullPointerException: Cannot invoke
"java.lang.Class.getDeclaredConstructor(java.lang.Class[])" because "targetClass" is null
The message does not mention the missing base class.
A loose Class argument goes elsewhere: builder().hiddenImpl(Foo.class) binds hiddenImpl(Class, Class...) with Foo as the target class, and a miss there is recorded against Foo rather than against the base class. What does reach it is a call with no arguments, or one passing an explicit Class<?>[], which is also what a Scala : _* splat compiles to and is the call shape ReflectUtils already uses against the DynMethods builder. So builder(Foo.class).hiddenImpl(new Class<?>[] {String.class}) does find Foo(String), while the same call on a class-less builder throws.
No caller in Kyuubi reaches it. The zero-argument hiddenImpl() has no use in the repository, and the twenty-odd production sites that call DynConstructors.builder() without a class all continue with one of the impl overloads, naming either a class or a class name. So this is a latent defect in a retained upstream API rather than a failure anyone is hitting.
Affected code
DynConstructors.Builder.hiddenImpl(Class<?>... types). The String and Class<T> overloads name their own target class. Handing an explicit null to those, as in hiddenImpl((Class<?>) null, String.class), also throws NPE, but that is a caller supplying null rather than the builder forwarding its own unset field.
Affects Version(s)
master (1.13.0-SNAPSHOT), 1.12, 1.11, 1.10, 1.9. All carry the overload unchanged.
Upstream
iceberg-common deprecated this overload in 1.6.0, with "This varargs method conflicts with hiddenImpl(Class, Class...). Use builder(Class) instead.", and removed it in 1.7.0 (commit d17a7f189, apache/iceberg#10818, the same commit that removed the ctorImpl overloads behind #7688). The class-less builder() stayed; only the overload that read baseClass went away. There is no newer upstream release to sync a fix from, and dropping the overload is upstream's own answer to the ambiguity that produces this NPE. Note that dropping it here would not be free: a caller passing an explicit array, or a Scala splat, binds to this overload today.
Are you willing to submit PR?
Yes.
Description
DynConstructors.builder()creates aBuilderwhosebaseClassis null, and the zero-argument overload forwards that field:hiddenImpl(Class<T> targetClass, Class<?>... types)then callstargetClass.getDeclaredConstructor(types)and catches onlySecurityExceptionandNoSuchMethodException, so the null dereference leaves the builder chain as a bare NPE:The message does not mention the missing base class.
A loose
Classargument goes elsewhere:builder().hiddenImpl(Foo.class)bindshiddenImpl(Class, Class...)withFooas the target class, and a miss there is recorded againstFoorather than against the base class. What does reach it is a call with no arguments, or one passing an explicitClass<?>[], which is also what a Scala: _*splat compiles to and is the call shapeReflectUtilsalready uses against theDynMethodsbuilder. Sobuilder(Foo.class).hiddenImpl(new Class<?>[] {String.class})does findFoo(String), while the same call on a class-less builder throws.No caller in Kyuubi reaches it. The zero-argument
hiddenImpl()has no use in the repository, and the twenty-odd production sites that callDynConstructors.builder()without a class all continue with one of theimploverloads, naming either a class or a class name. So this is a latent defect in a retained upstream API rather than a failure anyone is hitting.Affected code
DynConstructors.Builder.hiddenImpl(Class<?>... types). TheStringandClass<T>overloads name their own target class. Handing an explicit null to those, as inhiddenImpl((Class<?>) null, String.class), also throws NPE, but that is a caller supplying null rather than the builder forwarding its own unset field.Affects Version(s)
master (1.13.0-SNAPSHOT), 1.12, 1.11, 1.10, 1.9. All carry the overload unchanged.
Upstream
iceberg-common deprecated this overload in 1.6.0, with "This varargs method conflicts with
hiddenImpl(Class, Class...). Usebuilder(Class)instead.", and removed it in 1.7.0 (commitd17a7f189, apache/iceberg#10818, the same commit that removed thectorImploverloads behind #7688). The class-lessbuilder()stayed; only the overload that readbaseClasswent away. There is no newer upstream release to sync a fix from, and dropping the overload is upstream's own answer to the ambiguity that produces this NPE. Note that dropping it here would not be free: a caller passing an explicit array, or a Scala splat, binds to this overload today.Are you willing to submit PR?
Yes.