Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for plugin configuration classes #478

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
package com.amazon.dataprepper.model.annotations;

import com.amazon.dataprepper.model.PluginType;
import com.amazon.dataprepper.model.configuration.PluginSetting;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
Expand Down Expand Up @@ -66,4 +67,15 @@
* @since 1.2
*/
Class<?> pluginType() default Void.class;

/**
* The configuration type which the plugin takes in the constructor.
* <p>
* By default, this value is a {@link PluginSetting}, but you can provide
* a POJO object to facilitate cleaner code in your plugins.
*
* @return The Java class type for plugin configurations
* @since 1.2
*/
Class<?> pluginConfigurationType() default PluginSetting.class;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
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.NoPluginFoundException;
import com.amazon.dataprepper.model.plugin.PluginFactory;
Expand All @@ -20,9 +21,10 @@ public class DefaultPluginFactory implements PluginFactory {

private final Collection<PluginProvider> pluginProviders;
private final PluginCreator pluginCreator;
private final PluginConfigurationConverter pluginConfigurationConverter;

public DefaultPluginFactory() {
this(new PluginProviderLoader(), new PluginCreator());
this(new PluginProviderLoader(), new PluginCreator(), new PluginConfigurationConverter());
}

/**
Expand All @@ -31,9 +33,11 @@ public DefaultPluginFactory() {
*/
DefaultPluginFactory(
final PluginProviderLoader pluginProviderLoader,
final PluginCreator pluginCreator) {
final PluginCreator pluginCreator,
final PluginConfigurationConverter pluginConfigurationConverter) {
Objects.requireNonNull(pluginProviderLoader);
this.pluginCreator = Objects.requireNonNull(pluginCreator);
this.pluginConfigurationConverter = Objects.requireNonNull(pluginConfigurationConverter);

this.pluginProviders = Objects.requireNonNull(pluginProviderLoader.getPluginProviders());

Expand All @@ -48,7 +52,9 @@ public <T> T loadPlugin(final Class<T> baseClass, final PluginSetting pluginSett
final String pluginName = pluginSetting.getName();
final Class<? extends T> pluginClass = getPluginClass(baseClass, pluginName);

return pluginCreator.newPluginInstance(pluginClass, pluginSetting);
final Object configuration = getConfiguration(pluginSetting, pluginClass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really looking forward to getting rid of Object as a type that holds configuration...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in this case we need to stick with Object. The type is determined at runtime with this change. There might be an approach to making this work with generics, but I suspect it would be overly complicated without providing any value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sad 😢 , we can leave it as is for right now, but perhaps the work that Shivani is doing with the configuration models can help this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move away from Object by using inheritence and providing a base class with common configuration attirbutes like the plugin metrics. But I think you were thinking of keeping these objects separate have your considered combining?


return pluginCreator.newPluginInstance(pluginClass, configuration, pluginName);
}

@Override
Expand All @@ -64,13 +70,22 @@ public <T> List<T> loadPlugins(
if(numberOfInstances == null || numberOfInstances < 0)
throw new IllegalArgumentException("The numberOfInstances must be provided as a non-negative integer.");

final Object configuration = getConfiguration(pluginSetting, pluginClass);

final List<T> plugins = new ArrayList<>(numberOfInstances);
for (int i = 0; i < numberOfInstances; i++) {
plugins.add(pluginCreator.newPluginInstance(pluginClass, pluginSetting));
plugins.add(pluginCreator.newPluginInstance(pluginClass, configuration, pluginName));
}
return plugins;
}

private <T> Object getConfiguration(final PluginSetting pluginSetting, final Class<? extends T> pluginClass) {
final DataPrepperPlugin pluginAnnotation = pluginClass.getAnnotation(DataPrepperPlugin.class);

final Class<?> pluginConfigurationType = pluginAnnotation.pluginConfigurationType();
return pluginConfigurationConverter.convert(pluginConfigurationType, pluginSetting);
}

private <T> Class<? extends T> getPluginClass(final Class<T> baseClass, final String pluginName) {
return pluginProviders.stream()
.map(pluginProvider -> pluginProvider.<T>findPluginClass(baseClass, pluginName))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.amazon.dataprepper.plugin;

import com.amazon.dataprepper.model.configuration.PluginSetting;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;

import java.util.Objects;

class PluginConfigurationConverter {
private final ObjectMapper objectMapper;

PluginConfigurationConverter() {
this.objectMapper = new ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE);
}

public Object convert(final Class<?> pluginConfigurationType, final PluginSetting pluginSetting) {
Objects.requireNonNull(pluginConfigurationType);
Objects.requireNonNull(pluginSetting);

if(pluginConfigurationType.equals(PluginSetting.class))
return pluginSetting;

return objectMapper.convertValue(pluginSetting.getSettings(), pluginConfigurationType);
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit on my part, but I'd like to see this as an if / else block.

}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.amazon.dataprepper.plugin;

import com.amazon.dataprepper.model.configuration.PluginSetting;
import com.amazon.dataprepper.model.plugin.InvalidPluginDefinitionException;
import com.amazon.dataprepper.model.plugin.PluginInvocationException;
import org.slf4j.Logger;
Expand All @@ -15,14 +14,16 @@
class PluginCreator {
private static final Logger LOG = LoggerFactory.getLogger(PluginCreator.class);

<T> T newPluginInstance(final Class<T> pluginClass, final PluginSetting pluginSetting) {
<T> T newPluginInstance(final Class<T> pluginClass,
final Object pluginConfiguration,
final String pluginName) {
Objects.requireNonNull(pluginClass);

final String pluginName = pluginSetting.getName();
final Constructor<?> constructor = getConstructor(pluginClass, pluginName);
final Class<?> pluginConfigurationType = pluginConfiguration.getClass();
final Constructor<?> constructor = getConstructor(pluginClass, pluginConfigurationType, pluginName);

try {
return (T) constructor.newInstance(pluginSetting);
return (T) constructor.newInstance(pluginConfiguration);
} catch (final IllegalAccessException | InstantiationException ex) {
LOG.error("Encountered exception while instantiating the plugin {}", pluginClass.getSimpleName(), ex);
throw new InvalidPluginDefinitionException("Unable to access or instantiate the plugin '" + pluginClass.getSimpleName() + ".'", ex);
Expand All @@ -32,15 +33,15 @@ <T> T newPluginInstance(final Class<T> pluginClass, final PluginSetting pluginSe
}
}

private <T> Constructor<?> getConstructor(final Class<T> pluginClass, final String pluginName) {
private <T> Constructor<?> getConstructor(final Class<T> pluginClass, final Class<?> pluginConfigurationType, final String pluginName) {
try {
return pluginClass.getConstructor(PluginSetting.class);
return pluginClass.getConstructor(pluginConfigurationType);
} catch (final NoSuchMethodException ex) {
LOG.error("Data Prepper plugin requires a constructor with {} parameter;" +
" Plugin {} with name {} is missing such constructor.", PluginSetting.class.getSimpleName(),
" Plugin {} with name {} is missing such constructor.", pluginConfigurationType,
pluginClass.getSimpleName(), pluginName, ex);
throw new InvalidPluginDefinitionException(format("Data Prepper plugin requires a constructor with %s parameter;" +
" Plugin %s with name %s is missing such constructor.", PluginSetting.class.getSimpleName(),
" Plugin %s with name %s is missing such constructor.", pluginConfigurationType,
pluginClass.getSimpleName(), pluginName), ex);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class DefaultPluginFactoryTest {

private PluginProviderLoader pluginProviderLoader;
private PluginCreator pluginCreator;
private PluginConfigurationConverter pluginConfigurationConverter;
private Collection<PluginProvider> pluginProviders;
private PluginProvider firstPluginProvider;
private Class<?> baseClass;
Expand All @@ -39,6 +40,7 @@ class DefaultPluginFactoryTest {
void setUp() {
pluginProviderLoader = mock(PluginProviderLoader.class);
pluginCreator = mock(PluginCreator.class);
pluginConfigurationConverter = mock(PluginConfigurationConverter.class);

pluginProviders = new ArrayList<>();
given(pluginProviderLoader.getPluginProviders()).willReturn(pluginProviders);
Expand All @@ -52,7 +54,7 @@ void setUp() {
}

private DefaultPluginFactory createObjectUnderTest() {
return new DefaultPluginFactory(pluginProviderLoader, pluginCreator);
return new DefaultPluginFactory(pluginProviderLoader, pluginCreator, pluginConfigurationConverter);
}

@Test
Expand Down Expand Up @@ -136,7 +138,10 @@ void setUp() {
void loadPlugin_should_create_a_new_instance_of_the_first_plugin_found() {

final TestSink expectedInstance = mock(TestSink.class);
given(pluginCreator.newPluginInstance(expectedPluginClass, pluginSetting))
final Object convertedConfiguration = mock(Object.class);
given(pluginConfigurationConverter.convert(PluginSetting.class, pluginSetting))
.willReturn(convertedConfiguration);
given(pluginCreator.newPluginInstance(expectedPluginClass, convertedConfiguration, pluginName))
.willReturn(expectedInstance);

assertThat(createObjectUnderTest().loadPlugin(baseClass, pluginSetting),
Expand All @@ -155,7 +160,7 @@ void loadPlugins_should_throw_for_null_number_of_instances() {

@ParameterizedTest
@ValueSource(ints = {-100, -2, -1})
void loadPlugins_should_throw_for_invalid_number_of_instances(int numberOfInstances) {
void loadPlugins_should_throw_for_invalid_number_of_instances(final int numberOfInstances) {

final DefaultPluginFactory objectUnderTest = createObjectUnderTest();
assertThrows(IllegalArgumentException.class, () -> objectUnderTest.loadPlugins(
Expand All @@ -178,7 +183,10 @@ void loadPlugins_should_return_an_empty_list_when_the_number_of_instances_is_0()
@Test
void loadPlugins_should_return_a_single_instance_when_the_the_numberOfInstances_is_1() {
final TestSink expectedInstance = mock(TestSink.class);
given(pluginCreator.newPluginInstance(expectedPluginClass, pluginSetting))
final Object convertedConfiguration = mock(Object.class);
given(pluginConfigurationConverter.convert(PluginSetting.class, pluginSetting))
.willReturn(convertedConfiguration);
given(pluginCreator.newPluginInstance(expectedPluginClass, convertedConfiguration, pluginName))
.willReturn(expectedInstance);

final List<?> plugins = createObjectUnderTest().loadPlugins(
Expand All @@ -194,7 +202,11 @@ void loadPlugins_should_return_an_instance_for_the_total_count() {
final TestSink expectedInstance1 = mock(TestSink.class);
final TestSink expectedInstance2 = mock(TestSink.class);
final TestSink expectedInstance3 = mock(TestSink.class);
given(pluginCreator.newPluginInstance(expectedPluginClass, pluginSetting))
final Object convertedConfiguration = mock(Object.class);
given(pluginConfigurationConverter.convert(PluginSetting.class, pluginSetting))
.willReturn(convertedConfiguration);

given(pluginCreator.newPluginInstance(expectedPluginClass, convertedConfiguration, pluginName))
.willReturn(expectedInstance1)
.willReturn(expectedInstance2)
.willReturn(expectedInstance3);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package com.amazon.dataprepper.plugin;

import com.amazon.dataprepper.model.configuration.PluginSetting;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.Collections;
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.CoreMatchers.sameInstance;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.BDDMockito.given;
import static org.mockito.BDDMockito.then;
import static org.mockito.Mockito.mock;

class PluginConfigurationConverterTest {
private PluginSetting pluginSetting;

static class TestConfiguration {
private String myValue;

public String getMyValue() {
return myValue;
}
}

@BeforeEach
void setUp() {
pluginSetting = mock(PluginSetting.class);
}

private PluginConfigurationConverter createObjectUnderTest() {
return new PluginConfigurationConverter();
}

@Test
void convert_with_null_configurationType_should_throw() {
final PluginConfigurationConverter objectUnderTest = createObjectUnderTest();

assertThrows(NullPointerException.class,
() -> objectUnderTest.convert(null, pluginSetting));
}

@Test
void convert_with_null_pluginSetting_should_throw() {
final PluginConfigurationConverter objectUnderTest = createObjectUnderTest();

assertThrows(NullPointerException.class,
() -> objectUnderTest.convert(PluginSetting.class, null));
}

@Test
void convert_with_PluginSetting_target_should_return_pluginSetting_object_directly() {
assertThat(createObjectUnderTest().convert(PluginSetting.class, pluginSetting),
sameInstance(pluginSetting));

then(pluginSetting).shouldHaveNoInteractions();
}

@Test
void convert_with_other_target_should_return_pluginSetting_object_directly() {

final String value = UUID.randomUUID().toString();
given(pluginSetting.getSettings())
.willReturn(Collections.singletonMap("my_value", value));

final Object convertedConfiguration = createObjectUnderTest().convert(TestConfiguration.class, pluginSetting);

assertThat(convertedConfiguration, notNullValue());
assertThat(convertedConfiguration, instanceOf(TestConfiguration.class));

final TestConfiguration convertedTestConfiguration = (TestConfiguration) convertedConfiguration;

assertThat(convertedTestConfiguration.getMyValue(), equalTo(value));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
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.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
Expand All @@ -15,6 +17,7 @@
class PluginCreatorTest {

private PluginSetting pluginSetting;
private String pluginName;

public static class ValidPluginClass {
private final PluginSetting pluginSetting;
Expand All @@ -40,42 +43,44 @@ public AlwaysThrowingPluginClass(@SuppressWarnings("UnusedParameters") final Plu
@BeforeEach
void setUp() {
pluginSetting = mock(PluginSetting.class);

pluginName = UUID.randomUUID().toString();
}

private PluginCreator createObjectUnderTest() {
return new PluginCreator();
}

@Test
void newPluginInstance_should_create_new_instance_from_PluginSettings() {
void newPluginInstance_should_create_new_instance_from_pluginConfiguration() {

final ValidPluginClass instance = createObjectUnderTest().newPluginInstance(ValidPluginClass.class, pluginSetting);
final ValidPluginClass instance = createObjectUnderTest().newPluginInstance(ValidPluginClass.class, pluginSetting, pluginName);

assertThat(instance, notNullValue());
assertThat(instance.pluginSetting, equalTo(pluginSetting));
}

@Test
void newPluginInstance_should_throw_if_no_constructor_with_PluginSetting() {
void newPluginInstance_should_throw_if_no_constructor_with_pluginConfiguration() {

final PluginCreator objectUnderTest = createObjectUnderTest();
assertThrows(InvalidPluginDefinitionException.class,
() -> objectUnderTest.newPluginInstance(PluginClassWithoutConstructor.class, pluginSetting));
() -> objectUnderTest.newPluginInstance(PluginClassWithoutConstructor.class, pluginSetting, pluginName));
}

@Test
void newPluginInstance_should_throw_if_plugin_is_abstract() {

final PluginCreator objectUnderTest = createObjectUnderTest();
assertThrows(InvalidPluginDefinitionException.class,
() -> objectUnderTest.newPluginInstance(AbstractPluginClass.class, pluginSetting));
() -> objectUnderTest.newPluginInstance(AbstractPluginClass.class, pluginSetting, pluginName));
}

@Test
void newPluginInstance_should_throw_if_plugin_throws_in_constructor() {

final PluginCreator objectUnderTest = createObjectUnderTest();
assertThrows(PluginInvocationException.class,
() -> objectUnderTest.newPluginInstance(AlwaysThrowingPluginClass.class, pluginSetting));
() -> objectUnderTest.newPluginInstance(AlwaysThrowingPluginClass.class, pluginSetting, pluginName));
}
}
Loading