Skip to content

Provide a way to bind classes with multiple prefixes #26144

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
asarkar opened this issue Apr 16, 2021 · 9 comments
Closed

Provide a way to bind classes with multiple prefixes #26144

asarkar opened this issue Apr 16, 2021 · 9 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@asarkar
Copy link

asarkar commented Apr 16, 2021

I've a use case where I need to bind configuration properties based on two prefixes, one of which is determined at runtime. Let's say the constant prefix is foo and the runtime prefix is bar.

Given a new instance of Java Bean class FooBar, the code should bind all environment variables FOO_, then overwrite with all environment variables BAR_.

public class FooBar {
    private Duration latency = Duration.ofMillis(500L);
    // other properties
    // getters, setters
}

If there are no environment variables FOO_LATENCY or BAR_LATENCY, FooBar.getLatency() is 500 ms. If only one of FOO_LATENCY and BAR_LATENCY is present, FooBar.getLatency() takes its value. If both FOO_LATENCY and BAR_LATENCY are present, FooBar.getLatency() takes the value of BAR_LATENCY.

This SO answer shows a way to do it by repeated binding. It works, but it confusing.

It'd be nice to be able to merge BindResults. Some ideas off the top of my head:

  1. binder.bind accepts multiple prefixes, not just one, latter progressively overwriting the former.
  2. bindResult.mergeWith(anotherBindResult), latter overwriting the former.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 16, 2021
@wilkinsona
Copy link
Member

The answer on Stack Overflow can be simplified a little bit as you're binding to an existing instance:

FooBar fooBar = new FooBar();
binder.bind("x", Bindable.ofInstance(fooBar));
binder.bind("y", Bindable.ofInstance(fooBar));

It's obviously subjective, but that reads quite nicely to my eyes.

I think merging results would look something like this:

FooBar fooBar = new FooBar();
BindResult<FooBar> x = binder.bind("x", Bindable.ofInstance(fooBar));
BindResult<FooBar> y = binder.bind("y", Bindable.ofInstance(fooBar));
fooBar = x.mergeWith(y).get();

That's slightly more verbose and I don't find it any more readable.

Binding multiple prefixes would look something like this:

FooBar fooBar = new FooBar();
binder.bind(Arrays.asList("x", "y"), Bindable.ofInstance(fooBar));

That's more concise than binding twice, but I find it less readable. It's not as clear to me how the two prefixes will be handled.

Let's see what the rest of the team thinks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 16, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Apr 16, 2021

One benefit of the multiple prefixes approach is that it would support constructor binding to an immutable type.

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Apr 16, 2021
@asarkar
Copy link
Author

asarkar commented Apr 16, 2021

It's not as clear to me how the two prefixes will be handled

IMO, the inherent ordering in a list is a good indication that the prefixes are handled as shown.

@asarkar
Copy link
Author

asarkar commented Apr 16, 2021

One benefit of the multiple prefixes approach is that it would support constructor binding to an immutable type.

It can also be done with mergeWith?:

Bindable bindable = Bindable.of(FooBar.class);
binder.bind("x", bindable)
    .mergeWith(binder.bind("y", bindable))
    .orElse(new FooBar());

@philwebb
Copy link
Member

Not so easily unfortunately. The org.springframework.boot.context.properties.bind.ValueObjectBinder class creates objects during the actual bind operation. The BindResult object doesn't actually know how to create new instances. Things get even more complex when you consider nested object.

The Binder code is quite complex and I suspect that supporting multiple prefixes will be significantly easier than adding BindResult.mergeWith.

@philwebb
Copy link
Member

One other thing we'll need to consider is merging rules for Lists, Maps and Arrays. We should probably keep the same logic as the current Binder where Lists and Arrays are only taken from the current property source.

@philwebb philwebb changed the title Provide an API to merge bind results Provide a way to bind classes with mutliple prefixes Apr 17, 2021
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 17, 2021
@philwebb philwebb added this to the General Backlog milestone Apr 17, 2021
@wilkinsona wilkinsona changed the title Provide a way to bind classes with mutliple prefixes Provide a way to bind classes with multiple prefixes Apr 17, 2021
@philwebb philwebb modified the milestones: General Backlog, 2.6.x May 12, 2021
@philwebb
Copy link
Member

Spring Cloud would also like this feature.

@philwebb
Copy link
Member

See also #7986

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 12, 2021
@mbhave mbhave removed their assignment Aug 12, 2021
@philwebb philwebb modified the milestones: 2.6.x, 2.x Aug 16, 2021
@philwebb philwebb added status: pending-design-work Needs design work before any code can be developed and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Aug 16, 2021
@mbhave
Copy link
Contributor

mbhave commented Dec 8, 2021

We discussed this with the Spring Cloud team and they were able to use a different approach to support their use-case.

We think it's best not to add multiple prefix support to the binder at this time, given how complex the code is and that there is way to achieve the fallback, if necessary, by calling bind multiple times.

@mbhave mbhave closed this as completed Dec 8, 2021
@mbhave mbhave removed type: enhancement A general enhancement status: pending-design-work Needs design work before any code can be developed labels Dec 8, 2021
@mbhave mbhave added the status: declined A suggestion or change that we don't feel we should currently apply label Dec 8, 2021
@mbhave mbhave removed this from the 2.x milestone Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

5 participants