import org.junit.jupiter.api.Test;
import org.springframework.beans.BeansException;
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
import org.springframework.beans.factory.support.BeanDefinitionRegistry;
import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.core.env.Environment;
import org.springframework.core.env.Profiles;

public class NullBeanDefinitionRegistryPostProcessorTest {

    @Test
    public void test() {
        try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
            ctx.register(TestConfiguration.class);
            // ctx.getEnvironment().setActiveProfiles("enhancement");  // uncomment it to pass test
            ctx.refresh();
        }
    }

    static class TestConfiguration {
        @Bean
        protected static BeanDefinitionRegistryPostProcessor enhancementBeanDefinitionRegistryPostProcessor(
                Environment env) {
            if (env.acceptsProfiles(Profiles.of("enhancement"))) {
                return new BeanDefinitionRegistryPostProcessor() {
                    @Override
                    public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory)
                            throws BeansException {
                        // do some enhancement
                    }

                    @Override
                    public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry)
                            throws BeansException {
                        // do some enhancement
                    }
                };
            } else {
                return null;
            }
        }
    }
}

org.springframework.beans.factory.BeanNotOfRequiredTypeException: Bean named 'enhancementBeanDefinitionRegistryPostProcessor' is expected to be of type 'org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor' but was actually of type 'org.springframework.beans.factory.support.NullBean'
    at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:399)
    at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:207)
    at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:119)
    at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:707)
    at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:533)

Comment From: sbrannen

Why don't you annotate the corresponding @Bean method with @Profile("enhancement")?

As an alternative, have you considered using functional bean registration instead of an @Bean method?

Comment From: quaff

@sbrannen It's a showcase, actually I need read properties from Environment and do some tests, for example test http endpoint is available or remote tcp port is open, It's not easy be solved by @Conditional or @Profile.

Comment From: sbrannen

@sbrannen It's a showcase, actually I need read properties from Environment and do some tests, for example test http endpoint is available or remote tcp port is open, It's not easy be solved by @Conditional or @Profile.

OK, though my question still remains:

Have you considered using functional bean registration instead of an @Bean method?

Comment From: quaff

@sbrannen It's a showcase, actually I need read properties from Environment and do some tests, for example test http endpoint is available or remote tcp port is open, It's not easy be solved by @Conditional or @Profile.

OK, though my question still remains:

Have you considered using functional bean registration instead of an @Bean method?

I'm using traditional @ComponentScan and @Configuration, not considering functional bean registration. I think this is a valid feature that spring should implement if not difficult, I'm OK if spring team decline it, I will use an empty bean instead null.

Comment From: quaff

My real intention is decorating dataSource and transactionManager with tracing instrumentation by wrapping existed BeanDefinition, could you @sbrannen give me any advice? thanks!

        return new BeanDefinitionRegistryPostProcessor() {
            @Override
            public void postProcessBeanFactory(ConfigurableListableBeanFactory configurableListableBeanFactory)
                    throws BeansException {
            }

            @Override
            public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry beanDefinitionRegistry)
                    throws BeansException {
                if (enabled) { // calculated by env
                    try {
                        String beanName = "dataSource";
                        BeanDefinition oldBd = beanDefinitionRegistry.getBeanDefinition(beanName);
                        beanDefinitionRegistry.removeBeanDefinition(beanName);
                        RootBeanDefinition newBd = new RootBeanDefinition(TracingDataSource.class);
                        newBd.setTargetType(DataSource.class);
                        newBd.setAutowireMode(AbstractBeanDefinition.AUTOWIRE_NO);
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(0,
                                new ConstructorArgumentValues.ValueHolder(GlobalTracer.get()));
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(1, oldBd);
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(2,
                                new ConstructorArgumentValues.ValueHolder(null));
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(3,
                                new ConstructorArgumentValues.ValueHolder(true));
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(4,
                                new ConstructorArgumentValues.ValueHolder(Collections.emptySet()));
                        if (oldBd.isPrimary()) {
                            newBd.setPrimary(true);
                            oldBd.setPrimary(false);
                        }
                        beanDefinitionRegistry.registerBeanDefinition(beanName, newBd);
                        log.info("Wrapped DataSource with {}", newBd.getBeanClassName());
                    } catch (NoSuchBeanDefinitionException ignored) {
                    }
                    try {
                        String beanName = "transactionManager";
                        BeanDefinition oldBd = beanDefinitionRegistry.getBeanDefinition(beanName);
                        beanDefinitionRegistry.removeBeanDefinition(beanName);
                        RootBeanDefinition newBd = new RootBeanDefinition(TracingTransactionManager.class);
                        newBd.setTargetType(PlatformTransactionManager.class);
                        newBd.setAutowireMode(AbstractBeanDefinition.AUTOWIRE_NO);
                        newBd.getConstructorArgumentValues().addIndexedArgumentValue(0, oldBd);
                        if (oldBd.isPrimary()) {
                            newBd.setPrimary(true);
                            oldBd.setPrimary(false);
                        }
                        beanDefinitionRegistry.registerBeanDefinition(beanName, newBd);
                        log.info("Wrapped PlatformTransactionManager with {}", newBd.getBeanClassName());
                    } catch (NoSuchBeanDefinitionException ignored) {
                    }
                }
            }
        };

