Skip to content

Commit

Permalink
Validate CleanupConfig when building ErrorContexts (#308)
Browse files Browse the repository at this point in the history 
* Add CleanupConfig arg to checkCommonArguments and validate if not null
* There is no need to call checkCommonArguments in buildWithJdbi3 in
  ErrorContextBuilder because the Jdbi3Context constructor performs
  validation itself (as does SimpleErrorContext)

Closes #307
  • Loading branch information
@sleberknight
sleberknight authored Oct 26, 2023
1 parent 7493b84 commit 848ee9c
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 12 deletions.
10 changes: 6 additions & 4 deletions src/main/java/org/kiwiproject/dropwizard/error/ErrorContextBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -329,8 +333,6 @@ public ErrorContext buildWithJdbi3(Jdbi jdbi) {
dataStoreType = DataStoreType.SHARED;
}

checkCommonArguments(environment, serviceDetails, dataStoreType, timeWindowValue, timeWindowUnit);

return newJdbi3ErrorContext(jdbi);
}

Expand Down
12 changes: 10 additions & 2 deletions src/main/java/org/kiwiproject/dropwizard/error/ErrorContextUtilities.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/org/kiwiproject/dropwizard/error/ErrorContextBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
31 changes: 25 additions & 6 deletions src/test/java/org/kiwiproject/dropwizard/error/ErrorContextUtilitiesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,30 @@ 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();
}

@Test
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
Expand All @@ -93,22 +101,33 @@ 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
@ValueSource(longs = {-10, -5, -1, 0})
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));
}
}

Expand Down

0 comments on commit 848ee9c

Please sign in to comment.