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

The getter and setter that's used during configuration property binding varies when a getter or setter has been overridden to use a subclass of the property's type #28917

Closed
telxs opened this issue Dec 7, 2021 · 10 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@telxs
Copy link

telxs commented Dec 7, 2021

Version: Spring Boot 2.5.7

Add BeanProperty from sub class first ,then ignore exist BeanProperty in super class (only use single parameter in BiConsumer),and finally bind to the properties of the super class, resulting in the loss of the subclass property extension.

private void addProperties(Class<?> type) {
	while (type != null && !Object.class.equals(type)) {
		Method[] declaredMethods = getSorted(type, Class::getDeclaredMethods, Method::getName);
		Field[] declaredFields = getSorted(type, Class::getDeclaredFields, Field::getName);
		addProperties(declaredMethods, declaredFields);
		type = type.getSuperclass();
	}
}

private void addMethodIfPossible(Method method, String prefix, int parameterCount,
		BiConsumer<BeanProperty, Method> consumer) {
	if (method != null && method.getParameterCount() == parameterCount && method.getName().startsWith(prefix)
			&& method.getName().length() > prefix.length()) {
		String propertyName = Introspector.decapitalize(method.getName().substring(prefix.length()));
		consumer.accept(this.properties.computeIfAbsent(propertyName, this::getBeanProperty), method);
	}
}

void addSetter(Method setter) {
	if (this.setter == null || isBetterSetter(setter)) {
		this.setter = setter;
	}
}
@snicoll snicoll transferred this issue from spring-projects/spring-boot Dec 7, 2021
@sbrannen
Copy link
Member

sbrannen commented Dec 7, 2021

@snicoll, this issue is referring to code in Spring Boot's JavaBeanBinder.

Can you provide details on why you think this should be addressed in Framework?

@wilkinsona
Copy link
Member

Sorry, that's my fault. I got my binder classes mixed up. We'll transfer it back.

@snicoll snicoll transferred this issue from spring-projects/spring-framework Dec 7, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 7, 2021
@wilkinsona
Copy link
Member

@jiangbingi Thanks for the report. Unfortunately, I don't think I understand the problem that you've described. Here is an example of a sub-class overriding a property:

package com.example.demo;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;

import com.example.demo.Gh28917Application.ExtensionProperties;

@SpringBootApplication
@EnableConfigurationProperties(ExtensionProperties.class)
public class Gh28917Application {

	public static void main(String[] args) {
		SpringApplication.run(Gh28917Application.class, "--extension.example=test");
	}

	static class BaseProperties {
		
		private String example;

		public String getExample() {
			return example;
		}

		public void setExample(String example) {
			System.out.println("Base = " + example);
			this.example = example;
		}

	}
	
	@ConfigurationProperties("extension")
	static class ExtensionProperties extends BaseProperties {
		
		public void setExample(String example) {
			System.out.println("Extension = " + example);
		}
		
	}


}

Running the above outputs Extension = test.

Could you please provide a similar example that reproduces the problem you are seeing?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Dec 7, 2021
@telxs
Copy link
Author

telxs commented Dec 8, 2021

9d6c457970896ca87286a4b5235882f

package test;

import org.springframework.beans.factory.InitializingBean;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;

@EnableConfigurationProperties(TestApplication.ExtensionProperties.class)
@SpringBootApplication
public class TestApplication {

    public static void main(String[] args) {
        SpringApplication.run(TestApplication.class, "--extension.field.group=my-group", "--extension.field.tag=my-tag");
    }

    static class BaseProperties {

        private BaseField field;

        public BaseField getField() {
            return field;
        }

        public void setField(BaseField field) {
            this.field = field;
        }

        static class BaseField {

            private String group;

            public String getGroup() {
                return group;
            }

            public void setGroup(String group) {
                this.group = group;
            }
        }
    }

    @ConfigurationProperties(prefix = "extension")
    static class ExtensionProperties extends BaseProperties implements InitializingBean {

        private BaseFieldExt field;

        @Override
        public BaseFieldExt getField() {
            return field;
        }

        public void setField(BaseFieldExt field) {
            this.field = field;
        }

        static class BaseFieldExt extends BaseField {

            private String tag;

            public String getTag() {
                return tag;
            }

            public void setTag(String tag) {
                this.tag = tag;
            }
        }

        @Override
        public void afterPropertiesSet() throws Exception {
            if (field != null) {
                System.out.println("bind ExtensionProperties success:" + field.getGroup() + "," + field.getTag());
            } else if (super.getField() != null) {
                System.out.println("bind ExtensionProperties fail. super property is not null:" + super.getField().getGroup());
            }
        }
    }
}

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 8, 2021
@philwebb
Copy link
Member

philwebb commented Dec 9, 2021

The binder is calling ExtensionProperties.setField(...) with a bound instance and ExtensionProperties.field is being set. You are calling super.getField() which is returning BaseProperties.field which has not been set. You probably want to update the setter in ExtensionProperties as follows:

