diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java index 2c5dbddaea..1565ce396e 100644 --- a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java @@ -44,7 +44,7 @@ public class DefaultPluginFactory implements PluginFactory { @Inject DefaultPluginFactory( final PluginProviderLoader pluginProviderLoader, - final PluginCreator pluginCreator, + @Named("pluginCreator") final PluginCreator pluginCreator, final PluginConfigurationConverter pluginConfigurationConverter, final PluginBeanFactoryProvider pluginBeanFactoryProvider, final PluginConfigurationObservableFactory pluginConfigurationObservableFactory, @@ -113,7 +113,7 @@ private ComponentPluginArgumentsContext getConstructionContext(final PluginS final Class pluginConfigurationType = pluginAnnotation.pluginConfigurationType(); final Object configuration = pluginConfigurationConverter.convert(pluginConfigurationType, pluginSetting); final PluginConfigObservable pluginConfigObservable = pluginConfigurationObservableFactory - .createDefaultPluginConfigObservable(pluginConfigurationConverter, pluginClass, pluginSetting); + .createDefaultPluginConfigObservable(pluginConfigurationConverter, pluginConfigurationType, pluginSetting); return new ComponentPluginArgumentsContext.Builder() .withPluginSetting(pluginSetting) diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/ExtensionLoader.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/ExtensionLoader.java index 5d5b675c13..16706448e9 100644 --- a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/ExtensionLoader.java +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/ExtensionLoader.java @@ -20,16 +20,16 @@ public class ExtensionLoader { private final ExtensionPluginConfigurationConverter extensionPluginConfigurationConverter; private final ExtensionClassProvider extensionClassProvider; - private final PluginCreator pluginCreator; + private final PluginCreator extensionPluginCreator; @Inject ExtensionLoader( final ExtensionPluginConfigurationConverter extensionPluginConfigurationConverter, final ExtensionClassProvider extensionClassProvider, - final PluginCreator pluginCreator) { + @Named("extensionPluginCreator") final PluginCreator extensionPluginCreator) { this.extensionPluginConfigurationConverter = extensionPluginConfigurationConverter; this.extensionClassProvider = extensionClassProvider; - this.pluginCreator = pluginCreator; + this.extensionPluginCreator = extensionPluginCreator; } List loadExtensions() { @@ -37,7 +37,8 @@ List loadExtensions() { .stream() .map(extensionClass -> { final PluginArgumentsContext pluginArgumentsContext = getConstructionContext(extensionClass); - return pluginCreator.newPluginInstance(extensionClass, pluginArgumentsContext, convertClassToName(extensionClass)); + return extensionPluginCreator.newPluginInstance( + extensionClass, pluginArgumentsContext, convertClassToName(extensionClass)); }) .collect(Collectors.toList()); } diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationObservableRegister.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationObservableRegister.java index 040bbde067..4293989453 100644 --- a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationObservableRegister.java +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginConfigurationObservableRegister.java @@ -7,6 +7,7 @@ import org.opensearch.dataprepper.model.plugin.PluginConfigObservable; import org.opensearch.dataprepper.model.plugin.PluginConfigPublisher; +import org.springframework.context.annotation.DependsOn; import javax.inject.Inject; import javax.inject.Named; @@ -15,6 +16,7 @@ import java.util.Set; @Named +@DependsOn("extensionsApplier") public class PluginConfigurationObservableRegister { private final Set pluginConfigPublishers; diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreator.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreator.java index cb19233e8a..857f519b31 100644 --- a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreator.java +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreator.java @@ -12,8 +12,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import javax.inject.Inject; -import javax.inject.Named; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.util.Arrays; @@ -22,13 +20,15 @@ import java.util.Optional; import java.util.stream.Collectors; -@Named class PluginCreator { private static final Logger LOG = LoggerFactory.getLogger(PluginCreator.class); private final PluginConfigurationObservableRegister pluginConfigurationObservableRegister; - @Inject + PluginCreator() { + this.pluginConfigurationObservableRegister = null; + } + PluginCreator(final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { this.pluginConfigurationObservableRegister = pluginConfigurationObservableRegister; } @@ -45,7 +45,9 @@ T newPluginInstance(final Class pluginClass, final Object[] constructorArguments = pluginArgumentsContext.createArguments(constructor.getParameterTypes(), args); - pluginConfigurationObservableRegister.registerPluginConfigurationObservables(constructorArguments); + if (pluginConfigurationObservableRegister != null) { + pluginConfigurationObservableRegister.registerPluginConfigurationObservables(constructorArguments); + } try { return (T) constructor.newInstance(constructorArguments); diff --git a/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreatorContext.java b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreatorContext.java new file mode 100644 index 0000000000..dc6f4ed743 --- /dev/null +++ b/data-prepper-core/src/main/java/org/opensearch/dataprepper/plugin/PluginCreatorContext.java @@ -0,0 +1,19 @@ +package org.opensearch.dataprepper.plugin; + +import org.springframework.context.annotation.Bean; + +import javax.inject.Named; + +@Named +public class PluginCreatorContext { + @Bean(name = "extensionPluginCreator") + public PluginCreator observablePluginCreator() { + return new PluginCreator(); + } + + @Bean(name = "pluginCreator") + public PluginCreator pluginCreator( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { + return new PluginCreator(pluginConfigurationObservableRegister); + } +} diff --git a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java index 5ba8f501c7..f66230a7b2 100644 --- a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java +++ b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java @@ -203,6 +203,8 @@ void loadPlugin_should_create_a_new_instance_of_the_first_plugin_found() { assertThat(createObjectUnderTest().loadPlugin(baseClass, pluginSetting), equalTo(expectedInstance)); + verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), + eq(PluginSetting.class), eq(pluginSetting)); verify(beanFactoryProvider).get(); } @@ -254,6 +256,8 @@ void loadPlugins_should_return_a_single_instance_when_the_the_numberOfInstances_ baseClass, pluginSetting, c -> 1); verify(beanFactoryProvider).get(); + verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), + eq(PluginSetting.class), eq(pluginSetting)); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); verify(pluginCreator).newPluginInstance(eq(expectedPluginClass), pluginArgumentsContextArgCapture.capture(), eq(pluginName)); final ComponentPluginArgumentsContext actualPluginArgumentsContext = pluginArgumentsContextArgCapture.getValue(); @@ -281,6 +285,8 @@ void loadPlugin_with_varargs_should_return_a_single_instance_when_the_the_number final Object plugin = createObjectUnderTest().loadPlugin(baseClass, pluginSetting, object); verify(beanFactoryProvider).get(); + verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), + eq(PluginSetting.class), eq(pluginSetting)); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); verify(pluginCreator).newPluginInstance(eq(expectedPluginClass), pluginArgumentsContextArgCapture.capture(), eq(pluginName), eq(object)); final ComponentPluginArgumentsContext actualPluginArgumentsContext = pluginArgumentsContextArgCapture.getValue(); diff --git a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/ExtensionLoaderTest.java b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/ExtensionLoaderTest.java index 76ca6900b8..176f3e0702 100644 --- a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/ExtensionLoaderTest.java +++ b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/ExtensionLoaderTest.java @@ -49,12 +49,12 @@ class ExtensionLoaderTest { @Mock private ExtensionClassProvider extensionClassProvider; @Mock - private PluginCreator pluginCreator; + private PluginCreator extensionPluginCreator; @Captor private ArgumentCaptor pluginArgumentsContextArgumentCaptor; private ExtensionLoader createObjectUnderTest() { - return new ExtensionLoader(extensionPluginConfigurationConverter, extensionClassProvider, pluginCreator); + return new ExtensionLoader(extensionPluginConfigurationConverter, extensionClassProvider, extensionPluginCreator); } @Test @@ -74,7 +74,7 @@ void loadExtensions_returns_single_extension_for_single_plugin_class() { when(extensionClassProvider.loadExtensionPluginClasses()).thenReturn(Collections.singleton(pluginClass)); final ExtensionPlugin expectedPlugin = mock(ExtensionPlugin.class); - when(pluginCreator.newPluginInstance( + when(extensionPluginCreator.newPluginInstance( eq(pluginClass), any(PluginArgumentsContext.class), startsWith("extension_plugin"))) @@ -98,7 +98,7 @@ void loadExtensions_returns_single_extension_with_config_for_single_plugin_class final String expectedPluginName = "test_extension_with_config"; when(extensionPluginConfigurationConverter.convert(eq(true), eq(TestExtensionConfig.class), eq("/test_extension"))).thenReturn(testExtensionConfig); - when(pluginCreator.newPluginInstance( + when(extensionPluginCreator.newPluginInstance( eq(TestExtensionWithConfig.class), any(PluginArgumentsContext.class), eq(expectedPluginName))) @@ -106,7 +106,7 @@ void loadExtensions_returns_single_extension_with_config_for_single_plugin_class final List extensionPlugins = createObjectUnderTest().loadExtensions(); - verify(pluginCreator).newPluginInstance(eq(TestExtensionWithConfig.class), + verify(extensionPluginCreator).newPluginInstance(eq(TestExtensionWithConfig.class), pluginArgumentsContextArgumentCaptor.capture(), eq(expectedPluginName)); assertThat(pluginArgumentsContextArgumentCaptor.getValue(), instanceOf( ExtensionLoader.SingleConfigArgumentArgumentsContext.class)); @@ -128,7 +128,7 @@ void loadExtensions_returns_multiple_extensions_for_multiple_plugin_classes() { final String expectedPluginName = ExtensionLoader.classNameToPluginName(pluginClass.getSimpleName()); final ExtensionPlugin extensionPlugin = mock((Class)pluginClass); - when(pluginCreator.newPluginInstance( + when(extensionPluginCreator.newPluginInstance( eq(pluginClass), any(PluginArgumentsContext.class), eq(expectedPluginName))) @@ -156,7 +156,7 @@ void loadExtensions_invokes_newPluginInstance_with_PluginArgumentsContext_not_su when(extensionClassProvider.loadExtensionPluginClasses()).thenReturn(Collections.singleton(pluginClass)); - when(pluginCreator.newPluginInstance( + when(extensionPluginCreator.newPluginInstance( any(Class.class), any(PluginArgumentsContext.class), anyString())) @@ -166,7 +166,7 @@ void loadExtensions_invokes_newPluginInstance_with_PluginArgumentsContext_not_su final ArgumentCaptor contextArgumentCaptor = ArgumentCaptor.forClass(PluginArgumentsContext.class); - verify(pluginCreator).newPluginInstance( + verify(extensionPluginCreator).newPluginInstance( eq(pluginClass), contextArgumentCaptor.capture(), anyString()); @@ -183,7 +183,7 @@ void loadExtensions_invokes_newPluginInstance_with_PluginArgumentsContext_which_ when(extensionClassProvider.loadExtensionPluginClasses()).thenReturn(Collections.singleton(pluginClass)); - when(pluginCreator.newPluginInstance( + when(extensionPluginCreator.newPluginInstance( any(Class.class), any(PluginArgumentsContext.class), anyString())) @@ -193,7 +193,7 @@ void loadExtensions_invokes_newPluginInstance_with_PluginArgumentsContext_which_ final ArgumentCaptor contextArgumentCaptor = ArgumentCaptor.forClass(PluginArgumentsContext.class); - verify(pluginCreator).newPluginInstance( + verify(extensionPluginCreator).newPluginInstance( eq(pluginClass), contextArgumentCaptor.capture(), any()); diff --git a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/PluginCreatorTest.java b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/PluginCreatorTest.java index 50617f50f1..0daa69a834 100644 --- a/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/PluginCreatorTest.java +++ b/data-prepper-core/src/test/java/org/opensearch/dataprepper/plugin/PluginCreatorTest.java @@ -5,6 +5,8 @@ package org.opensearch.dataprepper.plugin; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.opensearch.dataprepper.model.annotations.DataPrepperPluginConstructor; import org.opensearch.dataprepper.model.configuration.PluginSetting; import org.opensearch.dataprepper.model.plugin.InvalidPluginDefinitionException; @@ -13,9 +15,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import java.util.UUID; +import java.util.stream.Stream; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.notNullValue; @@ -122,18 +124,21 @@ void setUp() { pluginConfigurationObservableRegister = mock(PluginConfigurationObservableRegister.class); } - private PluginCreator createObjectUnderTest() { + private PluginCreator createObjectUnderTest( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { return new PluginCreator(pluginConfigurationObservableRegister); } - @Test - void newPluginInstance_should_create_new_instance_from_annotated_constructor() { + @ParameterizedTest + @MethodSource("providePluginConfigurationObservableRegister") + void newPluginInstance_should_create_new_instance_from_annotated_constructor( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { final AlternatePluginConfig alternatePluginConfig = mock(AlternatePluginConfig.class); given(pluginConstructionContext.createArguments(new Class[] {PluginSetting.class, AlternatePluginConfig.class})) .willReturn(new Object[] { pluginSetting, alternatePluginConfig }); - final PluginClassWithMultipleConstructors instance = createObjectUnderTest() + final PluginClassWithMultipleConstructors instance = createObjectUnderTest(pluginConfigurationObservableRegister) .newPluginInstance(PluginClassWithMultipleConstructors.class, pluginConstructionContext, pluginName); assertThat(instance, notNullValue()); @@ -141,15 +146,17 @@ void newPluginInstance_should_create_new_instance_from_annotated_constructor() { assertThat(instance.alternatePluginConfig, equalTo(alternatePluginConfig)); } - @Test - void newPluginInstance_should_create_new_instance_from_annotated_constructor_with_byte_decoder() { + @ParameterizedTest + @MethodSource("providePluginConfigurationObservableRegister") + void newPluginInstance_should_create_new_instance_from_annotated_constructor_with_byte_decoder( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { Object obj = new Object(); final AlternatePluginConfig alternatePluginConfig = mock(AlternatePluginConfig.class); given(pluginConstructionContext.createArguments(new Class[] {PluginSetting.class, AlternatePluginConfig.class, Object.class}, obj)) .willReturn(new Object[] { pluginSetting, alternatePluginConfig, obj}); - final PluginClassWithThreeArgs instance = createObjectUnderTest() + final PluginClassWithThreeArgs instance = createObjectUnderTest(pluginConfigurationObservableRegister) .newPluginInstance(PluginClassWithThreeArgs.class, pluginConstructionContext, pluginName, obj); assertThat(instance, notNullValue()); @@ -175,47 +182,71 @@ void newPluginInstance_should_register_pluginConfigurationObservable() { assertThat(instance.pluginConfigObservable, equalTo(pluginConfigObservable)); } - @Test - void newPluginInstance_should_create_new_instance_from_PluginSetting_if_the_constructor() { + @ParameterizedTest + @MethodSource("providePluginConfigurationObservableRegister") + void newPluginInstance_should_create_new_instance_from_PluginSetting_if_the_constructor( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { given(pluginConstructionContext.createArguments(new Class[] {PluginSetting.class})) .willReturn(new Object[] { pluginSetting }); - final ValidPluginClass instance = createObjectUnderTest().newPluginInstance(ValidPluginClass.class, pluginConstructionContext, pluginName); + final ValidPluginClass instance = createObjectUnderTest(pluginConfigurationObservableRegister) + .newPluginInstance(ValidPluginClass.class, pluginConstructionContext, pluginName); assertThat(instance, notNullValue()); assertThat(instance.pluginSetting, equalTo(pluginSetting)); } - @Test - void newPluginInstance_should_create_new_instance_using_default_constructor_if_available() { + @ParameterizedTest + @MethodSource("providePluginConfigurationObservableRegister") + void newPluginInstance_should_create_new_instance_using_default_constructor_if_available( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { given(pluginConstructionContext.createArguments(new Class[] {PluginSetting.class})) .willReturn(new Object[] { pluginSetting }); - final PluginClassWithoutConstructor instance = createObjectUnderTest().newPluginInstance(PluginClassWithoutConstructor.class, pluginConstructionContext, pluginName); + final PluginClassWithoutConstructor instance = createObjectUnderTest(pluginConfigurationObservableRegister) + .newPluginInstance(PluginClassWithoutConstructor.class, pluginConstructionContext, pluginName); assertThat(instance, notNullValue()); } @ParameterizedTest - @ValueSource(classes = { - InvalidPluginClassDueToUsableConstructor.class, - InvalidPluginClassDueToMultipleAnnotatedConstructors.class, - InvalidAbstractPluginClass.class - }) - void newPluginInstance_should_throw_for_pluginClass_with_invalid_definition(final Class invalidPluginClass) { - - final PluginCreator objectUnderTest = createObjectUnderTest(); + @MethodSource("providePluginConfigurationObservableRegisterAndInvalidPluginClasses") + void newPluginInstance_should_throw_for_pluginClass_with_invalid_definition( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister, + final Class invalidPluginClass) { + + final PluginCreator objectUnderTest = createObjectUnderTest(pluginConfigurationObservableRegister); assertThrows(InvalidPluginDefinitionException.class, () -> objectUnderTest.newPluginInstance(invalidPluginClass, pluginConstructionContext, pluginName)); } - @Test - void newPluginInstance_should_throw_if_plugin_throws_in_constructor() { + @ParameterizedTest + @MethodSource("providePluginConfigurationObservableRegister") + void newPluginInstance_should_throw_if_plugin_throws_in_constructor( + final PluginConfigurationObservableRegister pluginConfigurationObservableRegister) { given(pluginConstructionContext.createArguments(new Class[] {PluginSetting.class})) .willReturn(new Object[] { pluginSetting }); - final PluginCreator objectUnderTest = createObjectUnderTest(); + final PluginCreator objectUnderTest = createObjectUnderTest(pluginConfigurationObservableRegister); assertThrows(PluginInvocationException.class, () -> objectUnderTest.newPluginInstance(AlwaysThrowingPluginClass.class, pluginConstructionContext, pluginName)); } + + private static Stream providePluginConfigurationObservableRegister() { + return Stream.of( + null, + Arguments.of(mock(PluginConfigurationObservableRegister.class)) + ); + } + + private static Stream providePluginConfigurationObservableRegisterAndInvalidPluginClasses() { + return Stream.of( + new Object[]{null, InvalidPluginClassDueToUsableConstructor.class}, + new Object[]{null, InvalidPluginClassDueToMultipleAnnotatedConstructors.class}, + new Object[]{null, InvalidAbstractPluginClass.class}, + new Object[]{mock(PluginConfigurationObservableRegister.class), InvalidPluginClassDueToUsableConstructor.class}, + new Object[]{mock(PluginConfigurationObservableRegister.class), InvalidPluginClassDueToMultipleAnnotatedConstructors.class}, + new Object[]{mock(PluginConfigurationObservableRegister.class), InvalidAbstractPluginClass.class} + ); + } }