diff --git a/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java b/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java index bdca06a7c82a..3f7cdd6a5c52 100644 --- a/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java +++ b/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java @@ -21,7 +21,6 @@ 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; @@ -29,7 +28,6 @@ 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; } } diff --git a/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryProxyConfig.java b/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryProxyConfig.java index dcf56974c478..6e2afff41e4c 100644 --- a/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryProxyConfig.java +++ b/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryProxyConfig.java @@ -13,7 +13,6 @@ */ 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; @@ -21,7 +20,7 @@ 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) diff --git a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java index ea45a4b80416..2a0e8e9ce38b 100644 --- a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java +++ b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java @@ -15,6 +15,7 @@ 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; @@ -22,11 +23,11 @@ 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); } } diff --git a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryProxyConfig.java b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryProxyConfig.java index 7c254608442e..5d752acd93b5 100644 --- a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryProxyConfig.java +++ b/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); } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java index 52a185fa1821..2fde27caaa62 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java +++ b/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; } } diff --git a/plugin/trino-kafka/pom.xml b/plugin/trino-kafka/pom.xml index 9d258c46e5ff..0ec2d17fd084 100644 --- a/plugin/trino-kafka/pom.xml +++ b/plugin/trino-kafka/pom.xml @@ -105,11 +105,6 @@ trino-record-decoder - - jakarta.annotation - jakarta.annotation-api - - jakarta.validation jakarta.validation-api diff --git a/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java b/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java index 892dbc7ec475..e88439b1a1c8 100644 --- a/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/KafkaSecurityConfig.java +++ b/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); } } diff --git a/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/security/KafkaSslConfig.java b/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/security/KafkaSslConfig.java index 1fbe11c052ab..4fa73e8b3944 100644 --- a/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/security/KafkaSslConfig.java +++ b/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 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(); } } diff --git a/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSecurityConfig.java b/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSecurityConfig.java index 381d03e96963..b5df99541444 100644 --- a/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSecurityConfig.java +++ b/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSecurityConfig.java @@ -14,6 +14,7 @@ package io.trino.plugin.kafka; import com.google.common.collect.ImmutableMap; +import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; import java.util.Map; @@ -21,11 +22,12 @@ 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 io.airlift.testing.ValidationAssertions.assertValidates; import static org.apache.kafka.common.security.auth.SecurityProtocol.PLAINTEXT; import static org.apache.kafka.common.security.auth.SecurityProtocol.SASL_PLAINTEXT; import static org.apache.kafka.common.security.auth.SecurityProtocol.SASL_SSL; import static org.apache.kafka.common.security.auth.SecurityProtocol.SSL; -import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestKafkaSecurityConfig { @@ -50,28 +52,24 @@ public void testExplicitPropertyMappings() @Test public void testValidSecurityProtocols() { - new KafkaSecurityConfig() - .setSecurityProtocol(PLAINTEXT) - .validate(); + assertValidates(new KafkaSecurityConfig() + .setSecurityProtocol(PLAINTEXT)); - new KafkaSecurityConfig() - .setSecurityProtocol(SSL) - .validate(); + assertValidates(new KafkaSecurityConfig() + .setSecurityProtocol(SSL)); } @Test public void testInvalidSecurityProtocol() { - assertThatThrownBy(() -> new KafkaSecurityConfig() - .setSecurityProtocol(SASL_PLAINTEXT) - .validate()) - .isInstanceOf(IllegalStateException.class) - .hasMessage("Only PLAINTEXT and SSL security protocols are supported. See 'kafka.config.resources' if other security protocols are needed"); + assertFailsValidation(new KafkaSecurityConfig().setSecurityProtocol(SASL_PLAINTEXT), + "validSecurityProtocol", + "Only PLAINTEXT and SSL security protocols are supported. See 'kafka.config.resources' if other security protocols are needed", + AssertTrue.class); - assertThatThrownBy(() -> new KafkaSecurityConfig() - .setSecurityProtocol(SASL_SSL) - .validate()) - .isInstanceOf(IllegalStateException.class) - .hasMessage("Only PLAINTEXT and SSL security protocols are supported. See 'kafka.config.resources' if other security protocols are needed"); + assertFailsValidation(new KafkaSecurityConfig().setSecurityProtocol(SASL_SSL), + "validSecurityProtocol", + "Only PLAINTEXT and SSL security protocols are supported. See 'kafka.config.resources' if other security protocols are needed", + AssertTrue.class); } } diff --git a/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSslConfig.java b/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSslConfig.java index ad801a437f05..57bb668169ed 100644 --- a/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSslConfig.java +++ b/plugin/trino-kafka/src/test/java/io/trino/plugin/kafka/TestKafkaSslConfig.java @@ -14,9 +14,9 @@ package io.trino.plugin.kafka; import com.google.common.collect.ImmutableMap; -import com.google.inject.ConfigurationException; import io.trino.plugin.kafka.security.KafkaEndpointIdentificationAlgorithm; import io.trino.plugin.kafka.security.KafkaSslConfig; +import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; import java.io.FileWriter; @@ -28,6 +28,7 @@ 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 io.trino.plugin.kafka.security.KafkaEndpointIdentificationAlgorithm.DISABLED; import static io.trino.plugin.kafka.security.KafkaEndpointIdentificationAlgorithm.HTTPS; import static io.trino.plugin.kafka.security.KafkaKeystoreTruststoreType.JKS; @@ -44,7 +45,6 @@ import static org.apache.kafka.common.config.SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG; import static org.apache.kafka.common.security.auth.SecurityProtocol.SSL; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestKafkaSslConfig { @@ -148,9 +148,12 @@ public void testFailOnMissingKeystorePasswordWithKeystoreLocationSet() KafkaSslConfig config = new KafkaSslConfig(); config.setKeystoreLocation(keystorePath.toString()); - assertThatThrownBy(config::validate) - .isInstanceOf(ConfigurationException.class) - .hasMessageContaining("kafka.ssl.keystore.password must set when kafka.ssl.keystore.location is given"); + + assertFailsValidation( + config, + "keystorePasswordValid", + "kafka.ssl.keystore.password must be set when kafka.ssl.keystore.location is given", + AssertTrue.class); } @Test @@ -164,9 +167,12 @@ public void testFailOnMissingTruststorePasswordWithTruststoreLocationSet() KafkaSslConfig config = new KafkaSslConfig(); config.setTruststoreLocation(truststorePath.toString()); - assertThatThrownBy(config::validate) - .isInstanceOf(ConfigurationException.class) - .hasMessageContaining("kafka.ssl.truststore.password must set when kafka.ssl.truststore.location is given"); + + assertFailsValidation( + config, + "truststorePasswordValid", + "kafka.ssl.truststore.password must be set when kafka.ssl.truststore.location is given", + AssertTrue.class); } private void writeToFile(Path filepath, String content) diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java index c681d5f53743..ae070687d3c9 100755 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java @@ -21,7 +21,6 @@ import io.airlift.units.DataSize; import io.airlift.units.Duration; import io.airlift.units.MinDuration; -import jakarta.annotation.PostConstruct; import jakarta.validation.constraints.AssertTrue; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.NotNull; @@ -32,7 +31,6 @@ import java.util.concurrent.TimeUnit; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static io.airlift.units.DataSize.Unit.MEGABYTE; @@ -258,12 +256,10 @@ public PinotConfig setTargetSegmentPageSize(DataSize targetSegmentPageSize) return this; } - @PostConstruct - public void validate() + @AssertTrue(message = "Invalid configuration: pinot.aggregation-pushdown.enabled must be enabled if pinot.count-distinct-pushdown.enabled") + public boolean isValidConfiguration() { - checkState( - !countDistinctPushdownEnabled || aggregationPushdownEnabled, - "Invalid configuration: pinot.aggregation-pushdown.enabled must be enabled if pinot.count-distinct-pushdown.enabled"); + return !countDistinctPushdownEnabled || aggregationPushdownEnabled; } @AssertTrue(message = "All controller URLs must have the same scheme") diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java index 448eaf2e394f..a0fb359dd439 100644 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java @@ -13,13 +13,10 @@ */ package io.trino.plugin.pinot.client; -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.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; @@ -126,14 +123,15 @@ public PinotGrpcServerQueryClientTlsConfig setSslProvider(String sslProvider) return this; } - @PostConstruct - public void validate() + @AssertTrue(message = "pinot.grpc.tls.keystore-password must be set when pinot.grpc.tls.keystore-path is given") + public boolean isKeystorePasswordValid() { - if (getKeystorePath().isPresent() && getKeystorePassword().isEmpty()) { - throw new ConfigurationException(ImmutableList.of(new Message("pinot.grpc.tls.keystore-password must set when pinot.grpc.tls.keystore-path is given"))); - } - if (getTruststorePath().isPresent() && getTruststorePassword().isEmpty()) { - throw new ConfigurationException(ImmutableList.of(new Message("pinot.grpc.tls.truststore-password must set when pinot.grpc.tls.truststore-path is given"))); - } + return getKeystorePath().isEmpty() || getKeystorePassword().isPresent(); + } + + @AssertTrue(message = "pinot.grpc.tls.truststore-password must be set when pinot.grpc.tls.truststore-path is given") + public boolean isTruststorePasswordValid() + { + return getTruststorePath().isEmpty() || getTruststorePassword().isPresent(); } } diff --git a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotConfig.java b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotConfig.java index 21597d7fed52..36d08af59a49 100755 --- a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotConfig.java +++ b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotConfig.java @@ -19,14 +19,15 @@ import io.airlift.configuration.testing.ConfigAssertions; import io.airlift.units.DataSize; import io.airlift.units.Duration; +import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; import java.util.Map; import java.util.concurrent.TimeUnit; +import static io.airlift.testing.ValidationAssertions.assertFailsValidation; import static io.airlift.units.DataSize.Unit.MEGABYTE; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestPinotConfig { @@ -93,12 +94,13 @@ public void testExplicitPropertyMappings() @Test public void testInvalidCountDistinctPushdown() { - assertThatThrownBy(() -> new PinotConfig() - .setAggregationPushdownEnabled(false) - .setCountDistinctPushdownEnabled(true) - .validate()) - .isInstanceOf(IllegalStateException.class) - .hasMessage("Invalid configuration: pinot.aggregation-pushdown.enabled must be enabled if pinot.count-distinct-pushdown.enabled"); + assertFailsValidation( + new PinotConfig() + .setAggregationPushdownEnabled(false) + .setCountDistinctPushdownEnabled(true), + "validConfiguration", + "Invalid configuration: pinot.aggregation-pushdown.enabled must be enabled if pinot.count-distinct-pushdown.enabled", + AssertTrue.class); } @Test diff --git a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java index 97dfd5754d75..b53121220b1b 100644 --- a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java +++ b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java @@ -14,9 +14,9 @@ package io.trino.plugin.pinot; import com.google.common.collect.ImmutableMap; -import com.google.inject.ConfigurationException; import io.airlift.configuration.testing.ConfigAssertions; import io.trino.plugin.pinot.client.PinotGrpcServerQueryClientTlsConfig; +import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; import java.io.FileWriter; @@ -25,10 +25,10 @@ import java.nio.file.Path; import java.util.Map; +import static io.airlift.testing.ValidationAssertions.assertFailsValidation; import static io.trino.plugin.pinot.client.PinotKeystoreTrustStoreType.JKS; import static io.trino.plugin.pinot.client.PinotKeystoreTrustStoreType.PKCS12; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestPinotGrpcServerQueryClientTlsConfig { @@ -84,9 +84,12 @@ public void testFailOnMissingKeystorePasswordWithKeystorePathSet() PinotGrpcServerQueryClientTlsConfig config = new PinotGrpcServerQueryClientTlsConfig(); config.setKeystorePath(keystorePath.toFile()); - assertThatThrownBy(config::validate) - .isInstanceOf(ConfigurationException.class) - .hasMessageContaining("pinot.grpc.tls.keystore-password must set when pinot.grpc.tls.keystore-path is given"); + + assertFailsValidation( + config, + "keystorePasswordValid", + "pinot.grpc.tls.keystore-password must be set when pinot.grpc.tls.keystore-path is given", + AssertTrue.class); } @Test @@ -100,9 +103,12 @@ public void testFailOnMissingTruststorePasswordWithTruststorePathSet() PinotGrpcServerQueryClientTlsConfig config = new PinotGrpcServerQueryClientTlsConfig(); config.setTruststorePath(truststorePath.toFile()); - assertThatThrownBy(config::validate) - .isInstanceOf(ConfigurationException.class) - .hasMessageContaining("pinot.grpc.tls.truststore-password must set when pinot.grpc.tls.truststore-path is given"); + + assertFailsValidation( + config, + "truststorePasswordValid", + "pinot.grpc.tls.truststore-password must be set when pinot.grpc.tls.truststore-path is given", + AssertTrue.class); } private void writeToFile(Path filepath, String content) diff --git a/plugin/trino-prometheus/src/main/java/io/trino/plugin/prometheus/PrometheusConnectorConfig.java b/plugin/trino-prometheus/src/main/java/io/trino/plugin/prometheus/PrometheusConnectorConfig.java index d31d7c133fc1..0f347dcbc575 100644 --- a/plugin/trino-prometheus/src/main/java/io/trino/plugin/prometheus/PrometheusConnectorConfig.java +++ b/plugin/trino-prometheus/src/main/java/io/trino/plugin/prometheus/PrometheusConnectorConfig.java @@ -13,17 +13,14 @@ */ package io.trino.plugin.prometheus; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.net.HttpHeaders; -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.units.Duration; import io.airlift.units.MinDuration; -import jakarta.annotation.PostConstruct; +import jakarta.validation.constraints.AssertTrue; import jakarta.validation.constraints.NotNull; import java.io.File; @@ -216,22 +213,29 @@ public PrometheusConnectorConfig setAdditionalHeaders(String httpHeaders) return this; } - @PostConstruct - public void checkConfig() + @AssertTrue(message = "prometheus.max.query.range.duration must be greater than prometheus.query.chunk.size.duration") + public boolean isMaxQueryRangeDurationValid() { long maxQueryRangeDuration = (long) getMaxQueryRangeDuration().getValue(TimeUnit.SECONDS); long queryChunkSizeDuration = (long) getQueryChunkSizeDuration().getValue(TimeUnit.SECONDS); - if (maxQueryRangeDuration < queryChunkSizeDuration) { - throw new ConfigurationException(ImmutableList.of(new Message("prometheus.max.query.range.duration must be greater than prometheus.query.chunk.size.duration"))); - } - if (getBearerTokenFile().isPresent() && (getUser().isPresent() || getPassword().isPresent())) { - throw new IllegalStateException("Either on of bearer token file or basic authentication should be used"); - } - if (getUser().isPresent() ^ getPassword().isPresent()) { - throw new IllegalStateException("Both username and password must be set when using basic authentication"); - } - if (getAdditionalHeaders().containsKey(httpAuthHeaderName)) { - throw new IllegalStateException("Additional headers can not include: " + httpAuthHeaderName); - } + return maxQueryRangeDuration >= queryChunkSizeDuration; + } + + @AssertTrue(message = "Either one of bearer token file or basic authentication should be used") + public boolean isAuthConfigValid() + { + return !(getBearerTokenFile().isPresent() && (getUser().isPresent() || getPassword().isPresent())); + } + + @AssertTrue(message = "Both username and password must be set when using basic authentication") + public boolean isBasicAuthConfigValid() + { + return getUser().isPresent() == getPassword().isPresent(); + } + + @AssertTrue(message = "Additional headers can not include authorization header") + public boolean isAdditionalHeadersValid() + { + return !getAdditionalHeaders().containsKey(httpAuthHeaderName); } } diff --git a/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusConnectorConfig.java b/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusConnectorConfig.java index 5a8d2c4e81f3..1d2c6e024868 100644 --- a/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusConnectorConfig.java +++ b/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusConnectorConfig.java @@ -15,20 +15,21 @@ import com.google.common.collect.ImmutableMap; import com.google.common.net.HttpHeaders; -import com.google.inject.ConfigurationException; +import io.airlift.configuration.ConfigurationFactory; import io.airlift.units.Duration; +import jakarta.validation.constraints.AssertTrue; import org.junit.jupiter.api.Test; import java.io.File; import java.net.URI; 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.SECONDS; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; public class TestPrometheusConnectorConfig { @@ -50,7 +51,7 @@ public void testDefaults() } @Test - public void testExplicitPropertyMappings() + public void testExplicitPropertyMappingsWithBearerTokenFile() { Map properties = ImmutableMap.builder() .put("prometheus.uri", "file://test.json") @@ -59,28 +60,26 @@ public void testExplicitPropertyMappings() .put("prometheus.cache.ttl", "60s") .put("prometheus.auth.http.header.name", "X-team-auth") .put("prometheus.bearer.token.file", "/tmp/bearer_token.txt") - .put("prometheus.auth.user", "admin") - .put("prometheus.auth.password", "password") .put("prometheus.read-timeout", "30s") .put("prometheus.case-insensitive-name-matching", "true") .put("prometheus.http.additional-headers", "key\\:1:value\\,1, key\\,2:value\\:2") .buildOrThrow(); - URI uri = URI.create("file://test.json"); - PrometheusConnectorConfig expected = new PrometheusConnectorConfig(); - expected.setPrometheusURI(uri); - expected.setQueryChunkSizeDuration(new Duration(365, DAYS)); - expected.setMaxQueryRangeDuration(new Duration(1095, DAYS)); - expected.setCacheDuration(new Duration(60, SECONDS)); - expected.setHttpAuthHeaderName("X-team-auth"); - expected.setBearerTokenFile(new File("/tmp/bearer_token.txt")); - expected.setUser("admin"); - expected.setPassword("password"); - expected.setReadTimeout(new Duration(30, SECONDS)); - expected.setCaseInsensitiveNameMatching(true); - expected.setAdditionalHeaders("key\\:1:value\\,1, key\\,2:value\\:2"); + ConfigurationFactory configurationFactory = new ConfigurationFactory(properties); + PrometheusConnectorConfig config = configurationFactory.build(PrometheusConnectorConfig.class); - assertFullMapping(properties, expected); + URI uri = URI.create("file://test.json"); + assertThat(config.getPrometheusURI()).isEqualTo(uri); + assertThat(config.getQueryChunkSizeDuration()).isEqualTo(new Duration(365, DAYS)); + assertThat(config.getMaxQueryRangeDuration()).isEqualTo(new Duration(1095, DAYS)); + assertThat(config.getCacheDuration()).isEqualTo(new Duration(60, SECONDS)); + assertThat(config.getHttpAuthHeaderName()).isEqualTo("X-team-auth"); + assertThat(config.getBearerTokenFile()).contains(new File("/tmp/bearer_token.txt")); + assertThat(config.getUser()).isEmpty(); + assertThat(config.getPassword()).isEmpty(); + assertThat(config.getReadTimeout()).isEqualTo(new Duration(30, SECONDS)); + assertThat(config.isCaseInsensitiveNameMatching()).isTrue(); + assertThat(config.getAdditionalHeaders()).isEqualTo(ImmutableMap.of("key\\:1", "value\\,1", "key\\,2", "value\\:2")); } @Test @@ -91,33 +90,45 @@ public void testFailOnDurationLessThanQueryChunkConfig() config.setQueryChunkSizeDuration(new Duration(21, DAYS)); config.setMaxQueryRangeDuration(new Duration(1, DAYS)); config.setCacheDuration(new Duration(30, SECONDS)); - assertThatThrownBy(config::checkConfig) - .isInstanceOf(ConfigurationException.class) - .hasMessageContaining("prometheus.max.query.range.duration must be greater than prometheus.query.chunk.size.duration"); + + assertFailsValidation( + config, + "maxQueryRangeDurationValid", + "prometheus.max.query.range.duration must be greater than prometheus.query.chunk.size.duration", + AssertTrue.class); } @Test public void testInvalidAuth() { - assertThatThrownBy(new PrometheusConnectorConfig().setBearerTokenFile(new File("/tmp/bearer_token.txt")).setUser("test")::checkConfig) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Either on of bearer token file or basic authentication should be used"); + assertFailsValidation( + new PrometheusConnectorConfig().setBearerTokenFile(new File("/tmp/bearer_token.txt")).setUser("test"), + "authConfigValid", + "Either one of bearer token file or basic authentication should be used", + AssertTrue.class); - assertThatThrownBy(new PrometheusConnectorConfig().setBearerTokenFile(new File("/tmp/bearer_token.txt")).setPassword("test")::checkConfig) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Either on of bearer token file or basic authentication should be used"); + assertFailsValidation( + new PrometheusConnectorConfig().setBearerTokenFile(new File("/tmp/bearer_token.txt")).setPassword("test"), + "authConfigValid", + "Either one of bearer token file or basic authentication should be used", + AssertTrue.class); - assertThatThrownBy(new PrometheusConnectorConfig().setUser("test")::checkConfig) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Both username and password must be set when using basic authentication"); + assertFailsValidation( + new PrometheusConnectorConfig().setUser("test"), + "basicAuthConfigValid", + "Both username and password must be set when using basic authentication", + AssertTrue.class); - assertThatThrownBy(new PrometheusConnectorConfig().setPassword("test")::checkConfig) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Both username and password must be set when using basic authentication"); + assertFailsValidation( + new PrometheusConnectorConfig().setPassword("test"), + "basicAuthConfigValid", + "Both username and password must be set when using basic authentication", + AssertTrue.class); - assertThatThrownBy(new PrometheusConnectorConfig().setAdditionalHeaders("Authorization: test").setHttpAuthHeaderName("Authorization") - ::checkConfig) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Additional headers can not include: Authorization"); + assertFailsValidation( + new PrometheusConnectorConfig().setAdditionalHeaders("Authorization: test").setHttpAuthHeaderName("Authorization"), + "additionalHeadersValid", + "Additional headers can not include authorization header", + AssertTrue.class); } }