Common: Validate impl class implements base class in DynConstructors
- Dominant language
- Java
- Stars
- 9.2k
- Forks
- 3.5k
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 129
Description
### Feature Request / Improvement
## Problem
`DynConstructors.Builder` accepts a `baseClass` via `DynConstructors.builder(Class baseClass)` and is used throughout Iceberg to load pluggable implementations at runtime (e.g. `Catalog`, `FileIO`, `AuthManager`, `LockManager`).
When an implementation class does **not** implement or extend the expected base class, the error is deferred until `Ctor.newInstance()` is called, where it surfaces as a `ClassCastException`. This makes misconfiguration (wrong class name in catalog properties, typos, etc.) harder to diagnose.
There are existing TODOs in `TestDynConstructors` documenting this gap:
- `common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java:47` — string-based `.impl(...)` with an unrelated class
- `common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java:60` — class-based `.impl(...)` with an unrelated class
Current behavior (both tests expect `ClassCastException` at `newInstance()` time):
```java
DynConstructors.Ctor ctor =
DynConstructors.builder(MyInterface.class)
.impl(MyUnrelatedClass.class) // does not implement MyInterface
.buildChecked();
ctor.newInstance(); // ClassCastException here
```
## Proposed behavior
When `baseClass` is set on the builder, validate that the resolved implementation class is assignable to `baseClass` at **build time** (`build()` / `buildChecked()`), and fail fast with a clear, actionable error.
For example:
```java
assertThatThrownBy(() ->
DynConstructors.builder(MyInterface.class)
.impl(MyUnrelatedClass.class)
.buildChecked())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("does not implement");
```
The same validation should apply to both `.impl(String, ...)` and `.impl(Class, ...)` (and likely `.hiddenImpl(...)` variants as well).
## Scope
- **Module:** `common` (`DynConstructors.java`)
- **Tests:** Update `TestDynConstructors.testInterfaceWrongImplString` and `TestDynConstructors.testInterfaceWrongImplClass` to assert build-time failure instead of `ClassCastException` at instantiation
- **Impact:** Improves error messages for all runtime plugin loading paths that use `DynConstructors.builder(SomeInterface.class)` (Catalog, FileIO, AuthManager, etc.)
## Notes
- `DynConstructors` is copied from parquet-common; consider whether a similar upstream fix is warranted, but Iceberg can fix locally regardless.
- When `builder()` is called without a `baseClass`, no assignability check is needed (existing behavior for `DynConstructors.builder().impl(MyClass.class)` should remain unchanged).
### Query engine
_No response_
### Willingness to contribute
- [x] I can contribute this improvement/feature independently
- [x] I would be willing to contribute this improvement/feature with guidance from the Iceberg community
- [ ] I cannot contribute this improvement/feature at this time
Contributor guide
Research direction
Start in common/src/main/java/org/apache/iceberg/common/DynConstructors.java and read the builder and build/buildChecked paths. Then inspect common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java, especially testInterfaceWrongImplString and testInterfaceWrongImplClass. Run the targeted tests; done means mismatched implementations fail during build with a clear IllegalArgumentException while builders without a baseClass retain existing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- tooling
- Issue type
- Feature
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100