diff --git a/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextBuilder.java b/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextBuilder.java index e5f72b8..e36c394 100644 --- a/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextBuilder.java +++ b/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextBuilder.java @@ -276,7 +276,9 @@ public ErrorContext buildInMemoryH2() { dataStoreType = DataStoreType.NOT_SHARED; } - checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit); + // Check arguments before creating database and Jdbi instance + // (this avoids doing work if context creation will ultimately fail) + checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit, cleanupConfig); var dataSourceFactory = createInMemoryH2Database(); var jdbi = Jdbi3Builders.buildManagedJdbi(environment, dataSourceFactory, DEFAULT_DATABASE_HEALTH_CHECK_NAME); @@ -300,7 +302,9 @@ public ErrorContext buildWithDataStoreFactory(DataSourceFactory dataSourceFactor dataStoreType = ApplicationErrorJdbc.dataStoreTypeOf(dataSourceFactory); } - checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit); + // Check arguments before creating Jdbi instance + // (this avoids doing work if context creation will ultimately fail) + checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit, cleanupConfig); LOG.info("Creating a {} JDBI (version 3) ErrorContext instance from the dataSourceFactory", dataStoreType); var jdbi = Jdbi3Builders.buildManagedJdbi(environment, dataSourceFactory, DEFAULT_DATABASE_HEALTH_CHECK_NAME); @@ -329,8 +333,6 @@ public ErrorContext buildWithJdbi3(Jdbi jdbi) { dataStoreType = DataStoreType.SHARED; } - checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit); - return newJdbi3ErrorContext(jdbi); } diff --git a/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextUtilities.java b/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextUtilities.java index 9a10830..7f0d61b 100644 --- a/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextUtilities.java +++ b/src/main/java/org/kiwiproject/dropwizard/error/ErrorContextUtilities.java @@ -1,12 +1,15 @@ package org.kiwiproject.dropwizard.error; import static com.google.common.base.Preconditions.checkArgument; +import static java.util.Objects.nonNull; import static java.util.Objects.requireNonNull; import static org.kiwiproject.base.KiwiPreconditions.checkArgumentNotNull; +import static org.kiwiproject.validation.KiwiValidations.checkArgumentValid; import io.dropwizard.core.setup.Environment; import lombok.experimental.UtilityClass; import org.checkerframework.checker.nullness.qual.Nullable; +import org.kiwiproject.dropwizard.error.config.CleanupConfig; import org.kiwiproject.dropwizard.error.dao.ApplicationErrorDao; import org.kiwiproject.dropwizard.error.health.RecentErrorsHealthCheck; import org.kiwiproject.dropwizard.error.job.CleanupApplicationErrorsJob; @@ -37,20 +40,25 @@ static void checkCommonArguments(Environment environment, serviceDetails, options.getDataStoreType(), options.getTimeWindowValue(), - options.getTimeWindowUnit()); + options.getTimeWindowUnit(), + options.getCleanupConfig()); } static void checkCommonArguments(Environment environment, ServiceDetails serviceDetails, DataStoreType dataStoreType, long timeWindowValue, - TemporalUnit timeWindowUnit) { + TemporalUnit timeWindowUnit, + CleanupConfig cleanupConfig) { checkArgumentNotNull(environment, "Dropwizard Environment cannot be null"); checkArgumentNotNull(serviceDetails, "serviceDetails cannot be null"); checkArgumentNotNull(dataStoreType, "dataStoreType cannot be null"); checkArgument(timeWindowValue > 0, "timeWindowValue must be positive"); checkArgumentNotNull(timeWindowUnit, "timeWindowUnit cannot be null"); + if (nonNull(cleanupConfig)) { + checkArgumentValid(cleanupConfig); + } } static PersistentHostInformation setPersistentHostInformationFrom(ServiceDetails serviceDetails) { diff --git a/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextBuilderTest.java b/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextBuilderTest.java index b0a7189..c3dfb5b 100644 --- a/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextBuilderTest.java +++ b/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextBuilderTest.java @@ -46,6 +46,8 @@ import org.kiwiproject.dropwizard.jdbi3.Jdbi3Builders; import org.kiwiproject.test.dropwizard.mockito.DropwizardMockitoMocks; import org.kiwiproject.test.junit.jupiter.PostgresLiquibaseTestExtension; +import org.kiwiproject.validation.KiwiConstraintViolations; +import org.kiwiproject.validation.KiwiValidations; import org.postgresql.Driver; import java.time.temporal.ChronoUnit; @@ -130,6 +132,25 @@ void whenTimeWindowValueIsNotPositive(long amount, SoftAssertions softly) { assertIllegalArgumentExceptionThrownBuilding(softly, builder, "timeWindowValue must be positive"); } + @Test + void whenCleanupConfigIsNotValid(SoftAssertions softly) { + var cleanupConfig = new CleanupConfig(); + cleanupConfig.setCleanupStrategy(null); + cleanupConfig.setInitialJobDelay(Duration.seconds(-1)); + + var builder = ErrorContextBuilder.newInstance() + .environment(environment) + .serviceDetails(serviceDetails) + .dataStoreType(DataStoreType.SHARED) + .timeWindowValue(timeWindowAmount) + .timeWindowUnit(timeWindowUnit) + .cleanupConfig(cleanupConfig); + + var violations = KiwiValidations.validate(cleanupConfig); + var expectedMessage = KiwiConstraintViolations.simpleCombinedErrorMessage(violations); + assertIllegalArgumentExceptionThrownBuilding(softly, builder, expectedMessage); + } + private void assertIllegalArgumentExceptionThrownBuilding(SoftAssertions softly, ErrorContextBuilder builder, String expectedMessage) { diff --git a/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextUtilitiesTest.java b/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextUtilitiesTest.java index 47958c4..f23bad0 100644 --- a/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextUtilitiesTest.java +++ b/src/test/java/org/kiwiproject/dropwizard/error/ErrorContextUtilitiesTest.java @@ -64,7 +64,15 @@ class CheckCommonArguments { @Test void shouldNotThrowException_GivenValidArguments() { assertThatCode(() -> - ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 10, ChronoUnit.MINUTES)) + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 10, ChronoUnit.MINUTES, new CleanupConfig())) + .doesNotThrowAnyException(); + } + + @Test + void shouldAllowNullCleanupConfig() { + assertThatCode(() -> + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 10, ChronoUnit.MINUTES, null)) + .describedAs("cleanupConfig can be null when not adding cleanup job") .doesNotThrowAnyException(); } @@ -72,14 +80,14 @@ void shouldNotThrowException_GivenValidArguments() { void shouldThrowIllegalArgumentException_GivenNullEnvironment() { assertThatIllegalArgumentException() .isThrownBy(() -> - ErrorContextUtilities.checkCommonArguments(null, serviceDetails, DataStoreType.SHARED, 15, ChronoUnit.MINUTES)); + ErrorContextUtilities.checkCommonArguments(null, serviceDetails, DataStoreType.SHARED, 15, ChronoUnit.MINUTES, new CleanupConfig())); } @Test void shouldThrowIllegalArgumentException_GivenNullServiceDetails() { assertThatIllegalArgumentException() .isThrownBy(() -> - ErrorContextUtilities.checkCommonArguments(environment, null, DataStoreType.NOT_SHARED, 15, ChronoUnit.MINUTES)); + ErrorContextUtilities.checkCommonArguments(environment, null, DataStoreType.NOT_SHARED, 15, ChronoUnit.MINUTES, new CleanupConfig())); } @Test @@ -93,7 +101,7 @@ void shouldThrowIllegalArgumentException_GivenNullOptions() { void shouldThrowIllegalArgumentException_GivenNullDataStoreType() { assertThatIllegalArgumentException() .isThrownBy(() -> - ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, null, 15, ChronoUnit.MINUTES)); + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, null, 15, ChronoUnit.MINUTES, new CleanupConfig())); } @ParameterizedTest @@ -101,14 +109,25 @@ void shouldThrowIllegalArgumentException_GivenNullDataStoreType() { void shouldThrowIllegalArgumentException_GivenZeroOrNegativeTimeWindowValue(long value) { assertThatIllegalArgumentException() .isThrownBy(() -> - ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, value, ChronoUnit.MINUTES)); + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, value, ChronoUnit.MINUTES, new CleanupConfig())); } @Test void shouldThrowIllegalArgumentException_GivenNullTimeWindowUnit() { assertThatIllegalArgumentException() .isThrownBy(() -> - ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 15, null)); + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 15, null, new CleanupConfig())); + } + + @Test + void shouldThrowIllegalArgumentException_GivenInvalidCleanupConfig() { + var cleanupConfig = new CleanupConfig(); + cleanupConfig.setCleanupStrategy(null); + cleanupConfig.setInitialJobDelay(io.dropwizard.util.Duration.seconds(-1)); + + assertThatIllegalArgumentException() + .isThrownBy(() -> + ErrorContextUtilities.checkCommonArguments(environment, serviceDetails, DataStoreType.SHARED, 25, ChronoUnit.MINUTES, cleanupConfig)); } }