Skip to content

Common: Validate DynConstructors implementation types #15912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manuzhang wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Common: Validate DynConstructors implementation types #15912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion azure/src/test/java/org/apache/iceberg/azure/TestAdlsTokenCredentialProviders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ public void nonImplementingClassAsCredentialProvider() {
ImmutableMap.of(AzureProperties.ADLS_TOKEN_CREDENTIAL_PROVIDER, "java.lang.String");
assertThatIllegalArgumentException()
.isThrownBy(() -> AdlsTokenCredentialProviders.from(properties))
.withMessageContaining("java.lang.String does not implement AdlsTokenCredentialProvider");
.withMessage(
"Cannot use java.lang.String as an implementation of org.apache.iceberg.azure.AdlsTokenCredentialProvider");
}

@Test
Expand Down
12 changes: 12 additions & 0 deletions common/src/main/java/org/apache/iceberg/common/DynConstructors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ public <T> Builder impl(Class<T> targetClass, Class<?>... types) {
return this;
}

checkBaseClassAssignableFrom(targetClass);
try {
ctor = new Ctor<>(targetClass.getConstructor(types), targetClass);
} catch (NoSuchMethodException e) {
Expand Down Expand Up @@ -189,6 +190,7 @@ public <T> Builder hiddenImpl(Class<T> targetClass, Class<?>... types) {
return this;
}

checkBaseClassAssignableFrom(targetClass);
try {
Constructor<T> hidden = targetClass.getDeclaredConstructor(types);
AccessController.doPrivileged(new MakeAccessible(hidden));
Expand Down Expand Up @@ -230,6 +232,16 @@ private Class<?> classForName(String className) throws ClassNotFoundException {
}
}
}

private void checkBaseClassAssignableFrom(Class<?> targetClass) {
if (baseClass != null) {
Preconditions.checkArgument(
baseClass.isAssignableFrom(targetClass),
"Cannot use %s as an implementation of %s",
targetClass.getName(),
baseClass.getName());
}
}
}

private static class MakeAccessible implements PrivilegedAction<Void> {
Expand Down
43 changes: 26 additions & 17 deletions common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,38 @@ public void testInterfaceImplNewInstance() throws Exception {

@Test
public void testInterfaceWrongImplString() throws Exception {
DynConstructors.Ctor<MyInterface> ctor =
DynConstructors.builder(MyInterface.class)
// TODO this should throw, since the MyUnrelatedClass does not implement MyInterface
.impl("org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass")
.buildChecked();
assertThatThrownBy(ctor::newInstance)
.isInstanceOf(ClassCastException.class)
assertThatThrownBy(
() ->
DynConstructors.builder(MyInterface.class)
.impl("org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass")
.buildChecked())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith(
"class org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass cannot be cast to class org.apache.iceberg.common.TestDynConstructors$MyInterface");
"Cannot use org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass as an implementation of org.apache.iceberg.common.TestDynConstructors$MyInterface");
}

@Test
public void testInterfaceWrongImplClass() throws Exception {
DynConstructors.Ctor<MyInterface> ctor =
DynConstructors.builder(MyInterface.class)
// TODO this should throw or not compile at all, since the MyUnrelatedClass does not
// implement MyInterface
.impl(MyUnrelatedClass.class)
.buildChecked();
assertThatThrownBy(ctor::newInstance)
.isInstanceOf(ClassCastException.class)
assertThatThrownBy(
() ->
DynConstructors.builder(MyInterface.class)
.impl(MyUnrelatedClass.class)
.buildChecked())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith(
"Cannot use org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass as an implementation of org.apache.iceberg.common.TestDynConstructors$MyInterface");
}

@Test
public void testInterfaceWrongHiddenImplClass() throws Exception {
assertThatThrownBy(
() ->
DynConstructors.builder(MyInterface.class)
.hiddenImpl(MyUnrelatedClass.class)
.buildChecked())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith(
"class org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass cannot be cast to class org.apache.iceberg.common.TestDynConstructors$MyInterface");
"Cannot use org.apache.iceberg.common.TestDynConstructors$MyUnrelatedClass as an implementation of org.apache.iceberg.common.TestDynConstructors$MyInterface");
}

@Test
Expand Down
9 changes: 1 addition & 8 deletions core/src/main/java/org/apache/iceberg/CatalogUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,7 @@ public static FileIO loadFileIO(
String.format("Cannot initialize FileIO implementation %s: %s", impl, e.getMessage()), e);
}

FileIO fileIO;
try {
fileIO = ctor.newInstance();
} catch (ClassCastException e) {
throw new IllegalArgumentException(
String.format("Cannot initialize FileIO, %s does not implement FileIO.", impl), e);
}

FileIO fileIO = ctor.newInstance();
configureHadoopConf(fileIO, hadoopConf);
if (fileIO instanceof SupportsStorageCredentials) {
((SupportsStorageCredentials) fileIO).setCredentials(storageCredentials);
Expand Down
11 changes: 6 additions & 5 deletions core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ public void loadCustomCatalog_NotImplementCatalog() {
CatalogUtil.loadCatalog(
TestCatalogNoInterface.class.getName(), name, options, hadoopConf))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot initialize Catalog")
.hasMessageContaining("does not implement Catalog");
.hasMessage(
"Cannot use org.apache.iceberg.TestCatalogUtil$TestCatalogNoInterface as an implementation of org.apache.iceberg.catalog.Catalog");
}

@Test
Expand Down Expand Up @@ -214,8 +214,8 @@ public void loadCustomFileIO_badClass() {
() ->
CatalogUtil.loadFileIO(TestFileIONotImpl.class.getName(), Maps.newHashMap(), null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot initialize FileIO")
.hasMessageContaining("does not implement FileIO");
.hasMessage(
"Cannot use org.apache.iceberg.TestCatalogUtil$TestFileIONotImpl as an implementation of org.apache.iceberg.io.FileIO");
}

@Test
Expand Down Expand Up @@ -263,7 +263,8 @@ public void loadCustomMetricsReporter_badClass() {
CatalogProperties.METRICS_REPORTER_IMPL,
TestFileIONotImpl.class.getName())))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("does not implement MetricsReporter");
.hasMessage(
"Cannot use org.apache.iceberg.TestCatalogUtil$TestFileIONotImpl as an implementation of org.apache.iceberg.metrics.MetricsReporter");
}

@Test
Expand Down
3 changes: 1 addition & 2 deletions core/src/test/java/org/apache/iceberg/TestLocationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,7 @@ public void testInvalidNoInterfaceDynamicallyLoadedLocationProvider() {
assertThatThrownBy(() -> table.locationProvider())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(
"Provided implementation for dynamic instantiation should implement %s.",
LocationProvider.class);
"Cannot use org.apache.iceberg.TestLocationProvider$InvalidNoInterfaceDynamicallyLoadedLocationProvider as an implementation of org.apache.iceberg.io.LocationProvider");
}

@TestTemplate
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,8 @@ public void testLoadTLSConfigurerNotImplementTLSConfigurer() {
ImmutableMap.of(HTTPClient.REST_TLS_CONFIGURER, Object.class.getName());
assertThatThrownBy(() -> HTTPClient.configureConnectionManager(properties))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot initialize TLSConfigurer")
.hasMessageContaining("does not implement TLSConfigurer");
.hasMessage(
"Cannot use java.lang.Object as an implementation of org.apache.iceberg.rest.auth.TLSConfigurer");
}

@Test
Expand Down
Loading