This is an automated email from the ASF dual-hosted git repository.
adutra pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 26a394e0e Fix FileIO usage in PolarisRestCatalogIntegrationBase (#3658)
26a394e0e is described below
commit 26a394e0ea68c68373a75251c97ab0711e637d20
Author: Alexandre Dutra <[email protected]>
AuthorDate: Fri Feb 6 13:35:54 2026 +0100
Fix FileIO usage in PolarisRestCatalogIntegrationBase (#3658)
Currently, all tests inheriting from `PolarisRestCatalogIntegrationBase`
are forcibly using `InMemoryFileIO` for all `RESTCatalog` instances.
Furthermore, this class wasn't overriding
`CatalogTests.baseTableLocation()`, so tests calling this method were actually
operating on `file://` paths all the time.
Because of that, some child test classes, and in particular, the
MiniIO-based ones, weren't actually testing anything useful.
This PR changes `PolarisRestCatalogIntegrationBase` to use a configurable
`FileIO` impl, defaulting to `ResolvingFileIO`.
This change uncovered a few latent issues with current tests, and notably:
1) Tests using MinIO should request credential vending consistently,
otherwise most tests would fail because the S3 client attempts to load
credentials from the environment.
2) Tests calling `registerTable` need special handling since this endpoint
does NOT support access delegation at all. For these tests, the `RESTCatalog`
clients need to be given direct credentials for S3 – since the server won't
give them anything – even with vended credentials requested.
Incidentally, this PR simplifies
`IntegrationTestsHelper.mergeFromAnnotatedElements()` – but its functionality
stays the same.
---
.../service/it/env/IntegrationTestsHelper.java | 6 +-
.../test/PolarisPolicyServiceIntegrationTest.java | 6 +-
.../it/test/PolarisRestCatalogIntegrationBase.java | 84 ++++++++++++++++++----
.../service/it/env/IntegrationTestsHelperTest.java | 42 ++++-------
.../it/RestCatalogAdlsCredentialVendingIT.java | 2 +
.../service/it/PolarisRestCatalogMinIOIT.java | 30 ++++++--
6 files changed, 114 insertions(+), 56 deletions(-)
diff --git
a/integration-tests/src/main/java/org/apache/polaris/service/it/env/IntegrationTestsHelper.java
b/integration-tests/src/main/java/org/apache/polaris/service/it/env/IntegrationTestsHelper.java
index a2becabe3..08746e456 100644
---
a/integration-tests/src/main/java/org/apache/polaris/service/it/env/IntegrationTestsHelper.java
+++
b/integration-tests/src/main/java/org/apache/polaris/service/it/env/IntegrationTestsHelper.java
@@ -90,10 +90,7 @@ public final class IntegrationTestsHelper {
* annotation will be used.
*/
public static <A extends Annotation> Map<String, String>
mergeFromAnnotatedElements(
- TestInfo testInfo,
- Class<A> annotationClass,
- Function<A, String[]> propertiesExtractor,
- Map<String, String> defaults) {
+ TestInfo testInfo, Class<A> annotationClass, Function<A, String[]>
propertiesExtractor) {
String[] methodProperties =
testInfo
.getTestMethod()
@@ -115,7 +112,6 @@ public final class IntegrationTestsHelper {
+ Arrays.toString(properties));
}
ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
- builder.putAll(defaults);
for (int i = 0; i < properties.length; i += 2) {
builder.put(properties[i], properties[i + 1]);
}
diff --git
a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
index 2557466de..a8ea9fda6 100644
---
a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
+++
b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
@@ -176,9 +176,11 @@ public class PolarisPolicyServiceIntegrationTest {
CatalogProperties.Builder catalogPropsBuilder =
CatalogProperties.builder(catalogBaseLocation);
+ catalogPropsBuilder.putAll(DEFAULT_CATALOG_PROPERTIES);
+
Map<String, String> catalogProperties =
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo, CatalogConfig.class, CatalogConfig::properties,
DEFAULT_CATALOG_PROPERTIES);
+ testInfo, CatalogConfig.class, CatalogConfig::properties);
catalogPropsBuilder.putAll(catalogProperties);
if (!s3BucketBase.getScheme().equals("file")) {
@@ -206,7 +208,7 @@ public class PolarisPolicyServiceIntegrationTest {
Map<String, String> restCatalogProperties =
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo, RestCatalogConfig.class, RestCatalogConfig::value,
Map.of());
+ testInfo, RestCatalogConfig.class, RestCatalogConfig::value);
String principalToken = client.obtainToken(principalCredentials);
restCatalog =
diff --git
a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java
b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java
index 0b38f864a..69a39c7b0 100644
---
a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java
+++
b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java
@@ -159,10 +159,12 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
private URI externalCatalogBaseLocation;
private String catalogBaseLocation;
+ /**
+ * Default RESTCatalog properties. Most tests from the parent class expect
these properties to be
+ * set.
+ */
private static final Map<String, String> DEFAULT_REST_CATALOG_CONFIG =
Map.of(
- org.apache.iceberg.CatalogProperties.FILE_IO_IMPL,
- "org.apache.iceberg.inmemory.InMemoryFileIO",
org.apache.iceberg.CatalogProperties.TABLE_DEFAULT_PREFIX +
"default-key1",
"catalog-default-key1",
org.apache.iceberg.CatalogProperties.TABLE_DEFAULT_PREFIX +
"default-key2",
@@ -209,6 +211,19 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
return ImmutableMap.builder();
}
+ /**
+ * The expected location for a table. Some tests rely on this location, but
the default
+ * implementation uses file:// locations.
+ */
+ @Override
+ protected String baseTableLocation(TableIdentifier identifier) {
+ return RESTUtil.stripTrailingSlash(catalogBaseLocation)
+ + "/"
+ + identifier.namespace()
+ + "/"
+ + identifier.name();
+ }
+
/** Get the base URI for the external catalog. */
protected URI externalCatalogBaseLocation() {
return externalCatalogBaseLocation;
@@ -255,10 +270,17 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
CatalogProperties.Builder catalogPropsBuilder =
CatalogProperties.builder(catalogBaseLocation);
- Map<String, String> catalogProperties =
+ // Sources of catalog properties:
+ // 1. DEFAULT_CATALOG_PROPERTIES
+ // 2. config from annotations
+ // 3. DROP_WITH_PURGE_ENABLED
+ // 4. FILE scheme adjustments
+
+ catalogPropsBuilder.putAll(DEFAULT_CATALOG_PROPERTIES);
+
+ catalogPropsBuilder.putAll(
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo, CatalogConfig.class, CatalogConfig::properties,
DEFAULT_CATALOG_PROPERTIES);
- catalogPropsBuilder.putAll(catalogProperties);
+ testInfo, CatalogConfig.class, CatalogConfig::properties));
catalogPropsBuilder.addProperty(
FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(), "true");
@@ -283,14 +305,29 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
createPolarisCatalog(catalog);
managementApi.makeAdmin(principalRoleName, catalog);
- restCatalogConfig =
+ ImmutableMap.Builder<String, String> restCatalogConfigBuilder =
ImmutableMap.builder();
+
+ // Sources of RESTCatalog properties:
+ // 1. DEFAULT_REST_CATALOG_CONFIG
+ // 3. FileIO implementation
+ // 2. config from annotations
+ // 4. config from extraCatalogProperties()
+ // 5. config passed directly to initCatalog() (not stored in
restCatalogConfig)
+
+ restCatalogConfigBuilder.putAll(DEFAULT_REST_CATALOG_CONFIG);
+
+ restCatalogConfigBuilder.put(
+ org.apache.iceberg.CatalogProperties.FILE_IO_IMPL,
+ restCatalogFileIOImplementation().getName());
+
+ restCatalogConfigBuilder.putAll(
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo,
- RestCatalogConfig.class,
- RestCatalogConfig::value,
- DEFAULT_REST_CATALOG_CONFIG);
+ testInfo, RestCatalogConfig.class, RestCatalogConfig::value));
+
+ restCatalogConfigBuilder.putAll(extraCatalogProperties(testInfo));
- restCatalog = initCatalog(currentCatalogName, ImmutableMap.of());
+ restCatalogConfig = restCatalogConfigBuilder.buildKeepingLast();
+ restCatalog = initCatalog(currentCatalogName, Map.of());
}
/**
@@ -354,8 +391,11 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
/**
* Initialize a RESTCatalog for testing.
*
- * @param catalogName this parameter is currently unused.
- * @param additionalProperties additional properties to apply on top of the
default test settings
+ * @param catalogName this parameter is currently unused. All RESTCatalog
instances created by
+ * tests have the name "polaris".
+ * @param additionalProperties additional properties to apply on top of the
configured defaults.
+ * Some tests in the parent class use this to inject test-specific
properties e.g. {@link
+ * #testCatalogWithCustomMetricsReporter()}.
* @return a configured instance of RESTCatalog
*/
@Override
@@ -367,6 +407,23 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
endpoints, currentCatalogName,
extraPropertiesBuilder.buildKeepingLast(), principalToken);
}
+ /**
+ * Additional properties to apply when creating the RESTCatalog, on top of
the default test
+ * settings and any properties specified via {@link RestCatalogConfig}
annotations.
+ *
+ * <p>This is useful when subclasses need to override some of the default
settings with a value
+ * that cannot be expressed via annotations (e.g. because it needs to refer
to some other
+ * test-local state).
+ */
+ protected Map<String, String> extraCatalogProperties(TestInfo testInfo) {
+ return Map.of();
+ }
+
+ /** The FileIO implementation class name to be used for the RESTCatalog
instance. */
+ protected Class<? extends FileIO> restCatalogFileIOImplementation() {
+ return ResolvingFileIO.class;
+ }
+
@Override
protected boolean requiresNamespaceCreate() {
return true;
@@ -690,6 +747,7 @@ public abstract class PolarisRestCatalogIntegrationBase
extends CatalogTests<RES
* should succeed.
*/
@CatalogConfig(Catalog.TypeEnum.EXTERNAL)
+ @RestCatalogConfig({"header.X-Iceberg-Access-Delegation", ""}) // force
empty header
@Test
public void
testLoadTableWithoutAccessDelegationForExternalCatalogWithConfigDisabled() {
Namespace ns1 = Namespace.of("ns1");
diff --git
a/integration-tests/src/test/java/org/apache/polaris/service/it/env/IntegrationTestsHelperTest.java
b/integration-tests/src/test/java/org/apache/polaris/service/it/env/IntegrationTestsHelperTest.java
index 63f75fd91..ddd2f3674 100644
---
a/integration-tests/src/test/java/org/apache/polaris/service/it/env/IntegrationTestsHelperTest.java
+++
b/integration-tests/src/test/java/org/apache/polaris/service/it/env/IntegrationTestsHelperTest.java
@@ -136,17 +136,14 @@ class IntegrationTestsHelperTest {
@ParameterizedTest
@MethodSource
void mergeFromAnnotatedElements(
- Method testMethod,
- Class<?> testClass,
- Map<String, String> defaults,
- Map<String, String> expected) {
+ Method testMethod, Class<?> testClass, Map<String, String> expected) {
TestInfo testInfo = mock(TestInfo.class);
when(testInfo.getTestMethod()).thenReturn(Optional.ofNullable(testMethod));
when(testInfo.getTestClass()).thenReturn(Optional.ofNullable(testClass));
Map<String, String> result =
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo, TestAnnotation.class, TestAnnotation::properties,
defaults);
+ testInfo, TestAnnotation.class, TestAnnotation::properties);
assertThat(result).isEqualTo(expected);
}
@@ -160,42 +157,27 @@ class IntegrationTestsHelperTest {
Arguments.of(
annotatedMethod,
AnnotatedClass.class,
- Map.of(),
Map.of(
"sharedKey", "methodValue", "classKey", "classValue",
"methodKey", "methodValue")),
+ // Only method has properties
+ Arguments.of(
+ annotatedMethod,
+ UnannotatedClass.class,
+ Map.of("sharedKey", "methodValue", "methodKey", "methodValue")),
// Only class has properties
Arguments.of(
unannotatedMethod,
AnnotatedClass.class,
- Map.of(),
Map.of("sharedKey", "classValue", "classKey", "classValue")),
// No method, class has properties
Arguments.of(
null,
AnnotatedClass.class,
- Map.of(),
- Map.of("sharedKey", "classValue", "classKey", "classValue")),
- // Neither has properties - return defaults
- Arguments.of(
- unannotatedMethod,
- UnannotatedClass.class,
- Map.of("defaultKey", "defaultValue"),
- Map.of("defaultKey", "defaultValue")),
- // No method, no class - return defaults
- Arguments.of(null, null, Map.of("key", "value"), Map.of("key",
"value")),
- // Defaults are overridden by class properties
- Arguments.of(
- null,
- AnnotatedClass.class,
- Map.of("sharedKey", "overridden"),
Map.of("sharedKey", "classValue", "classKey", "classValue")),
- // Defaults are overridden by method properties
- Arguments.of(
- annotatedMethod,
- AnnotatedClass.class,
- Map.of("methodKey", "overridden"),
- Map.of(
- "sharedKey", "methodValue", "classKey", "classValue",
"methodKey", "methodValue")));
+ // Neither has properties - return empty
+ Arguments.of(unannotatedMethod, UnannotatedClass.class, Map.of()),
+ // No method, no class - return empty
+ Arguments.of(null, null, Map.of()));
}
@TestAnnotation(properties = {"key1", "value1", "key2"})
@@ -213,7 +195,7 @@ class IntegrationTestsHelperTest {
assertThatThrownBy(
() ->
IntegrationTestsHelper.mergeFromAnnotatedElements(
- testInfo, TestAnnotation.class,
TestAnnotation::properties, Map.of()))
+ testInfo, TestAnnotation.class,
TestAnnotation::properties))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Properties must be in key-value pairs");
}
diff --git
a/runtime/service/src/cloudTest/java/org/apache/polaris/service/it/RestCatalogAdlsCredentialVendingIT.java
b/runtime/service/src/cloudTest/java/org/apache/polaris/service/it/RestCatalogAdlsCredentialVendingIT.java
index 169337d99..77f335cab 100644
---
a/runtime/service/src/cloudTest/java/org/apache/polaris/service/it/RestCatalogAdlsCredentialVendingIT.java
+++
b/runtime/service/src/cloudTest/java/org/apache/polaris/service/it/RestCatalogAdlsCredentialVendingIT.java
@@ -20,10 +20,12 @@ package org.apache.polaris.service.it;
import io.quarkus.test.junit.QuarkusIntegrationTest;
import io.quarkus.test.junit.TestProfile;
+import org.apache.polaris.service.it.env.RestCatalogConfig;
import
org.apache.polaris.service.it.test.PolarisRestCatalogAdlsIntegrationTestBase;
import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable;
@QuarkusIntegrationTest
@TestProfile(CredentialVendingProfile.class)
@EnabledIfEnvironmentVariable(named = "INTEGRATION_TEST_AZURE_PATH", matches =
".+")
+@RestCatalogConfig({"header.X-Iceberg-Access-Delegation",
"vended-credentials"})
public class RestCatalogAdlsCredentialVendingIT extends
PolarisRestCatalogAdlsIntegrationTestBase {}
diff --git
a/runtime/service/src/intTest/java/org/apache/polaris/service/it/PolarisRestCatalogMinIOIT.java
b/runtime/service/src/intTest/java/org/apache/polaris/service/it/PolarisRestCatalogMinIOIT.java
index 48a19d8ae..76c95dac1 100644
---
a/runtime/service/src/intTest/java/org/apache/polaris/service/it/PolarisRestCatalogMinIOIT.java
+++
b/runtime/service/src/intTest/java/org/apache/polaris/service/it/PolarisRestCatalogMinIOIT.java
@@ -25,21 +25,24 @@ import io.quarkus.test.junit.TestProfile;
import java.net.URI;
import java.util.List;
import java.util.Map;
+import org.apache.iceberg.aws.s3.S3FileIOProperties;
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
-import org.apache.polaris.core.storage.StorageAccessProperty;
+import org.apache.polaris.service.it.env.RestCatalogConfig;
import org.apache.polaris.service.it.ext.PolarisIntegrationTestExtension;
import org.apache.polaris.service.it.test.PolarisRestCatalogIntegrationBase;
import org.apache.polaris.test.minio.Minio;
import org.apache.polaris.test.minio.MinioAccess;
import org.apache.polaris.test.minio.MinioExtension;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.ExtendWith;
@QuarkusIntegrationTest
@TestProfile(PolarisRestCatalogMinIOIT.Profile.class)
@ExtendWith(MinioExtension.class)
@ExtendWith(PolarisIntegrationTestExtension.class)
+@RestCatalogConfig({"header.X-Iceberg-Access-Delegation",
"vended-credentials"})
public class PolarisRestCatalogMinIOIT extends
PolarisRestCatalogIntegrationBase {
protected static final String BUCKET_URI_PREFIX = "/minio-test-polaris";
@@ -61,20 +64,24 @@ public class PolarisRestCatalogMinIOIT extends
PolarisRestCatalogIntegrationBase
private static URI storageBase;
private static String endpoint;
+ private static Map<String, String> s3Properties;
+
@BeforeAll
static void setup(
@Minio(accessKey = MINIO_ACCESS_KEY, secretKey = MINIO_SECRET_KEY)
MinioAccess minioAccess) {
storageBase = minioAccess.s3BucketUri(BUCKET_URI_PREFIX);
endpoint = minioAccess.s3endpoint();
+ s3Properties =
+ Map.of(
+ S3FileIOProperties.ENDPOINT, endpoint,
+ S3FileIOProperties.PATH_STYLE_ACCESS, "true",
+ S3FileIOProperties.ACCESS_KEY_ID, MINIO_ACCESS_KEY,
+ S3FileIOProperties.SECRET_ACCESS_KEY, MINIO_SECRET_KEY);
}
@Override
protected ImmutableMap.Builder<String, String> clientFileIOProperties() {
- return super.clientFileIOProperties()
- .put(StorageAccessProperty.AWS_ENDPOINT.getPropertyName(), endpoint)
- .put(StorageAccessProperty.AWS_PATH_STYLE_ACCESS.getPropertyName(),
"true")
- .put(StorageAccessProperty.AWS_KEY_ID.getPropertyName(),
MINIO_ACCESS_KEY)
- .put(StorageAccessProperty.AWS_SECRET_KEY.getPropertyName(),
MINIO_SECRET_KEY);
+ return super.clientFileIOProperties().putAll(s3Properties);
}
@Override
@@ -88,4 +95,15 @@ public class PolarisRestCatalogMinIOIT extends
PolarisRestCatalogIntegrationBase
return storageConfig.build();
}
+
+ @Override
+ protected Map<String, String> extraCatalogProperties(TestInfo testInfo) {
+ if
(testInfo.getTestMethod().orElseThrow().getName().equals("testRegisterTable")) {
+ // This test registers a table – operation that doesn't support access
delegation –
+ // then attempts to use the table's FileIO. This can only work if the
client has its
+ // own S3 credentials.
+ return s3Properties;
+ }
+ return Map.of();
+ }
}
