LuciferYang opened a new pull request, #7698:
URL: https://github.com/apache/kyuubi/pull/7698
### Why are the changes needed?
Closes #7697.
`DynConstructors.builder()` leaves `baseClass` null, and the zero-argument
`hiddenImpl(Class<?>... types)` forwarded that field into `hiddenImpl(Class,
Class...)`, which calls `targetClass.getDeclaredConstructor(types)` and catches
only `SecurityException` and `NoSuchMethodException`. The null escaped as a
bare NPE from inside the builder chain, with no mention of a missing base
class. A builder that never named a class asking for a hidden constructor in
its base class has no correct reading, so the patch rejects the call. AGENTS.md
asks for a throw on unsupported usage rather than a silent skip, and
`IllegalStateException` is what this file already uses when a call does not fit
the receiver's state: `Ctor.bind` throws it for "Cannot bind constructors".
A loose `Class` argument does not reach that overload:
`builder().hiddenImpl(Foo.class)` binds `hiddenImpl(Class, Class...)` with
`Foo` as the target. What reaches it is a call with no arguments, or one
passing an explicit `Class<?>[]`, which is what a Scala `: _*` splat compiles
to and is the shape `ReflectUtils` already uses against the `DynMethods`
builder.
Nothing in Kyuubi reaches it either. `hiddenImpl()` has no caller outside
the new tests, and the twenty-odd production sites that build without a class
all continue with one of the `impl` overloads. This is hygiene on a retained
upstream API, not a field failure being fixed.
There is a second option a maintainer may prefer: drop the overload, as
iceberg-common did in 1.7.0 (apache/iceberg#10818) after deprecating it in
1.6.0 over the same varargs conflict. That is not free, though: a caller
passing an explicit array, or a Scala splat, binds here today, so removal is a
source break for them even if nothing in this repository notices. Keeping the
overload means carrying this guard and its explanation indefinitely. Removing a
public method from a published module is a maintainer call rather than mine;
#7689 put the same choice to reviewers for `DynMethods.ctorImpl`.
Two scope notes. The guard sits below the `ctor != null` short-circuit that
opens every other `impl` and `hiddenImpl` overload, so a class-less builder
that already matched keeps working:
`builder().impl("java.lang.String").hiddenImpl()` builds before this patch and
still builds after. Placed above the short-circuit it would start throwing. And
the generic `Cannot find constructor for null` that a class-less builder
reports when all its name-based lookups miss is untouched; `hiddenImpl()` no
longer reaches it, and improving that message belongs with the `problems`
diagnostics work.
### How was this patch tested?
Two tests in `DynConstructorsTest`, one per half of the contract.
`testHiddenImplWithoutBaseClassFailsFast` asserts that
`builder().hiddenImpl()` throws `IllegalStateException` whose message says the
builder has no base class. Against `origin/master`'s `DynConstructors` it goes
red with `Unexpected exception type thrown, expected:
<java.lang.IllegalStateException> but was: <java.lang.NullPointerException>`,
that NPE being `Cannot invoke
"java.lang.Class.getDeclaredConstructor(java.lang.Class[])" because
"targetClass" is null`.
`testHiddenImplWithoutBaseClassIsSkippedOnceFound` builds
`builder().impl("java.lang.String").hiddenImpl().buildChecked()` and asserts
the constructed class is `String`. It pins the guard's placement rather than
restating the first test: with the null check moved above the `ctor != null`
short-circuit, it fails with `IllegalStateException`.
```
build/mvn -o test -pl kyuubi-util -am -DwildcardSuites=none
-Dtest=DynConstructorsTest
build/mvn -o spotless:check -pl kyuubi-util
```
6 of 6 green and spotless clean. Both revert checks above ran against a copy
of the pre-fix class compiled outside the repository.
### Was this patch assisted by generative AI tooling?
Assisted-by: Claude Opus 5
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
