Minor refactor of integration test classes (#2384)

adutra · web-flow · commit 12ab61851876 · 2025-08-19T16:15:21.000+02:00
This change promotes `CatalogConfig` and `RestCatalogConfig` to top-level, public annotations and introduces a few "hooks" in `PolarisRestCatalogIntegrationBase` that can be overridden by subclasses. This change is a preparatory work for #2280 (S3 remote signing).
diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogConfig.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogConfig.java
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.it.env;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Inherited;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.apache.polaris.core.admin.model.Catalog;
+
+/**
+ * Annotation to configure the catalog type and properties for integration tests.
+ *
+ * <p>This is a server-side setting; it is used to specify the Polaris Catalog type (e.g., INTERNAL,
+ * EXTERNAL) and any additional properties required for the catalog configuration.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.METHOD})
+@Inherited
+public @interface CatalogConfig {
+
+  /** The type of the catalog. Defaults to INTERNAL. */
+  Catalog.TypeEnum value() default Catalog.TypeEnum.INTERNAL;
+
+  /** Additional properties for the catalog configuration. */
+  String[] properties() default {};
+}
diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/IcebergHelper.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/IcebergHelper.java
@@ -20,6 +20,7 @@
 
 import com.google.common.collect.ImmutableMap;
 import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
 import org.apache.iceberg.rest.RESTCatalog;
 import org.apache.iceberg.rest.auth.OAuth2Properties;
 
@@ -35,12 +36,8 @@ public static RESTCatalog restCatalog(
 
     ImmutableMap.Builder<String, String> propertiesBuilder =
         ImmutableMap.<String, String>builder()
-            .put(
-                org.apache.iceberg.CatalogProperties.URI, endpoints.catalogApiEndpoint().toString())
+            .put(CatalogProperties.URI, endpoints.catalogApiEndpoint().toString())
             .put(OAuth2Properties.TOKEN, authToken)
-            .put(
-                org.apache.iceberg.CatalogProperties.FILE_IO_IMPL,
-                "org.apache.iceberg.inmemory.InMemoryFileIO")
             .put("warehouse", catalog)
             .put("header." + endpoints.realmHeaderName(), endpoints.realmId())
             .putAll(extraProperties);
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
@@ -19,9 +19,16 @@
 package org.apache.polaris.service.it.env;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableMap;
+import java.lang.annotation.Annotation;
+import java.lang.reflect.AnnotatedElement;
 import java.net.URI;
 import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Map;
 import java.util.function.Function;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.TestInfo;
 
 public final class IntegrationTestsHelper {
 
@@ -54,4 +61,66 @@ static URI getTemporaryDirectory(Function<String, String> getenv, Path defaultLo
     envVar = envVar.startsWith("/") ? "file://" + envVar : envVar;
     return URI.create(envVar + "/").normalize();
   }
+
+  /**
+   * Extract a value from the annotated elements of the test method and class.
+   *
+   * <p>This method looks for annotations of the specified type on both the test method and the test
+   * class, extracts the value using the provided extractor function, and returns it.
+   *
+   * <p>If the value is present in both the method and class annotations, the value from the method
+   * annotation will be used. If the value is not present in either the method or class annotations,
+   * this method returns the default value.
+   */
+  public static <A extends Annotation, T> T extractFromAnnotatedElements(
+      TestInfo testInfo, Class<A> annotationClass, Function<A, T> extractor, T defaultValue) {
+    return testInfo
+        .getTestMethod()
+        .map(AnnotatedElement.class::cast)
+        .or(testInfo::getTestClass)
+        .map(clz -> clz.getAnnotation(annotationClass))
+        .map(extractor)
+        .orElse(defaultValue);
+  }
+
+  /**
+   * Collect properties from annotated elements in the test method and class.
+   *
+   * <p>This method looks for annotations of the specified type on both the test method and the test
+   * class, extracts properties using the provided extractor function, and combines them into a map.
+   * If a property appears in both the method and class annotations, the value from the method
+   * 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) {
+    String[] methodProperties =
+        testInfo
+            .getTestMethod()
+            .map(m -> m.getAnnotation(annotationClass))
+            .map(propertiesExtractor)
+            .orElse(new String[0]);
+    String[] classProperties =
+        testInfo
+            .getTestClass()
+            .map(clz -> clz.getAnnotation(annotationClass))
+            .map(propertiesExtractor)
+            .orElse(new String[0]);
+    String[] properties =
+        Stream.concat(Arrays.stream(methodProperties), Arrays.stream(classProperties))
+            .toArray(String[]::new);
+    if (properties.length % 2 != 0) {
+      throw new IllegalArgumentException(
+          "Properties must be in key-value pairs, but found an odd number of elements: "
+              + 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]);
+    }
+    return builder.buildKeepingLast();
+  }
 }
diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestCatalogConfig.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestCatalogConfig.java
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.it.env;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Inherited;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Annotation to configure the REST catalog for integration tests.
+ *
+ * <p>This is a client-side setting; it is used to configure the client-side REST catalog that is
+ * used in test code to connect to the Polaris REST API and to the storage layer.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.METHOD})
+@Inherited
+public @interface RestCatalogConfig {
+  String[] value() default {};
+}
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
@@ -22,11 +22,9 @@
 import static org.apache.polaris.service.it.env.PolarisClient.polarisClient;
 import static org.assertj.core.api.Assertions.assertThat;
 
-import com.google.common.collect.ImmutableMap;
 import jakarta.ws.rs.client.Entity;
 import jakarta.ws.rs.core.Response;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
+import java.io.IOException;
 import java.lang.reflect.Method;
 import java.net.URI;
 import java.nio.file.Path;
@@ -64,13 +62,15 @@
 import org.apache.polaris.core.entity.CatalogEntity;
 import org.apache.polaris.core.policy.PredefinedPolicyTypes;
 import org.apache.polaris.core.policy.exceptions.PolicyInUseException;
+import org.apache.polaris.service.it.env.CatalogConfig;
 import org.apache.polaris.service.it.env.ClientCredentials;
 import org.apache.polaris.service.it.env.IcebergHelper;
 import org.apache.polaris.service.it.env.IntegrationTestsHelper;
 import org.apache.polaris.service.it.env.ManagementApi;
 import org.apache.polaris.service.it.env.PolarisApiEndpoints;
 import org.apache.polaris.service.it.env.PolarisClient;
 import org.apache.polaris.service.it.env.PolicyApi;
+import org.apache.polaris.service.it.env.RestCatalogConfig;
 import org.apache.polaris.service.it.ext.PolarisIntegrationTestExtension;
 import org.apache.polaris.service.types.ApplicablePolicy;
 import org.apache.polaris.service.types.AttachPolicyRequest;
@@ -131,25 +131,10 @@ public class PolarisPolicyServiceIntegrationTest {
   private final String catalogBaseLocation =
       s3BucketBase + "/" + System.getenv("USER") + "/path/to/data";
 
-  private static final String[] DEFAULT_CATALOG_PROPERTIES = {
-    "polaris.config.allow.unstructured.table.location", "true",
-    "polaris.config.allow.external.table.location", "true"
-  };
-
-  @Retention(RetentionPolicy.RUNTIME)
-  private @interface CatalogConfig {
-    Catalog.TypeEnum value() default Catalog.TypeEnum.INTERNAL;
-
-    String[] properties() default {
-      "polaris.config.allow.unstructured.table.location", "true",
-      "polaris.config.allow.external.table.location", "true"
-    };
-  }
-
-  @Retention(RetentionPolicy.RUNTIME)
-  private @interface RestCatalogConfig {
-    String[] value() default {};
-  }
+  private static final Map<String, String> DEFAULT_CATALOG_PROPERTIES =
+      Map.of(
+          "polaris.config.allow.unstructured.table.location", "true",
+          "polaris.config.allow.external.table.location", "true");
 
   @BeforeAll
   public static void setup(
@@ -188,28 +173,26 @@ public void before(TestInfo testInfo) {
             .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
             .setAllowedLocations(List.of("s3://my-old-bucket/path/to/data"))
             .build();
-    Optional<PolarisPolicyServiceIntegrationTest.CatalogConfig> catalogConfig =
-        Optional.ofNullable(
-            method.getAnnotation(PolarisPolicyServiceIntegrationTest.CatalogConfig.class));
 
     CatalogProperties.Builder catalogPropsBuilder = CatalogProperties.builder(catalogBaseLocation);
-    String[] properties =
-        catalogConfig
-            .map(PolarisPolicyServiceIntegrationTest.CatalogConfig::properties)
-            .orElse(DEFAULT_CATALOG_PROPERTIES);
-    for (int i = 0; i < properties.length; i += 2) {
-      catalogPropsBuilder.addProperty(properties[i], properties[i + 1]);
-    }
+
+    Map<String, String> catalogProperties =
+        IntegrationTestsHelper.mergeFromAnnotatedElements(
+            testInfo, CatalogConfig.class, CatalogConfig::properties, DEFAULT_CATALOG_PROPERTIES);
+    catalogPropsBuilder.putAll(catalogProperties);
+
     if (!s3BucketBase.getScheme().equals("file")) {
       catalogPropsBuilder.addProperty(
           CatalogEntity.REPLACE_NEW_LOCATION_PREFIX_WITH_CATALOG_DEFAULT_KEY, "file:");
     }
+
+    Catalog.TypeEnum catalogType =
+        IntegrationTestsHelper.extractFromAnnotatedElements(
+            testInfo, CatalogConfig.class, CatalogConfig::value, Catalog.TypeEnum.INTERNAL);
+
     Catalog catalog =
         PolarisCatalog.builder()
-            .setType(
-                catalogConfig
-                    .map(PolarisPolicyServiceIntegrationTest.CatalogConfig::value)
-                    .orElse(Catalog.TypeEnum.INTERNAL))
+            .setType(catalogType)
             .setName(currentCatalogName)
             .setProperties(catalogPropsBuilder.build())
             .setStorageConfigInfo(
@@ -221,26 +204,14 @@ public void before(TestInfo testInfo) {
 
     managementApi.createCatalog(principalRoleName, catalog);
 
-    Optional<PolarisPolicyServiceIntegrationTest.RestCatalogConfig> restCatalogConfig =
-        testInfo
-            .getTestMethod()
-            .flatMap(
-                m ->
-                    Optional.ofNullable(
-                        m.getAnnotation(
-                            PolarisPolicyServiceIntegrationTest.RestCatalogConfig.class)));
-    ImmutableMap.Builder<String, String> extraPropertiesBuilder = ImmutableMap.builder();
-    restCatalogConfig.ifPresent(
-        config -> {
-          for (int i = 0; i < config.value().length; i += 2) {
-            extraPropertiesBuilder.put(config.value()[i], config.value()[i + 1]);
-          }
-        });
+    Map<String, String> restCatalogProperties =
+        IntegrationTestsHelper.mergeFromAnnotatedElements(
+            testInfo, RestCatalogConfig.class, RestCatalogConfig::value, Map.of());
 
     String principalToken = client.obtainToken(principalCredentials);
     restCatalog =
         IcebergHelper.restCatalog(
-            endpoints, currentCatalogName, extraPropertiesBuilder.build(), principalToken);
+            endpoints, currentCatalogName, restCatalogProperties, principalToken);
     CatalogGrant catalogGrant =
         new CatalogGrant(CatalogPrivilege.CATALOG_MANAGE_CONTENT, GrantResource.TypeEnum.CATALOG);
     managementApi.createCatalogRole(currentCatalogName, CATALOG_ROLE_1);
@@ -253,8 +224,14 @@ public void before(TestInfo testInfo) {
   }
 
   @AfterEach
-  public void cleanUp() {
-    client.cleanUp(adminToken);
+  public void cleanUp() throws IOException {
+    try {
+      if (restCatalog != null) {
+        restCatalog.close();
+      }
+    } finally {
+      client.cleanUp(adminToken);
+    }
   }
 
   @Test
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
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
