From b058d2efd1fa7589639db0e01136c23926ba7ff6 Mon Sep 17 00:00:00 2001 From: David Venable Date: Tue, 4 Jan 2022 14:03:49 -0600 Subject: [PATCH] Validate plugin configuration objects using JSR-303/380 validation. Hibernate Validator provides this validation, removing the need for bval. Signed-off-by: David Venable --- .../InvalidPluginConfigurationException.java | 18 ++++ ...validPluginConfigurationExceptionTest.java | 33 +++++++ data-prepper-core/build.gradle | 4 +- .../plugin/PluginConfigurationConverter.java | 41 ++++++++- .../plugin/DefaultPluginFactoryIT.java | 88 +++++++++++++++++++ .../PluginConfigurationConverterTest.java | 74 +++++++++++++++- .../plugin/TestPluggableInterface.java | 9 ++ .../plugin/TestPluginConfiguration.java | 31 +++++++ .../dataprepper/plugins/TestPlugin.java | 29 ++++++ 9 files changed, 323 insertions(+), 4 deletions(-) create mode 100644 data-prepper-api/src/main/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationException.java create mode 100644 data-prepper-api/src/test/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationExceptionTest.java create mode 100644 data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/DefaultPluginFactoryIT.java create mode 100644 data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluggableInterface.java create mode 100644 data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluginConfiguration.java create mode 100644 data-prepper-core/src/test/java/com/amazon/dataprepper/plugins/TestPlugin.java diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationException.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationException.java new file mode 100644 index 0000000000..354aff721e --- /dev/null +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationException.java @@ -0,0 +1,18 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.model.plugin; + +/** + * This exception is thrown when a plugin in configured with an invalid + * configuration. + * + * @since 1.3 + */ +public class InvalidPluginConfigurationException extends RuntimeException { + public InvalidPluginConfigurationException(final String message) { + super(message); + } +} diff --git a/data-prepper-api/src/test/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationExceptionTest.java b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationExceptionTest.java new file mode 100644 index 0000000000..c26c121127 --- /dev/null +++ b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/plugin/InvalidPluginConfigurationExceptionTest.java @@ -0,0 +1,33 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.model.plugin; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.UUID; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.MatcherAssert.assertThat; + +class InvalidPluginConfigurationExceptionTest { + private String message; + + @BeforeEach + void setUp() { + message = UUID.randomUUID().toString(); + } + + private InvalidPluginConfigurationException createObjectUnderTest() { + return new InvalidPluginConfigurationException(message); + } + + @Test + void getMessage_returns_message() { + assertThat(createObjectUnderTest().getMessage(), + equalTo(message)); + } +} \ No newline at end of file diff --git a/data-prepper-core/build.gradle b/data-prepper-core/build.gradle index a400b013fa..b4d4ddd6d1 100644 --- a/data-prepper-core/build.gradle +++ b/data-prepper-core/build.gradle @@ -20,13 +20,13 @@ dependencies { implementation 'com.fasterxml.jackson.core:jackson-databind' implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml' implementation "javax.validation:validation-api:2.0.1.Final" - implementation "org.apache.bval:bval-extras:2.0.5" - implementation "org.apache.bval:bval-jsr:2.0.5" implementation "org.reflections:reflections:0.10.2" implementation 'io.micrometer:micrometer-core' implementation 'io.micrometer:micrometer-registry-prometheus' implementation 'io.micrometer:micrometer-registry-cloudwatch2' implementation 'software.amazon.awssdk:cloudwatch' + implementation 'org.hibernate.validator:hibernate-validator:7.0.2.Final' + implementation 'org.glassfish:jakarta.el:4.0.1' implementation platform('org.apache.logging.log4j:log4j-bom:2.17.1') implementation 'org.apache.logging.log4j:log4j-core' implementation 'org.apache.logging.log4j:log4j-slf4j-impl' diff --git a/data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginConfigurationConverter.java b/data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginConfigurationConverter.java index 517a35d41c..13be439931 100644 --- a/data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginConfigurationConverter.java +++ b/data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginConfigurationConverter.java @@ -5,19 +5,44 @@ package com.amazon.dataprepper.plugin; +import com.amazon.dataprepper.model.annotations.DataPrepperPlugin; import com.amazon.dataprepper.model.configuration.PluginSetting; +import com.amazon.dataprepper.model.plugin.InvalidPluginConfigurationException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.PropertyNamingStrategies; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Validation; +import jakarta.validation.Validator; +import jakarta.validation.ValidatorFactory; import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; +/** + * Converts and validates a plugin configuration. This class is responsible for taking a {@link PluginSetting} + * and converting it to the plugin model type which should be denoted by {@link DataPrepperPlugin#pluginConfigurationType()} + */ class PluginConfigurationConverter { private final ObjectMapper objectMapper; + private final Validator validator; PluginConfigurationConverter() { this.objectMapper = new ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE); + + final ValidatorFactory validationFactory = Validation.buildDefaultValidatorFactory(); + validator = validationFactory.getValidator(); } + /** + * Converts and validates to a plugin model type. The conversion happens via + * Java Bean Validation 2.0. + * + * @param pluginConfigurationType the destination type + * @param pluginSetting The source {@link PluginSetting} + * @return The converted object of type pluginConfigurationType + * @throws InvalidPluginConfigurationException - If the plugin configuration is invalid + */ public Object convert(final Class pluginConfigurationType, final PluginSetting pluginSetting) { Objects.requireNonNull(pluginConfigurationType); Objects.requireNonNull(pluginSetting); @@ -25,6 +50,20 @@ public Object convert(final Class pluginConfigurationType, final PluginSettin if(pluginConfigurationType.equals(PluginSetting.class)) return pluginSetting; - return objectMapper.convertValue(pluginSetting.getSettings(), pluginConfigurationType); + final Object configuration = objectMapper.convertValue(pluginSetting.getSettings(), pluginConfigurationType); + + final Set> constraintViolations = validator.validate(configuration); + + if(!constraintViolations.isEmpty()) { + final String violationsString = constraintViolations.stream() + .map(v -> v.getPropertyPath().toString() + " " + v.getMessage()) + .collect(Collectors.joining(". ")); + + final String exceptionMessage = String.format("Plugin %s in pipeline %s is configured incorrectly: %s", + pluginSetting.getName(), pluginSetting.getPipelineName(), violationsString); + throw new InvalidPluginConfigurationException(exceptionMessage); + } + + return configuration; } } diff --git a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/DefaultPluginFactoryIT.java b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/DefaultPluginFactoryIT.java new file mode 100644 index 0000000000..b2d39b558d --- /dev/null +++ b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/DefaultPluginFactoryIT.java @@ -0,0 +1,88 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.plugin; + +import com.amazon.dataprepper.model.configuration.PluginSetting; +import com.amazon.dataprepper.model.plugin.InvalidPluginConfigurationException; +import com.amazon.dataprepper.plugins.TestPlugin; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Integration test of the plugin framework. These tests should not mock any portion + * of the plugin framework. But, they may mock inputs when appropriate. + */ +class DefaultPluginFactoryIT { + + private String pluginName; + private String pipelineName; + + @BeforeEach + void setUp() { + pluginName = "test_plugin"; + pipelineName = UUID.randomUUID().toString(); + } + + private DefaultPluginFactory createObjectUnderTest() { + return new DefaultPluginFactory(); + } + + @Test + void loadPlugin_should_return_a_new_plugin_instance_with_the_expected_configuration() { + + final String requiredStringValue = UUID.randomUUID().toString(); + final String optionalStringValue = UUID.randomUUID().toString(); + + final Map pluginSettingMap = new HashMap<>(); + pluginSettingMap.put("required_string", requiredStringValue); + pluginSettingMap.put("optional_string", optionalStringValue); + final PluginSetting pluginSetting = createPluginSettings(pluginSettingMap); + + final TestPluggableInterface plugin = createObjectUnderTest().loadPlugin(TestPluggableInterface.class, pluginSetting); + + assertThat(plugin, instanceOf(TestPlugin.class)); + + final TestPlugin testPlugin = (TestPlugin) plugin; + + final TestPluginConfiguration configuration = testPlugin.getConfiguration(); + + assertThat(configuration.getRequiredString(), equalTo(requiredStringValue)); + assertThat(configuration.getOptionalString(), equalTo(optionalStringValue)); + } + + @Test + void loadPlugin_should_throw_when_a_plugin_configuration_is_invalid() { + final String optionalStringValue = UUID.randomUUID().toString(); + + final Map pluginSettingMap = new HashMap<>(); + pluginSettingMap.put("optional_string", optionalStringValue); + final PluginSetting pluginSetting = createPluginSettings(pluginSettingMap); + + final DefaultPluginFactory objectUnderTest = createObjectUnderTest(); + + final InvalidPluginConfigurationException actualException = assertThrows(InvalidPluginConfigurationException.class, + () -> objectUnderTest.loadPlugin(TestPluggableInterface.class, pluginSetting)); + + assertThat(actualException.getMessage(), notNullValue()); + assertThat(actualException.getMessage(), equalTo("Plugin test_plugin in pipeline " + pipelineName + " is configured incorrectly: requiredString must not be null")); + } + + private PluginSetting createPluginSettings(final Map pluginSettingMap) { + final PluginSetting pluginSetting = new PluginSetting(pluginName, pluginSettingMap); + pluginSetting.setPipelineName(pipelineName); + return pluginSetting; + } +} \ No newline at end of file diff --git a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/PluginConfigurationConverterTest.java b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/PluginConfigurationConverterTest.java index 5cb0b0a235..fe1fe452ea 100644 --- a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/PluginConfigurationConverterTest.java +++ b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/PluginConfigurationConverterTest.java @@ -6,26 +6,38 @@ package com.amazon.dataprepper.plugin; import com.amazon.dataprepper.model.configuration.PluginSetting; +import com.amazon.dataprepper.model.plugin.InvalidPluginConfigurationException; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Path; +import jakarta.validation.Validation; +import jakarta.validation.Validator; +import jakarta.validation.ValidatorFactory; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import java.util.Collections; import java.util.UUID; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; class PluginConfigurationConverterTest { private PluginSetting pluginSetting; + private Validator validator; static class TestConfiguration { + @SuppressWarnings("unused") private String myValue; public String getMyValue() { @@ -36,10 +48,19 @@ public String getMyValue() { @BeforeEach void setUp() { pluginSetting = mock(PluginSetting.class); + + validator = mock(Validator.class); } private PluginConfigurationConverter createObjectUnderTest() { - return new PluginConfigurationConverter(); + final ValidatorFactory validatorFactory = mock(ValidatorFactory.class); + given(validatorFactory.getValidator()).willReturn(validator); + + try(final MockedStatic validationMockedStatic = mockStatic(Validation.class)) { + validationMockedStatic.when(Validation::buildDefaultValidatorFactory) + .thenReturn(validatorFactory); + return new PluginConfigurationConverter(); + } } @Test @@ -64,6 +85,7 @@ void convert_with_PluginSetting_target_should_return_pluginSetting_object_direct sameInstance(pluginSetting)); then(pluginSetting).shouldHaveNoInteractions(); + then(validator).shouldHaveNoInteractions(); } @Test @@ -83,4 +105,54 @@ void convert_with_other_target_should_return_pluginSetting_object_directly() { assertThat(convertedTestConfiguration.getMyValue(), equalTo(value)); } + @Test + void convert_with_other_target_should_validate_configuration() { + + final String value = UUID.randomUUID().toString(); + given(pluginSetting.getSettings()) + .willReturn(Collections.singletonMap("my_value", value)); + + final Object convertedConfiguration = createObjectUnderTest().convert(TestConfiguration.class, pluginSetting); + + then(validator) + .should() + .validate(convertedConfiguration); + } + + @Test + void convert_with_other_target_should_throw_exception_when_there_are_constraint_violations() { + + final String value = UUID.randomUUID().toString(); + given(pluginSetting.getSettings()) + .willReturn(Collections.singletonMap("my_value", value)); + + final String pluginName = UUID.randomUUID().toString(); + given(pluginSetting.getName()) + .willReturn(pluginName); + + final String pipelineName = UUID.randomUUID().toString(); + given(pluginSetting.getPipelineName()) + .willReturn(pipelineName); + + @SuppressWarnings("unchecked") final ConstraintViolation constraintViolation = mock(ConstraintViolation.class); + final String errorMessage = UUID.randomUUID().toString(); + given(constraintViolation.getMessage()).willReturn(errorMessage); + final String propertyPathString = UUID.randomUUID().toString(); + final Path propertyPath = mock(Path.class); + given(propertyPath.toString()).willReturn(propertyPathString); + given(constraintViolation.getPropertyPath()).willReturn(propertyPath); + + given(validator.validate(any())) + .willReturn(Collections.singleton(constraintViolation)); + + final PluginConfigurationConverter objectUnderTest = createObjectUnderTest(); + + final InvalidPluginConfigurationException actualException = assertThrows(InvalidPluginConfigurationException.class, + () -> objectUnderTest.convert(TestConfiguration.class, pluginSetting)); + + assertThat(actualException.getMessage(), containsString(pluginName)); + assertThat(actualException.getMessage(), containsString(pipelineName)); + assertThat(actualException.getMessage(), containsString(propertyPathString)); + assertThat(actualException.getMessage(), containsString(errorMessage)); + } } \ No newline at end of file diff --git a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluggableInterface.java b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluggableInterface.java new file mode 100644 index 0000000000..19afc26219 --- /dev/null +++ b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluggableInterface.java @@ -0,0 +1,9 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.plugin; + +public interface TestPluggableInterface { +} diff --git a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluginConfiguration.java b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluginConfiguration.java new file mode 100644 index 0000000000..5f89325de4 --- /dev/null +++ b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugin/TestPluginConfiguration.java @@ -0,0 +1,31 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.plugin; + +import jakarta.validation.constraints.NotNull; + +public class TestPluginConfiguration { + @NotNull + private String requiredString; + + private String optionalString; + + public String getRequiredString() { + return requiredString; + } + + public void setRequiredString(final String requiredString) { + this.requiredString = requiredString; + } + + public String getOptionalString() { + return optionalString; + } + + public void setOptionalString(final String optionalString) { + this.optionalString = optionalString; + } +} diff --git a/data-prepper-core/src/test/java/com/amazon/dataprepper/plugins/TestPlugin.java b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugins/TestPlugin.java new file mode 100644 index 0000000000..dac53abdb0 --- /dev/null +++ b/data-prepper-core/src/test/java/com/amazon/dataprepper/plugins/TestPlugin.java @@ -0,0 +1,29 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.amazon.dataprepper.plugins; + +import com.amazon.dataprepper.model.annotations.DataPrepperPlugin; +import com.amazon.dataprepper.model.annotations.DataPrepperPluginConstructor; +import com.amazon.dataprepper.plugin.TestPluggableInterface; +import com.amazon.dataprepper.plugin.TestPluginConfiguration; + +/** + * Used for integration testing the plugin framework. + * TODO: Move this into the com.amazon.dataprepper.plugin package once alternate packages are supported per #379. + */ +@DataPrepperPlugin(name = "test_plugin", pluginType = TestPluggableInterface.class, pluginConfigurationType = TestPluginConfiguration.class) +public class TestPlugin implements TestPluggableInterface { + private final TestPluginConfiguration configuration; + + @DataPrepperPluginConstructor + public TestPlugin(final TestPluginConfiguration configuration) { + this.configuration = configuration; + } + + public TestPluginConfiguration getConfiguration() { + return configuration; + } +}