Make targetBeanName field in AbstractBeanFactoryBasedTargetSource protected to avoid exceptions in logging and toString()

[chenggwang]

Fix #35167 AbstractBeanFactoryBasedTargetSourceCreator getTargetSource method returns targetSource instance has yet to be completed TargetBeanName Settings, At this point, IDEA or an external tracking tool will call the toString() of targetSource, and toString() will call the getTargetBeanName() method. At this time, the assertion will throw an error.

Analysis

The call chain is as follows：

• Container by createBeanFactoryBasedTargetSource method to obtain a targetSource instance，However, this instance has not completed the configuration Settings, such as setTargetBeanName and setBeanFactory.

• At this point, when IDEA calls targetSource to print the basic information of the object, it will call getTargetBeanName. At this time, targetBeanName==null. But there is an assertion here： Assert.state(this.targetBeanName != null, "Target bean name not set"), An error will be thrown at this point

The Java® Language Specification:The method toString returns a String representation of the object. Therefore, after an object is instantiated, we should be able to call its toString() smoothly. So, according to the definition of the java specification, when we obtain the current property through the toString() method for display, it should be a simple get method directly obtaining the property, rather than undertaking logical judgment. This violates the single responsibility of toString()

Similarly, we also have multiple loglogger.debug ("No target for prototype '" + getTargetBeanName()).Log recording itself should be a safe and side-effect-free operation. It must not disrupt the normal program flow. If log statements may throw exceptions, they may mask original errors, or even cause cascading failures
The following is the key code segment where the problem occurred:

//   /aop/framework/autoproxy/targetAbstractBeanFactoryBasedTargetSourceCreator.java
@Override
	public final @Nullable TargetSource getTargetSource(Class<?> beanClass, String beanName) {
		AbstractBeanFactoryBasedTargetSource targetSource =
				createBeanFactoryBasedTargetSource(beanClass, beanName);
		if (targetSource == null) {
			return null;
		}

		if (logger.isDebugEnabled()) {
			logger.debug("Configuring AbstractBeanFactoryBasedTargetSource: " + targetSource);
		}

		DefaultListableBeanFactory internalBeanFactory = getInternalBeanFactoryForBean(beanName);

		// We need to override just this bean definition, as it may reference other beans
		// and we're happy to take the parent's definition for those.
		// Always use prototype scope if demanded.
		BeanDefinition bd = getConfigurableBeanFactory().getMergedBeanDefinition(beanName);
		GenericBeanDefinition bdCopy = new GenericBeanDefinition(bd);
		if (isPrototypeBased()) {
			bdCopy.setScope(BeanDefinition.SCOPE_PROTOTYPE);
		}
		internalBeanFactory.registerBeanDefinition(beanName, bdCopy);

		// Complete configuring the PrototypeTargetSource.
		targetSource.setTargetBeanName(beanName);
		targetSource.setBeanFactory(internalBeanFactory);

		return targetSource;
	}

//  /aop/target /PrototypeTargetSource.java
@Override
	public String toString() {
		return "PrototypeTargetSource for target bean with name '" + getTargetBeanName() + "'";
	}

//   org/springframework/aop/target /AbstractBeanFactoryBasedTargetSource.java
public String getTargetBeanName() {
		Assert.state(this.targetBeanName != null, "Target bean name not set");
		return this.targetBeanName;
	}

//  org/springframework/aop/target /AbstractPrototypeBasedTargetSource.java
protected Object newPrototypeInstance() throws BeansException {
		if (logger.isDebugEnabled()) {
			logger.debug("Creating new instance of bean '" + getTargetBeanName() + "'");
		}
		return getBeanFactory().getBean(getTargetBeanName());
	}

Solution

So in class AbstractBeanFactoryBasedTargetSourceCreator increase method named getPlainTargetBeanName (),

public String getPlainTargetBeanName() {
		return this.targetBeanName;
	}

This simple method is used to provide plain targetBeanName.toString() and both logger calls. When calling methods that require strict non-null judgment, use the original getTargetBeanName()

[sbrannen]

Hi @chenggwang,

We've decided just to make org.springframework.aop.target.AbstractBeanFactoryBasedTargetSource.targetBeanName a protected field instead of introducing a new getPlainTargetBeanName() method.

Can you please update this PR to do that and then simply reference this.targetBeanName instead of getPlainTargetBeanName()?

Thanks

p.s. I've changed the title of this PR to reflect the new focus.

[chenggwang]

Hi @chenggwang,

We've decided just to make org.springframework.aop.target.AbstractBeanFactoryBasedTargetSource.targetBeanName a protected field instead of introducing a new getPlainTargetBeanName() method.

Can you please update this PR to do that and then simply reference this.targetBeanName instead of getPlainTargetBeanName()?

Thanks

p.s. I've changed the title of this PR to reflect the new focus.

Hi，@sbrannen
I have revised it according to your suggestion

//org.springframework.aop.target.AbstractBeanFactoryBasedTargetSource.java
-- private @Nullable String targetBeanName;
++ protected @Nullable String targetBeanName; 

There are approximately seven places：logging and toString() use this.targetBeanName. I once again confirmed that these areas are AbstractBeanFactoryBasedTargetSource subclasses, and therefore protected modify targetBeanName, then this. targetBeanName acquisition is a very good solution. It's more perfect than adding one more method.Thank you for your suggestion.

[sbrannen]

This has been merged into 6.2.x and main.

Thanks

[sbrannen]

Please note that this does not compile with this..

In the future, please run a full local build before pushing changes to a PR.

For this particular case, I fixed the issue while merging this PR onto 6.2.x, so this has been taken care of.

[chenggwang]

I'm sorry I thought can be completed with simple text editor, I know the inner class general use ThreadLocalTargetSource.this.targetBeanName , The grammar plugin reminds me that perhaps what I want to use is: this. I thought this was a new grammar，I followed the advice of the plugin!!! This is a stupid mistake😭. In the future,i will run a full local build! Sorry to have caused you trouble! @sbrannen