Skip to content

ConfigurationPropertiesBean detects class-level @ConstructorBinding unexpectedly #18685

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
snicoll opened this issue Oct 22, 2019 · 3 comments
Closed
Assignees
Labels
type: task A general task
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Oct 22, 2019

Consider the following:

@ConfigurationProperties("test")
@ConstructorBinding
public class TestProperties {

    public TestProperties(String property, int counter) { ... }

}

And the current code:

Constructor<?> bindConstructor = BindMethod.findBindConstructor(type);
BindMethod bindMethod = (bindConstructor != null) ? BindMethod.VALUE_OBJECT : BindMethod.forClass(type);

BindMethod.findBindConstructor(TestProperties.class) will return null as it does not test the fact @ConstructorBinding is specified at class level. However, BindMethod.forClass(type) will work as it has the extra assertion.

The current code arrangement feels a bit weird considering we have a dedicated method to retrieve the relevant constructor and that returns null whereas it shouldn't.

@snicoll snicoll added the type: task A general task label Oct 22, 2019
@snicoll snicoll added this to the 2.2.x milestone Oct 22, 2019
@snicoll
Copy link
Member Author

snicoll commented Oct 22, 2019

An additional note that the binder takes any constructor when built in such a mode (i.e. I don't see an immediate check that only one relevant constructor is present).

Something else that I find a bit odd is that that check detects the Constructor to use but doesn't seem to store that anywhere. I think it would be worthwhile to store the result of the search so that the binder can call exactly what was detected.

@snicoll
Copy link
Member Author

snicoll commented Oct 24, 2019

At this point, I don't understand why we store a Predicate<Constructor<?>> rather than the Constructor<?> to use effectively as this information is statically defined and we have access to that.

@mbhave
Copy link
Contributor

mbhave commented Nov 2, 2019

I think this can be refactored as part of #18670. There are a few bugs covered by these tests which might involve touching a lot of that code. We can leave the issue open so that we don't forget about it. Assigning myself to avoid any duplication of effort.

@mbhave mbhave self-assigned this Nov 2, 2019
@mbhave mbhave closed this as completed in f9785d2 Nov 5, 2019
@mbhave mbhave modified the milestones: 2.2.x, 2.2.1 Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

No branches or pull requests

2 participants