Skip to content

Relocate @ConstructorBinding to bind package and update default behavior #32660

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

Closed
wind57 opened this issue Oct 10, 2022 · 10 comments
Closed

Relocate @ConstructorBinding to bind package and update default behavior #32660

wind57 opened this issue Oct 10, 2022 · 10 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@wind57
Copy link

wind57 commented Oct 10, 2022

In spring-cloud-kubernetes we have support for configdata, as such, much of the underlying code uses snippets likes these:

binder.bind("spring.cloud.kubernetes.config", ConfigMapConfigProperties.class)

which works.

But I am trying to change some code to move configs to records, so I have code that does this, for example:

AbstractConfigProperties.RetryProperties properties = binder.bind("spring.cloud.kubernetes.config.retry", AbstractConfigProperties.RetryProperties.class)
			.orElseGet(AbstractConfigProperties.RetryProperties::new);

where AbstractConfigProperties.RetryProperties is a record. Unfortunately, this does not work and binding does not happen.


My limited knowledge of Binder functionality does not allow me to figure out how to do it, and if it is possible at all. :( I can't provide a simple example here, but I could push the branch that I am currently working on and tell what test to run in order to reproduce this.

I am also not sure the name of the issue is appropriate, but after a few hours of debug I am inclined to think that JavaBeanBinder is the one responsible for this.

Thank you for looking into this.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 10, 2022
@wilkinsona
Copy link
Member

Binding to records is performed by the value object binder and should work – we have tests that verify that it does. Without a minimal sample and with only "does not work" as a description of the failure we aren't going to be able to help you. If you would like us to spend some more time investigating, please spend some time providing a complete yet minimal sample that reproduces the problem. You can share it with us by pushing it to a separate repository on GitHub or by zipping it up and attaching it to this issue.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Oct 10, 2022
@philwebb
Copy link
Member

You might also want to try the bindOrCreate method rather than using .orElseGet(AbstractConfigProperties.RetryProperties::new). That method will deal with @DefaultValue annotations.

@wind57
Copy link
Author

wind57 commented Oct 11, 2022

I am sorry, you are right, I should have provided an example to begin with.

here is one. I am not sure if I am entirely doing things correctly...

If I comment the constructor in SimpleServiceTest:

    public SimpleRecord() {
        this(5);
    }

the test passes.

@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 Oct 11, 2022
@wind57
Copy link
Author

wind57 commented Oct 11, 2022

I've also added one more example in that repo, with a nested record. This might need to be a separate issue, not sure.

@philwebb
Copy link
Member

The problem in your sample is caused because you're trying to use the low-level Binder to directly bind @ConfigurationProperties classes and there's no org.springframework.boot.context.properties.bind.BindConstructorProvider instance telling the binder which constructor to use.

The simplest fix for your sample is to inject the SimpleRecord bean directly into your service rather than trying to bind it. The bean will have been created by the ConfigurationPropertiesBeanRegistrar which will use the ConfigurationPropertiesBindConstructorProvider to identify the @ConstructorBinding annotated constructor.

If you do want to use the Binder directly for whatever reason you'll need to write your own BindConstructorProvider (at least for Spring Boot 2.7.x, things might be easier in Spring Boot 3.0).

Here's an updated SimpleService implementation that uses a BindConstructorProvider that just picks the first constructor for records:

@Component
public class SimpleService {

	@Autowired
	private ApplicationContext applicationContext;

	@Autowired
	private Environment environment;

	public SimpleRecord simple() {
		Iterable<ConfigurationPropertySource> configurationPropertySources = ConfigurationPropertySources
				.from(((ConfigurableEnvironment) environment).getPropertySources());
		PropertySourcesPlaceholdersResolver placeholdersResolver = new PropertySourcesPlaceholdersResolver(environment);
		Consumer<PropertyEditorRegistry> propertyEditorInitializer = ((ConfigurableApplicationContext) applicationContext)
				.getBeanFactory()::copyRegisteredEditorsTo;
		BindConstructorProvider constructorProvider = (bindable, isNestedConstructorBinding) -> {
			Class<?> type = bindable.getType().resolve(Object.class);
			return type.isRecord() ? type.getConstructors()[0] : null;
		};
		Binder binder = new Binder(configurationPropertySources, placeholdersResolver,
				ApplicationConversionService.getSharedInstance(), propertyEditorInitializer, null, constructorProvider);
		return binder.bindOrCreate("retry", SimpleRecord.class);
	}

}

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2022
@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 Oct 11, 2022
@wind57
Copy link
Author

wind57 commented Oct 11, 2022

I'll take a look at the sample, thank you.

The thing is we use this in spring-cloud-kubernetes (where I am a minor contributor), see here.

So we do have boot-3... I do not want to sound impolite, but can you may be show how to do it (or a hint where to look) for spring-boot-3? I very much appreciate your effort here. thank you.

@philwebb philwebb reopened this Oct 11, 2022
@philwebb
Copy link
Member

Ohh, I see the Binder here is obtained from the ConfigDataLocationResolverContext. That makes things quite tricky because you can't set the BindConstructorProvider.

can you may be show how to do it (or a hint where to look) for spring-boot-3?

The DefaultBindConstructorProvider in Spring Boot 3 will return a single record based constructor. One you're on Spring Boot 3.0, I think that simple records with a single constructor will work.

Flagging this one for further team discussion as we need to decide if we want to support a way to use a custom BindConstructorProvider with an existing Binder.

@philwebb philwebb added status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: invalid An issue that we don't feel is valid labels Oct 11, 2022
@philwebb philwebb changed the title JavaBeanBinder does not support records Binder provided by ConfigDataLocationResolverContext cannot use a custom BindConstructorProvider Oct 11, 2022
@philwebb philwebb changed the title Binder provided by ConfigDataLocationResolverContext cannot use a custom BindConstructorProvider BindConstructorProvider cannot be plugged into existing Binder instance Oct 11, 2022
@wind57
Copy link
Author

wind57 commented Oct 12, 2022

you're right, this does work for a record with a single constructor, but we have a different problem other there.

The record is nested inside ConfigMapConfigProperties, and that does not work - even if I change the record to have a single constructor. What I means is this:

ConfigMapConfigProperties.RetryProperties retryProperties = binder.bindOrCreate("spring.cloud.kubernetes.config.retry", ConfigMapConfigProperties.RetryProperties.class);

will correctly create the defaults and inject all the properties I have defined under spring.cloud.kubernetes.config.retry.

While this:

ConfigMapConfigProperties properties = binder.bindOrCreate("spring.cloud.kubernetes.config", ConfigMapConfigProperties.class);

will only apply defaults, without injecting any properties I have defined, for the Retry record.

That btw is what I was trying to show in the sample I provided, with NestedService.

I hope I make sense here.

P.S. I can workaround this, but it isn't elegant at all, imho.

@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Oct 13, 2022
@wilkinsona wilkinsona added this to the 3.0.x milestone Oct 13, 2022
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 13, 2022
@philwebb
Copy link
Member

We spoke about this today and we're going to look to see if we can move @ConstructorBinding to the bind package and align the default behavior.

@philwebb philwebb changed the title BindConstructorProvider cannot be plugged into existing Binder instance Relocate @ConstructorBinding to bind package and update default behavior Oct 18, 2022
@philwebb philwebb modified the milestones: 3.0.x, 3.0.0-RC1 Oct 18, 2022
@wilkinsona
Copy link
Member

The annotation processor is still looking for org.springframework.boot.context.properties.ConstructorBinding.

@wilkinsona wilkinsona reopened this Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants