Do not use PostConstruct for config validation

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java

@@ -21,15 +21,13 @@
 import io.airlift.units.Duration;
 import io.airlift.units.MinDuration;
 import io.trino.plugin.base.logging.SessionInterpolatedValues;
-import jakarta.annotation.PostConstruct;
 import jakarta.validation.constraints.AssertTrue;
 import jakarta.validation.constraints.Max;
 import jakarta.validation.constraints.Min;
 import jakarta.validation.constraints.NotNull;
 
 import java.util.Optional;
 
-import static com.google.common.base.Preconditions.checkState;
 import static io.trino.plugin.base.logging.FormatInterpolator.hasValidPlaceholders;
 import static java.util.concurrent.TimeUnit.HOURS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -386,19 +384,27 @@ public BigQueryConfig setMetadataParallelism(int metadataParallelism)
         return this;
     }
 
-    @PostConstruct
-    public void validate()
+    @AssertTrue(message = "View expiration duration must be longer than view cache TTL")
+    public boolean isValidViewExpireDuration()
     {
-        checkState(viewExpireDuration.toMillis() > viewsCacheTtl.toMillis(), "View expiration duration must be longer than view cache TTL");
+        return viewExpireDuration.toMillis() > viewsCacheTtl.toMillis();
+    }
+
+    @AssertTrue(message = VIEWS_ENABLED + " config property must be enabled when bigquery.skip-view-materialization is enabled")
+    public boolean isValidViewsWehnEnabledSkipViewMaterialization()
+    {
+        return !skipViewMaterialization || viewsEnabled;
+    }
 
-        if (skipViewMaterialization) {
-            checkState(viewsEnabled, "%s config property must be enabled when skipping view materialization", VIEWS_ENABLED);
-        }
-        if (viewMaterializationWithFilter) {
-            checkState(viewsEnabled, "%s config property must be enabled when view materialization with filter is enabled", VIEWS_ENABLED);
-        }
-        if (!caseInsensitiveNameMatchingCacheTtl.isZero()) {
-            checkState(caseInsensitiveNameMatching, "bigquery.case-insensitive-name-matching config must be enabled when case insensitive name matching cache TTL is set");
-        }
+    @AssertTrue(message = VIEWS_ENABLED + " config property must be enabled when bigquery.view-materialization-with-filter is enabled")
+    public boolean isValidViewsEnableWhenViewMaterializationWithFilter()
+    {
+        return !viewMaterializationWithFilter || viewsEnabled;
+    }
+
+    @AssertTrue(message = "bigquery.case-insensitive-name-matching config must be enabled when bigquery.case-insensitive-name-matching.cache-ttl is set")
+    public boolean isValidCaseInsensitiveNameMatchingCacheTtl()
+    {
+        return caseInsensitiveNameMatchingCacheTtl.isZero() || caseInsensitiveNameMatching;
     }
 }

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryProxyConfig.java

@@ -13,15 +13,14 @@
  */
 package io.trino.plugin.bigquery;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.inject.ConfigurationException;
 import com.google.inject.spi.Message;
 import io.airlift.configuration.Config;
 import io.airlift.configuration.ConfigDescription;
 import io.airlift.configuration.ConfigSecuritySensitive;
 import io.airlift.configuration.validation.FileExists;
-import jakarta.annotation.PostConstruct;
+import jakarta.validation.constraints.AssertTrue;
 import jakarta.validation.constraints.NotNull;
 
 import java.io.File;
@@ -135,21 +134,22 @@ public BigQueryProxyConfig setTruststorePassword(String truststorePassword)
         return this;
     }
 
-    @PostConstruct
-    @VisibleForTesting
-    void validate()
+    @AssertTrue(message = "BigQuery RPC proxy URI cannot specify path")
+    public boolean isUriValid()
     {
-        if (!isNullOrEmpty(uri.getPath())) {
-            throw exception("BigQuery RPC proxy URI cannot specify path");
-        }
+        return isNullOrEmpty(uri.getPath());
+    }
 
-        if (username.isPresent() && password.isEmpty()) {
-            throw exception("bigquery.rpc-proxy.username was set but bigquery.rpc-proxy.password is empty");
-        }
+    @AssertTrue(message = "bigquery.rpc-proxy.username was set but bigquery.rpc-proxy.password is empty")
+    public boolean isPasswordNonEmptyIfUserProvided()
+    {
+        return password.isPresent() || username.isEmpty();
+    }
 
-        if (username.isEmpty() && password.isPresent()) {
-            throw exception("bigquery.rpc-proxy.password was set but bigquery.rpc-proxy.username is empty");
-        }
+    @AssertTrue(message = "bigquery.rpc-proxy.password was set but bigquery.rpc-proxy.username is empty")
+    public boolean isUserNotEmptyIfPasswordProvided()
+    {
+        return username.isPresent() || password.isEmpty();
     }
 
     private static ConfigurationException exception(String message)

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java

@@ -15,18 +15,19 @@
 
 import com.google.common.collect.ImmutableMap;
 import io.airlift.units.Duration;
+import jakarta.validation.constraints.AssertTrue;
 import org.junit.jupiter.api.Test;
 
 import java.util.Map;
 
 import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping;
 import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults;
 import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults;
+import static io.airlift.testing.ValidationAssertions.assertFailsValidation;
 import static java.util.concurrent.TimeUnit.DAYS;
 import static java.util.concurrent.TimeUnit.HOURS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.MINUTES;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class TestBigQueryConfig
 {
@@ -119,32 +120,36 @@ public void testExplicitPropertyMappingsWithCredentialsKey()
     @Test
     public void testInvalidViewSetting()
     {
-        assertThatThrownBy(() -> new BigQueryConfig()
-                .setViewExpireDuration(new Duration(5, MINUTES))
-                .setViewsCacheTtl(new Duration(10, MINUTES))
-                .validate())
-                .isInstanceOf(IllegalStateException.class)
-                .hasMessage("View expiration duration must be longer than view cache TTL");
+        assertFailsValidation(
+                new BigQueryConfig()
+                        .setViewExpireDuration(new Duration(5, MINUTES))
+                        .setViewsCacheTtl(new Duration(10, MINUTES)),
+                "validViewExpireDuration",
+                "View expiration duration must be longer than view cache TTL",
+                AssertTrue.class);
 
-        assertThatThrownBy(() -> new BigQueryConfig()
-                .setSkipViewMaterialization(true)
-                .setViewsEnabled(false)
-                .validate())
-                .isInstanceOf(IllegalStateException.class)
-                .hasMessage("bigquery.views-enabled config property must be enabled when skipping view materialization");
+        assertFailsValidation(
+                new BigQueryConfig()
+                        .setSkipViewMaterialization(true)
+                        .setViewsEnabled(false),
+                "validViewsWehnEnabledSkipViewMaterialization",
+                "bigquery.views-enabled config property must be enabled when bigquery.skip-view-materialization is enabled",
+                AssertTrue.class);
 
-        assertThatThrownBy(() -> new BigQueryConfig()
-                .setViewMaterializationWithFilter(true)
-                .setViewsEnabled(false)
-                .validate())
-                .isInstanceOf(IllegalStateException.class)
-                .hasMessage("bigquery.views-enabled config property must be enabled when view materialization with filter is enabled");
+        assertFailsValidation(
+                new BigQueryConfig()
+                        .setViewMaterializationWithFilter(true)
+                        .setViewsEnabled(false),
+                "validViewsEnableWhenViewMaterializationWithFilter",
+                "bigquery.views-enabled config property must be enabled when bigquery.view-materialization-with-filter is enabled",
+                AssertTrue.class);
 
-        assertThatThrownBy(() -> new BigQueryConfig()
-                .setCaseInsensitiveNameMatching(false)
-                .setCaseInsensitiveNameMatchingCacheTtl(new Duration(30, MINUTES))
-                .validate())
-                .isInstanceOf(IllegalStateException.class)
-                .hasMessage("bigquery.case-insensitive-name-matching config must be enabled when case insensitive name matching cache TTL is set");
+        assertFailsValidation(
+                new BigQueryConfig()
+                        .setCaseInsensitiveNameMatching(false)
+                        .setCaseInsensitiveNameMatchingCacheTtl(new Duration(30, MINUTES)),
+                "validCaseInsensitiveNameMatchingCacheTtl",
+                "bigquery.case-insensitive-name-matching config must be enabled when bigquery.case-insensitive-name-matching.cache-ttl is set",
+                AssertTrue.class);
     }
 }

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryProxyConfig.java

@@ -14,7 +14,7 @@
 package io.trino.plugin.bigquery;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.inject.ConfigurationException;
+import jakarta.validation.constraints.AssertTrue;
 import org.junit.jupiter.api.Test;
 
 import java.io.IOException;
@@ -26,7 +26,8 @@
 import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping;
 import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults;
 import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static io.airlift.testing.ValidationAssertions.assertFailsValidation;
+import static io.airlift.testing.ValidationAssertions.assertValidates;
 
 public class TestBigQueryProxyConfig
 {
@@ -78,26 +79,32 @@ public void testInvalidConfiguration()
         BigQueryProxyConfig config = new BigQueryProxyConfig();
         config.setUri(URI.create("http://localhost:8000/path"));
 
-        assertThatThrownBy(config::validate)
-                .isInstanceOf(ConfigurationException.class)
-                .hasMessageContaining("BigQuery RPC proxy URI cannot specify path");
+        assertFailsValidation(
+                config,
+                "uriValid",
+                "BigQuery RPC proxy URI cannot specify path",
+                AssertTrue.class);
 
         config.setUri(URI.create("http://localhost:8000"));
 
         config.setUsername("username");
 
-        assertThatThrownBy(config::validate)
-                .isInstanceOf(ConfigurationException.class)
-                .hasMessageContaining("bigquery.rpc-proxy.username was set but bigquery.rpc-proxy.password is empty");
+        assertFailsValidation(
+                config,
+                "passwordNonEmptyIfUserProvided",
+                "bigquery.rpc-proxy.username was set but bigquery.rpc-proxy.password is empty",
+                AssertTrue.class);
 
         config.setUsername(null);
         config.setPassword("password");
 
-        assertThatThrownBy(config::validate)
-                .isInstanceOf(ConfigurationException.class)
-                .hasMessageContaining("bigquery.rpc-proxy.password was set but bigquery.rpc-proxy.username is empty");
+        assertFailsValidation(
+                config,
+                "userNotEmptyIfPasswordProvided",
+                "bigquery.rpc-proxy.password was set but bigquery.rpc-proxy.username is empty",
+                AssertTrue.class);
 
         config.setUsername("username");
-        config.validate();
+        assertValidates(config);
     }
 }

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java

@@ -18,14 +18,12 @@
 import io.airlift.configuration.ConfigSecuritySensitive;
 import io.airlift.configuration.DefunctConfig;
 import io.airlift.configuration.LegacyConfig;
-import jakarta.annotation.PostConstruct;
+import jakarta.validation.constraints.AssertTrue;
 import jakarta.validation.constraints.Max;
 import jakarta.validation.constraints.Min;
 
 import java.util.Optional;
 
-import static com.google.common.base.Preconditions.checkState;
-
 @DefunctConfig("hive.metastore.glue.use-instance-credentials")
 public class GlueHiveMetastoreConfig
 {
@@ -333,12 +331,12 @@ public GlueHiveMetastoreConfig setSkipArchive(boolean skipArchive)
         return this;
     }
 
-    @PostConstruct
-    public void validate()
+    @AssertTrue(message = "Both hive.metastore.glue.region and hive.metastore.glue.endpoint-url must be provided when Glue proxy API ID is present")
+    public boolean isGlueProxyApiIdValid()
     {
         if (getGlueProxyApiId().isPresent()) {
-            checkState(getGlueRegion().isPresent() && getGlueEndpointUrl().isPresent(),
-                    "Both Glue region and Glue endpoint URL must be provided when Glue proxy API ID is present");
+            return getGlueRegion().isPresent() && getGlueEndpointUrl().isPresent();
         }
+        return true;
     }
 }

plugin/trino-kafka/pom.xml

@@ -105,11 +105,6 @@
             <artifactId>trino-record-decoder</artifactId>
         </dependency>
 
-        <dependency>
-            <groupId>jakarta.annotation</groupId>
-            <artifactId>jakarta.annotation-api</artifactId>
-        </dependency>
-
         <dependency>
             <groupId>jakarta.validation</groupId>
             <artifactId>jakarta.validation-api</artifactId>

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java

@@ -15,13 +15,11 @@
 
 import io.airlift.configuration.Config;
 import io.airlift.configuration.ConfigDescription;
-import jakarta.annotation.PostConstruct;
+import jakarta.validation.constraints.AssertTrue;
 import org.apache.kafka.common.security.auth.SecurityProtocol;
 
 import java.util.Optional;
 
-import static com.google.common.base.Preconditions.checkState;
-import static java.lang.String.format;
 import static org.apache.kafka.common.security.auth.SecurityProtocol.PLAINTEXT;
 import static org.apache.kafka.common.security.auth.SecurityProtocol.SSL;
 
@@ -42,11 +40,9 @@ public KafkaSecurityConfig setSecurityProtocol(SecurityProtocol securityProtocol
         return this;
     }
 
-    @PostConstruct
-    public void validate()
+    @AssertTrue(message = "Only PLAINTEXT and SSL security protocols are supported. See 'kafka.config.resources' if other security protocols are needed")
+    public boolean isValidSecurityProtocol()
     {
-        checkState(
-                securityProtocol == null || securityProtocol.equals(PLAINTEXT) || securityProtocol.equals(SSL),
-                format("Only %s and %s security protocols are supported. See 'kafka.config.resources' if other security protocols are needed", PLAINTEXT, SSL));
+        return securityProtocol == null || securityProtocol.equals(PLAINTEXT) || securityProtocol.equals(SSL);
     }
 }

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/security/KafkaSslConfig.java

@@ -13,15 +13,12 @@
  */
 package io.trino.plugin.kafka.security;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.inject.ConfigurationException;
-import com.google.inject.spi.Message;
 import io.airlift.configuration.Config;
 import io.airlift.configuration.ConfigDescription;
 import io.airlift.configuration.ConfigSecuritySensitive;
 import io.airlift.configuration.validation.FileExists;
-import jakarta.annotation.PostConstruct;
+import jakarta.validation.constraints.AssertTrue;
 
 import java.util.Map;
 import java.util.Optional;
@@ -176,14 +173,15 @@ public Map<String, Object> getKafkaClientProperties()
         return properties.buildOrThrow();
     }
 
-    @PostConstruct
-    public void validate()
+    @AssertTrue(message = "kafka.ssl.keystore.password must be set when kafka.ssl.keystore.location is given")
+    public boolean isKeystorePasswordValid()
     {
-        if (getKeystoreLocation().isPresent() && getKeystorePassword().isEmpty()) {
-            throw new ConfigurationException(ImmutableList.of(new Message("kafka.ssl.keystore.password must set when kafka.ssl.keystore.location is given")));
-        }
-        if (getTruststoreLocation().isPresent() && getTruststorePassword().isEmpty()) {
-            throw new ConfigurationException(ImmutableList.of(new Message("kafka.ssl.truststore.password must set when kafka.ssl.truststore.location is given")));
-        }
+        return getKeystoreLocation().isEmpty() || getKeystorePassword().isPresent();
+    }
+
+    @AssertTrue(message = "kafka.ssl.truststore.password must be set when kafka.ssl.truststore.location is given")
+    public boolean isTruststorePasswordValid()
+    {
+        return getTruststoreLocation().isEmpty() || getTruststorePassword().isPresent();
     }
 }