LuciferYang opened a new issue, #7690:
URL: https://github.com/apache/kyuubi/issues/7690
### Description
`kyuubi-util`'s `DynFields` is a vendored copy of iceberg-common's class,
and its javadoc was carried over from the `DynMethods` variants without being
adapted. Fourteen statements are wrong: most describe a field API in terms of
methods, and two name an exception the method cannot throw.
`bind` is documented in terms of a type that does not exist in this file:
```java
/**
* Returns this method as a BoundMethod for the given receiver.
*
* @param target an Object on which to get or set this field
* @return a {@link BoundField} for this field and the target
* @throws IllegalStateException if the method is static
* @throws IllegalArgumentException if the receiver's class is
incompatible
*/
public BoundField<T> bind(Object target) {
```
There is no `BoundMethod` in `DynFields`, and the condition is `isStatic()`
on the wrapped `Field`, not on a method.
`build()` promises an exception it cannot throw:
```java
/**
* Returns the first valid implementation as a UnboundField or throws a
NoSuchFieldException if
* there is none.
*
* @param <T> Java class stored in the field
* @return a {@link UnboundField} with a valid implementation
* @throws RuntimeException if no implementation was found
*/
@SuppressWarnings("unchecked")
public <T> UnboundField<T> build() {
```
It has no `throws` clause and raises `RuntimeException`, so
`NoSuchFieldException` is impossible there, and the `@throws RuntimeException`
tag in the same comment already said so. `buildChecked(Object)` has the
mirror-image problem: it was documented as throwing `NoSuchMethodException`
when its signature declares `throws NoSuchFieldException`.
The full set: the `UnboundField` class doc ("invoke the wrapped method"),
`bind`, `asStatic`, `buildChecked(Object)`, `build()`, `build(Object)`,
`buildStaticChecked()`, `buildStatic()`, and three `@param fieldName` tags
carrying `(different from constructor)`, a parenthetical that separates methods
from constructors and has nothing to separate on a field.
The class doc has a second problem. It claims "all Exceptions wrapped by
RuntimeException, or with a single Exception catch block", but
`UnboundField.get` and `set` catch only `IllegalAccessException`. An
`IllegalArgumentException` for an incompatible target, a `NullPointerException`
for a null target on an instance field, or an `ExceptionInInitializerError`
through `StaticField` all propagate unwrapped, and there is no checked-throwing
accessor here for the "single Exception catch block" to refer to.
Both kinds of error cost a reader time rather than breaking anything at
runtime. `BoundMethod` cannot be resolved at all, and the documented checked
exceptions are ones the compiler will not let a caller catch, so the failure
path the javadoc describes turns out to be unwritable. The wrapping claim is
the one that can produce a real bug: code written as `catch (RuntimeException
e) { log(e.getCause().getMessage()); }` on the strength of it throws a
`NullPointerException` of its own when the actual failure was an unwrapped
`IllegalArgumentException` with no cause.
### Upstream
Upstream iceberg-common has the same wrong docs, so this is not something a
newer upstream release would bring in. Thirteen of the fourteen lines have a
direct upstream counterpart and are worth sending to iceberg separately. The
remaining one is `buildStaticChecked()`'s `@throws`, and that method exists
only in Kyuubi's copy because iceberg removed it along with `ctorImpl`.
--
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]
