From 3aecf2f9deddba61a5b2d72ee9218895197286c0 Mon Sep 17 00:00:00 2001 From: Santhosh Gandhe <1909520+san81@users.noreply.github.com> Date: Mon, 21 Oct 2024 09:29:04 -0700 Subject: [PATCH] Make plugin config injectable (#5078) * making plugin config injectable Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> * End to End test case for plugin config injectable Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> * two additional test cases Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> * reorganized test classes to separate out different test plugins Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> * addressing review comments Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> * better assertions when no injected bean expected in the bean factory Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> --------- Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> --- .../plugin/DefaultPluginFactoryIT.java | 28 +++++++++ .../plugin/DefaultPluginFactory.java | 2 +- .../plugin/PluginBeanFactoryProvider.java | 10 +++- .../plugin/DefaultPluginFactoryTest.java | 18 +++--- .../plugin/PluginBeanFactoryProviderTest.java | 60 +++++++++++++++++-- .../TestComponentWithConfigInject.java | 22 +++++++ .../configtest/TestDISourceWithConfig.java | 42 +++++++++++++ 7 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestComponentWithConfigInject.java create mode 100644 data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestDISourceWithConfig.java diff --git a/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryIT.java b/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryIT.java index 04944a8e6c..27dfa97cef 100644 --- a/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryIT.java +++ b/data-prepper-core/src/integrationTest/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryIT.java @@ -14,6 +14,8 @@ import org.opensearch.dataprepper.core.event.EventFactoryApplicationContextMarker; import org.opensearch.dataprepper.core.validation.LoggingPluginErrorsHandler; import org.opensearch.dataprepper.core.validation.PluginErrorCollector; +import org.opensearch.dataprepper.plugins.configtest.TestComponentWithConfigInject; +import org.opensearch.dataprepper.plugins.configtest.TestDISourceWithConfig; import org.opensearch.dataprepper.validation.PluginErrorsHandler; import org.opensearch.dataprepper.model.configuration.PipelinesDataFlowModel; import org.opensearch.dataprepper.model.configuration.PluginSetting; @@ -118,6 +120,32 @@ void loadPlugin_should_return_a_new_plugin_instance_with_DI_context_initialized( assertThat(plugin.getTestComponent().getIdentifier(), equalTo("test-component")); } + @Test + void loadPlugin_should_return_a_new_plugin_instance_with_DI_context_and_config_injected() { + + 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 = new PluginSetting("test_di_source_with_config", pluginSettingMap); + pluginSetting.setPipelineName("test_di_source_with_config"); + + final Source sourcePlugin = createObjectUnderTest().loadPlugin(Source.class, pluginSetting); + + assertThat(sourcePlugin, instanceOf(TestDISourceWithConfig.class)); + TestDISourceWithConfig plugin = (TestDISourceWithConfig) sourcePlugin; + // Testing the auto wired been with the Dependency Injection + assertNotNull(plugin.getTestComponent()); + assertInstanceOf(TestComponentWithConfigInject.class, plugin.getTestComponent()); + TestPluginConfiguration pluginConfig = plugin.getTestComponent().getConfiguration(); + assertInstanceOf(TestPluginConfiguration.class, pluginConfig); + assertThat(pluginConfig.getRequiredString(), equalTo(requiredStringValue)); + assertThat(pluginConfig.getOptionalString(), equalTo(optionalStringValue)); + assertThat(plugin.getTestComponent().getIdentifier(), equalTo("test-component-with-plugin-config-injected")); + } + @Test void loadPlugin_should_return_a_new_plugin_instance_with_the_expected_configuration_variable_args() { diff --git a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java index a866016e27..81fd1c2b5b 100644 --- a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java +++ b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/DefaultPluginFactory.java @@ -117,7 +117,7 @@ private ComponentPluginArgumentsContext getConstructionContext(final PluginS .createDefaultPluginConfigObservable(pluginConfigurationConverter, pluginConfigurationType, pluginSetting); Class[] markersToScan = pluginAnnotation.packagesToScan(); - BeanFactory beanFactory = pluginBeanFactoryProvider.createPluginSpecificContext(markersToScan); + BeanFactory beanFactory = pluginBeanFactoryProvider.createPluginSpecificContext(markersToScan, configuration); return new ComponentPluginArgumentsContext.Builder() .withPluginSetting(pluginSetting) diff --git a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProvider.java b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProvider.java index 6a16917f9d..76762f8d8e 100644 --- a/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProvider.java +++ b/data-prepper-plugin-framework/src/main/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProvider.java @@ -5,7 +5,9 @@ package org.opensearch.dataprepper.plugin; +import org.opensearch.dataprepper.model.configuration.PluginSetting; import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.support.GenericApplicationContext; @@ -58,9 +60,13 @@ GenericApplicationContext getCoreApplicationContext() { * instead, a new isolated {@link ApplicationContext} should be created. * @return BeanFactory A BeanFactory that inherits from {@link PluginBeanFactoryProvider#sharedPluginApplicationContext} */ - public BeanFactory createPluginSpecificContext(Class[] markersToScan) { + public BeanFactory createPluginSpecificContext(Class[] markersToScan, Object configuration) { AnnotationConfigApplicationContext isolatedPluginApplicationContext = new AnnotationConfigApplicationContext(); + DefaultListableBeanFactory beanFactory = (DefaultListableBeanFactory) isolatedPluginApplicationContext.getBeanFactory(); if(markersToScan !=null && markersToScan.length>0) { + if(configuration !=null && !(configuration instanceof PluginSetting)) { + beanFactory.registerSingleton(configuration.getClass().getName(), configuration); + } // If packages to scan is provided in this plugin annotation, which indicates // that this plugin is interested in using Dependency Injection isolated for its module Arrays.stream(markersToScan) @@ -69,6 +75,6 @@ public BeanFactory createPluginSpecificContext(Class[] markersToScan) { isolatedPluginApplicationContext.refresh(); } isolatedPluginApplicationContext.setParent(sharedPluginApplicationContext); - return isolatedPluginApplicationContext.getBeanFactory(); + return beanFactory; } } diff --git a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java index 8c282152d9..6f54a55c95 100644 --- a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java +++ b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/DefaultPluginFactoryTest.java @@ -210,7 +210,7 @@ void loadPlugin_should_create_a_new_instance_of_the_plugin_with_di_initialized() equalTo(expectedInstance)); verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), eq(PluginSetting.class), eq(pluginSetting)); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{TestDISource.class}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{TestDISource.class}, convertedConfiguration); } @Test @@ -227,7 +227,7 @@ void loadPlugin_should_create_a_new_instance_of_the_first_plugin_found() { equalTo(expectedInstance)); verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), eq(PluginSetting.class), eq(pluginSetting)); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); } @Test @@ -261,7 +261,7 @@ void loadPlugins_should_return_an_empty_list_when_the_number_of_instances_is_0() assertThat(plugins, notNullValue()); assertThat(plugins.size(), equalTo(0)); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, null); verifyNoInteractions(pluginCreator); } @@ -277,7 +277,7 @@ void loadPlugins_should_return_a_single_instance_when_the_the_numberOfInstances_ final List plugins = createObjectUnderTest().loadPlugins( baseClass, pluginSetting, c -> 1); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), eq(PluginSetting.class), eq(pluginSetting)); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); @@ -306,7 +306,7 @@ void loadPlugin_with_varargs_should_return_a_single_instance_when_the_the_number final Object plugin = createObjectUnderTest().loadPlugin(baseClass, pluginSetting, object); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); verify(pluginConfigurationObservableFactory).createDefaultPluginConfigObservable(eq(pluginConfigurationConverter), eq(PluginSetting.class), eq(pluginSetting)); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); @@ -341,7 +341,7 @@ void loadPlugins_should_return_an_instance_for_the_total_count() { final List plugins = createObjectUnderTest().loadPlugins( baseClass, pluginSetting, c -> 3); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); verify(pluginCreator, times(3)).newPluginInstance(eq(expectedPluginClass), pluginArgumentsContextArgCapture.capture(), eq(pluginName)); final List actualPluginArgumentsContextList = pluginArgumentsContextArgCapture.getAllValues(); @@ -377,7 +377,7 @@ void loadPlugins_should_return_a_single_instance_with_values_from_ApplicationCon final List plugins = createObjectUnderTest().loadPlugins( baseClass, pluginSetting, c -> 1); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); final ArgumentCaptor pluginArgumentsContextArgCapture = ArgumentCaptor.forClass(ComponentPluginArgumentsContext.class); verify(pluginCreator).newPluginInstance(eq(expectedPluginClass), pluginArgumentsContextArgCapture.capture(), eq(pluginName)); final ComponentPluginArgumentsContext actualPluginArgumentsContext = pluginArgumentsContextArgCapture.getValue(); @@ -419,7 +419,7 @@ void loadPlugin_should_create_a_new_instance_of_the_first_plugin_found_with_corr assertThat(createObjectUnderTest().loadPlugin(baseClass, pluginSetting), equalTo(expectedInstance)); MatcherAssert.assertThat(expectedInstance.getClass().getAnnotation(DataPrepperPlugin.class).deprecatedName(), equalTo(TEST_SINK_DEPRECATED_NAME)); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); } } @@ -448,7 +448,7 @@ void loadPlugin_should_create_a_new_instance_of_the_first_plugin_found_with_corr assertThat(createObjectUnderTest().loadPlugin(baseClass, pluginSetting), equalTo(expectedInstance)); MatcherAssert.assertThat(expectedInstance.getClass().getAnnotation(DataPrepperPlugin.class).alternateNames(), equalTo(new String[]{TEST_SINK_ALTERNATE_NAME})); - verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}); + verify(beanFactoryProvider).createPluginSpecificContext(new Class[]{}, convertedConfiguration); } } } diff --git a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProviderTest.java b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProviderTest.java index 4545f21310..a80e42c011 100644 --- a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProviderTest.java +++ b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugin/PluginBeanFactoryProviderTest.java @@ -7,11 +7,21 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.opensearch.dataprepper.model.configuration.PluginSetting; import org.opensearch.dataprepper.plugins.test.TestComponent; import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.ListableBeanFactory; import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.support.GenericApplicationContext; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.function.Predicate; +import java.util.stream.Collectors; + import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; @@ -68,7 +78,7 @@ void testPluginBeanFactoryProviderGetReturnsBeanFactory() { final PluginBeanFactoryProvider beanFactoryProvider = createObjectUnderTest(); verify(context).getParent(); - assertThat(beanFactoryProvider.createPluginSpecificContext(new Class[]{}), is(instanceOf(BeanFactory.class))); + assertThat(beanFactoryProvider.createPluginSpecificContext(new Class[]{}, null), is(instanceOf(BeanFactory.class))); } @Test @@ -76,8 +86,8 @@ void testPluginBeanFactoryProviderGetReturnsUniqueBeanFactory() { doReturn(context).when(context).getParent(); final PluginBeanFactoryProvider beanFactoryProvider = createObjectUnderTest(); - final BeanFactory isolatedBeanFactoryA = beanFactoryProvider.createPluginSpecificContext(new Class[]{}); - final BeanFactory isolatedBeanFactoryB = beanFactoryProvider.createPluginSpecificContext(new Class[]{}); + final BeanFactory isolatedBeanFactoryA = beanFactoryProvider.createPluginSpecificContext(new Class[]{}, null); + final BeanFactory isolatedBeanFactoryB = beanFactoryProvider.createPluginSpecificContext(new Class[]{}, null); verify(context).getParent(); assertThat(isolatedBeanFactoryA, not(sameInstance(isolatedBeanFactoryB))); @@ -103,7 +113,7 @@ void getSharedPluginApplicationContext_called_multiple_times_returns_same_instan void testCreatePluginSpecificContext() { when(context.getParent()).thenReturn(context); final PluginBeanFactoryProvider objectUnderTest = createObjectUnderTest(); - BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{TestComponent.class}); + BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{TestComponent.class}, null); assertThat(beanFactory, notNullValue()); assertThat(beanFactory.getBean(TestComponent.class), notNullValue()); } @@ -112,8 +122,48 @@ void testCreatePluginSpecificContext() { void testCreatePluginSpecificContext_with_empty_array() { when(context.getParent()).thenReturn(context); final PluginBeanFactoryProvider objectUnderTest = createObjectUnderTest(); - BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{}); + BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{}, null); + assertThat(beanFactory, notNullValue()); + assertThat(beanFactory, instanceOf(ListableBeanFactory.class)); + ListableBeanFactory listableBeanFactory = (ListableBeanFactory) beanFactory; + List nonSpringBeans = Arrays.stream(listableBeanFactory.getBeanDefinitionNames()) + .filter(Predicate.not(name -> name.startsWith("org.springframework"))) + .collect(Collectors.toList()); + assertThat(nonSpringBeans, equalTo(Collections.emptyList())); + } + + @Test + void testCreatePluginSpecificContext_with_pipeline_settings() { + when(context.getParent()).thenReturn(context); + final PluginBeanFactoryProvider objectUnderTest = createObjectUnderTest(); + PluginSetting pipelineSettings = new PluginSetting(UUID.randomUUID().toString(), Map.of("key", "val")); + BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{}, pipelineSettings); + assertThat(beanFactory, notNullValue()); + assertThrows(NoSuchBeanDefinitionException.class, ()->beanFactory.getBean(PluginSetting.class)); + } + + @Test + void testCreatePluginSpecificContext_with_empty_array_with_plugin_config() { + when(context.getParent()).thenReturn(context); + final PluginBeanFactoryProvider objectUnderTest = createObjectUnderTest(); + TestPluginConfiguration config = new TestPluginConfiguration(); + BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{}, config); assertThat(beanFactory, notNullValue()); assertThrows(NoSuchBeanDefinitionException.class, ()->beanFactory.getBean(TestComponent.class)); + assertThrows(NoSuchBeanDefinitionException.class, ()->beanFactory.getBean(TestPluginConfiguration.class)); + } + + @Test + void testCreatePluginSpecificContext_with_plugin_config() { + when(context.getParent()).thenReturn(context); + final PluginBeanFactoryProvider objectUnderTest = createObjectUnderTest(); + TestPluginConfiguration config = new TestPluginConfiguration(); + String requiredStringValue = UUID.randomUUID().toString(); + config.setRequiredString(requiredStringValue); + BeanFactory beanFactory = objectUnderTest.createPluginSpecificContext(new Class[]{TestComponent.class}, config); + assertThat(beanFactory, notNullValue()); + assertThat(beanFactory.getBean(TestComponent.class), notNullValue()); + assertThat(beanFactory.getBean(TestPluginConfiguration.class), notNullValue()); + assertThat(beanFactory.getBean(TestPluginConfiguration.class).getRequiredString(), equalTo(requiredStringValue)); } } \ No newline at end of file diff --git a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestComponentWithConfigInject.java b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestComponentWithConfigInject.java new file mode 100644 index 0000000000..141f4f5264 --- /dev/null +++ b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestComponentWithConfigInject.java @@ -0,0 +1,22 @@ +package org.opensearch.dataprepper.plugins.configtest; + +import org.opensearch.dataprepper.plugin.TestPluginConfiguration; + +import javax.inject.Named; + +@Named +public class TestComponentWithConfigInject { + private final TestPluginConfiguration configuration; + + public TestComponentWithConfigInject(TestPluginConfiguration configuration) { + this.configuration = configuration; + } + + public String getIdentifier() { + return "test-component-with-plugin-config-injected"; + } + + public TestPluginConfiguration getConfiguration() { + return configuration; + } +} diff --git a/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestDISourceWithConfig.java b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestDISourceWithConfig.java new file mode 100644 index 0000000000..ac1947ba73 --- /dev/null +++ b/data-prepper-plugin-framework/src/test/java/org/opensearch/dataprepper/plugins/configtest/TestDISourceWithConfig.java @@ -0,0 +1,42 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.dataprepper.plugins.configtest; + +import org.opensearch.dataprepper.model.annotations.DataPrepperPlugin; +import org.opensearch.dataprepper.model.annotations.DataPrepperPluginConstructor; +import org.opensearch.dataprepper.model.buffer.Buffer; +import org.opensearch.dataprepper.model.record.Record; +import org.opensearch.dataprepper.model.source.Source; +import org.opensearch.dataprepper.plugin.TestPluggableInterface; +import org.opensearch.dataprepper.plugin.TestPluginConfiguration; + +@DataPrepperPlugin(name = "test_di_source_with_config", + alternateNames = { "test_source_alternate_name1", "test_source_alternate_name2" }, + deprecatedName = "test_source_deprecated_name", + pluginType = Source.class, + packagesToScan = {TestDISourceWithConfig.class}, + pluginConfigurationType = TestPluginConfiguration.class +) +public class TestDISourceWithConfig implements Source>, TestPluggableInterface { + + private final TestComponentWithConfigInject testComponent; + + @DataPrepperPluginConstructor + public TestDISourceWithConfig(final TestComponentWithConfigInject testComponent) { + this.testComponent = testComponent; + } + + @Override + public void start(Buffer> buffer) { + } + + public TestComponentWithConfigInject getTestComponent() { + return testComponent; + } + + @Override + public void stop() {} +}