Comment From: ttddyy

Hi @quaff

Isn't it more natural to use BeanPostProcessor to decorate those beans?

For example, spring-boot-data-source-decorator wraps DataSource with BPP here.

BTW, if you want to instrument DataSource with tracer, you could use spring-boot-data-source-decorator to wrap DataSource with datasource-proxy or p6spy for Spring Boot application. Or you could use java-jdbc if it is opentracing.

Comment From: quaff

Hi @quaff

Isn't it more natural to use BeanPostProcessor to decorate those beans?

For example, spring-boot-data-source-decorator wraps DataSource with BPP here.

BTW, if you want to instrument DataSource with tracer, you could use spring-boot-data-source-decorator to wrap DataSource with datasource-proxy or p6spy for Spring Boot application. Or you could use java-jdbc if it is opentracing.

Thanks for your advice, I'm not using spring-boot, and I'm active java-jdbc contributor.

Comment From: quaff

@ttddyy I take a look at spring-boot-data-source-decorator, I think my solution is simpler and elegant, all I need is translate dataSource = new TracingDataSource(GlobalTracer.get(), dataSource) to spring bean definition.

Comment From: ttddyy

I think my solution is simpler and elegant

Haha, yeah, probably. It's subjective and also depends on the requirement of your application. For example, sleuth uses more BPP and open-telemetry maybe use byte code manipulation(I didn't check, simply my guess since I remember I saw ByteBuddy dependency there) for tracing instrumentation. I think one drawback with BFPP impl above is it will lose the original bean definition with original name. So, whether it is acceptable compromise or not is upto the requirement for your app.

Sorry, it has derailed the original issue. Just a bit of side chatting.

Comment From: quaff

I think my solution is simpler and elegant

Haha, yeah, probably. It's subjective and also depends on the requirement of your application. For example, sleuth uses more BPP and open-telemetry maybe use byte code manipulation(I didn't check, simply my guess since I remember I saw ByteBuddy dependency there) for tracing instrumentation. I think one drawback with BFPP impl above is it will lose the original bean definition with original name. So, whether it is acceptable compromise or not is upto the requirement for your app.

Sorry, it has derailed the original issue. Just a bit of side chatting.

You are welcome, could you expand the drawback with BFPP? my solution is move down the original root bean definition as inner bean definition of the new root bean definition, It will retain original bean definition and reuse original name.

Comment From: ttddyy

@quaff hm, by quick looking the code, what I can think of are: - Original definition is looked up via bean name dataSource, so it won't work when DataSource is registered under different name. - Cannot simply retrieve the raw DataSource. It always need to unwrap from TracingDataSource. - For DataSource, if connection pool is involved, which usually shaped as DataSource and delegate to actual DataSource, usually what you want to instrument is the outer most DataSource because you want to capture the application behavior not the connection pool behavior. (can be but secondary to the application behavior). I think swapping bean definitions would be difficult to handle such case. - It is using GlobalTracer.get() as static retrieval. but you want to allow configuring own tracer bean to use. So, dependency injection would be hard to work on at this low level.

Comment From: quaff

@quaff hm, by quick looking the code, what I can think of are:

  • Original definition is looked up via bean name dataSource, so it won't work when DataSource is registered under different name.

I should transform bean definition by type not name

  • Cannot simply retrieve the raw DataSource. It always need to unwrap from TracingDataSource.

I added getUnderlying() method recently, let unwrap delegating to raw DataSource, we could keep using unwrap with/without tracing instrumentation like this :

if (dataSource.isWrapperFor(HikariDataSource.class))
  dataSource.unwrap(HikariDataSource.class).setMetricRegistry(meterRegistry);
  • For DataSource, if connection pool is involved, which usually shaped as DataSource and delegate to actual DataSource, usually what you want to instrument is the outer most DataSource because you want to capture the application behavior not the connection pool behavior. (can be but secondary to the application behavior). I think swapping bean definitions would be difficult to handle such case.

It need to be handled carefully

  • It is using GlobalTracer.get() as static retrieval. but you want to allow configuring own tracer bean to use. So, dependency injection would be hard to work on at this low level.

In real life, we want collect all traces to centralized storage, It's OK to using global tracer, same to metrics.

Thanks for your thoughts.

Comment From: quaff

@ttddyy I take your advice to use BPP instead of BFPP, It seems much cleaner, thanks again.

public class TracingBeanPostProcessor implements BeanPostProcessor {

    @Override
    public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
        if (bean instanceof DataSource) {
            bean = new TracingDataSource(GlobalTracer.get(), (DataSource) bean, null, true, null);
        } else if (bean instanceof PlatformTransactionManager) {
            bean = new TracingTransactionManager((PlatformTransactionManager) bean);
        }
        return bean;
    }

}

Comment From: sbrannen

I'm using traditional @componentscan and @configuration, not considering functional bean registration.

OK. Thanks for the feedback.

I think this is a valid feature that spring should implement if not difficult, I'm OK if spring team decline it, I will use an empty bean instead null.

We have decided not to ignore null references for registered BFPPs and BFPs. Instead, we suggest you either use functional bean registration to programmatically determine if you want to register the non-null BFPP or BFP, or alternatively return a no-op (empty implementation) for the BFPP or BPP.

In light of that, I am closing this issue.