public void setField(BaseFieldExt field) {
    this.field = field;
    super.setField(field);
}

@philwebb philwebb closed this as completed Dec 9, 2021
@philwebb philwebb added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Dec 9, 2021
@telxs
Copy link
Author

telxs commented Dec 9, 2021

@philwebb The reality is that ExtensionProperties.field has not been set.

 @Override
        public void afterPropertiesSet() throws Exception {
            if (field != null) {
                System.out.println("bind ExtensionProperties success:" + field.getGroup() + "," + field.getTag());
            } else if (super.getField() != null) {
                // Here ExtensionProperties.field is null, and BaseProperties.filed is not null
                System.out.println("bind ExtensionProperties fail. super property is not null:" + super.getField().getGroup());
            }
        }

@wilkinsona
Copy link
Member

The behaviour appears to be inconsistent. On repeated runs, sometimes the success message is output and sometimes the fail message is output:

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::                (v2.5.7)

2021-12-09 10:48:49.743  INFO 43098 --- [           main] com.example.demo.TestApplication         : Starting TestApplication using Java 1.8.0_252 on wilkinsona-a01.vmware.com with PID 43098 (/Users/awilkinson/dev/workspaces/spring-projects/spring-boot/2.6.x/gh-28917/bin/main started by awilkinson in /Users/awilkinson/dev/workspaces/spring-projects/spring-boot/2.6.x/gh-28917)
2021-12-09 10:48:49.745  INFO 43098 --- [           main] com.example.demo.TestApplication         : No active profile set, falling back to default profiles: default
bind ExtensionProperties fail. super property is not null:my-group
2021-12-09 10:48:50.122  INFO 43098 --- [           main] com.example.demo.TestApplication         : Started TestApplication in 0.65 seconds (JVM running for 0.946)

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::                (v2.5.7)

2021-12-09 10:50:03.309  INFO 43112 --- [           main] com.example.demo.TestApplication         : Starting TestApplication using Java 1.8.0_252 on wilkinsona-a01.vmware.com with PID 43112 (/Users/awilkinson/dev/workspaces/spring-projects/spring-boot/2.6.x/gh-28917/bin/main started by awilkinson in /Users/awilkinson/dev/workspaces/spring-projects/spring-boot/2.6.x/gh-28917)
2021-12-09 10:50:03.311  INFO 43112 --- [           main] com.example.demo.TestApplication         : No active profile set, falling back to default profiles: default
bind ExtensionProperties success:my-group,my-tag
2021-12-09 10:50:03.695  INFO 43112 --- [           main] com.example.demo.TestApplication         : Started TestApplication in 0.646 seconds (JVM running for 0.94)

@wilkinsona wilkinsona reopened this Dec 9, 2021
@wilkinsona
Copy link
Member

The sorting of the declared methods is the problem. Methods are sorted by name and there are two methods named getField. This results in their ordering being undefined and variable. The getter that goes first has a knock-on effect upon which setter is considered to be better.

@wilkinsona wilkinsona added type: bug A general bug and removed status: invalid An issue that we don't feel is valid labels Dec 9, 2021
@wilkinsona wilkinsona changed the title can't bind sub class property if parent class exist the same name property In JavaBeanBinder The getter and setter that's used during configuration property binding varies when a getter or setter has been overridden to use a subclass of the property's type Dec 9, 2021
@wilkinsona wilkinsona added this to the 2.5.x milestone Dec 9, 2021
@telxs
Copy link
Author

telxs commented Dec 15, 2021

consider the better getter is determined by the type of field ? like this :

private boolean isBetterGetter(Method getter) {
	return this.getter != null && (this.getter.getName().startsWith("is")
			|| this.field != null && this.field.getType().equals(getter.getReturnType()));
}

And it's possible to add field when adding method.

@scottfrederick scottfrederick self-assigned this Dec 17, 2021
@scottfrederick
Copy link
Contributor

scottfrederick commented Dec 17, 2021

In the example above, calling java.lang.Class.getDeclaringMethods on the ExtensionProperties class will return three methods:

  1. BaseField getField()
  2. BaseFieldExt getField()
  3. void setField(BaseFieldExt field)

When the superclass BaseProperties is inspected, getDeclaringMethods will return two methods:

  1. BaseField getField()
  2. void setField(BaseField field)

When walking all the methods of the class and superclass, there are three methods named getField(). The first one encountered will win and will influence which of the two setters is chosen. In the case of ExtensionProperties, the BaseField getField() method is a synthetic bridge method generated and added to the class by the compiler. This is why getDeclaringMethods is returning a method with this signature even though it advertises that the methods it returns excludes inherited methods.

We should be able to exclude bridge methods from consideration when choosing getters and setters to enable use cases like this.

@scottfrederick scottfrederick added this to the 2.5.8 milestone Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants