apache / apache/iceberg

Common: Validate impl class implements base class in DynConstructors

Open
#17,506 0 comments 0 reactions 0 assignees View on GitHub
improvement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.