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

@Autowired is sometimes required on the constructor of a ConfigurationProperties class just to ensure that the correct metadata is generated #34031

Closed
sobychacko opened this issue Jan 31, 2023 · 13 comments
Labels
status: duplicate A duplicate of another issue type: bug A general bug

Comments

@sobychacko
Copy link
Contributor

Using Boot 3.0.2, we see a behavior in which only properties injected through a constructor binding is generated as part of spring-configuration-metadata.json. Any other properties in the ConfigurationProperties class are omitted from it.

Let's say we have the following ConfigurationProperties class.

@ConfigurationProperties("test.properties")
public class TestProperties {

    private final MyData myData;

    private String custom;

    public TestProperties(MyData myData) {
        this.myData = myData;
    }

    public MyData getMyData() {
        return myData;
    }

    public String getCustom() {
        return custom;
    }

    public void setCustom(String custom) {
        this.custom = custom;
    }
}

and

record MyData(String data) {}

We see the following in the generated spring-configuration-metadata.json. The custom property in the above ConfigurationProperty class is not present.

{
  "groups": [
    {
      "name": "test.properties",
      "type": "com.example.demo.TestProperties",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "properties": [
    {
      "name": "test.properties.my-data",
      "type": "com.example.demo.MyData",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "hints": []
}

This issue is related to spring-cloud/spring-cloud-stream#2640

@ghost

This comment was marked as off-topic.

@philwebb

This comment was marked as outdated.

@wilkinsona
Copy link
Member

wilkinsona commented Feb 7, 2023

@sobychacko is your expectation that both test.properties.my-data.data and test.properties.custom will be documented at build time and bound at runtime? We don't support both constructor binding and JavaBean-style binding of the same class so this won't work. You'll either get test.properties.my-data.data through constructor binding or test.properties.custom through JavaBean-style binding but not both at the same time.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Feb 7, 2023
@sobychacko
Copy link
Contributor Author

@wilkinsona Thank you for the reply. When I spoke to @philwebb about this issue, I thought there might be a way to get around that limitation. My expectation was that both constructor-binding and JavaBean styles could co-exist. In the Kafka binder for Spring Cloud Stream, we have a ConfigurationProperties class that uses both approaches. It looks like we need to remove that constructor injection for KafkaProperties. cc @garyrussell

@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 Feb 7, 2023
@wilkinsona
Copy link
Member

It looks like the constructor that consumes KafkaProperties predates Boot's support for constructor binding so it would be interesting to understand the original intent.

KafkaBinderConfigurationProperties is defined as a @Bean, injecting an instance of Boot's KafkaProperties. Those KafkaProperties aren't constructor bound and aren't part of the spring.cloud.stream.kafka.binder namespace so I don't think they should be documented as such.

As far as I can tell, the problem here is that the annotation processor that generates the metadata may incorrectly determine that constructor binding is going to be used when it's actually JavaBean binding that will be performed. @Autowired on the constructor should fix it but perhaps the annotation processor isn't taking that into account when determining how the class's properties will be bound.

@sobychacko
Copy link
Contributor Author

sobychacko commented Feb 7, 2023

I tried the @Autowired approach for KafkaProperties, but I didn't notice any difference. We don't need to document all the individual properties in KafkaProperties under the spring.cloud.stream.kafka.binder namespace. What's happening is just the opposite now, i.e., all the relevant properties are missing since KafkaProperties uses constructor binding and thus eclipses all the other individual properties.

@wilkinsona
Copy link
Member

With the above TestProperties class, Boot 2.7 produces this metadata:

{
  "groups": [
    {
      "name": "test.properties",
      "type": "com.example.demo.TestProperties",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "properties": [
    {
      "name": "test.properties.custom",
      "type": "java.lang.String",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "hints": []
}

(The app also won't start as there are no MyData beans).

Boot 3.0 produces the following metadata:

{
  "groups": [
    {
      "name": "test.properties",
      "type": "com.example.demo.TestProperties",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "properties": [
    {
      "name": "test.properties.my-data",
      "type": "com.example.demo.MyData",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "hints": []
}

This is due to the improved @ConstructorBinding detection in 3.0:

If, however, you have a @ConfigurationProperties and you want to inject beans into the constructor rather than binding it, you’ll now need to add an @Autowired annotation.

While the constructor injection is manual in the @Bean method in Cloud Stream's Kafka Binder, that's effectively the situation here. Adding @Autowired to the constructor of TestProperties results in Boot 3.0.2 producing the following metadata:

{
  "groups": [
    {
      "name": "test.properties",
      "type": "com.example.demo.TestProperties",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "properties": [
    {
      "name": "test.properties.custom",
      "type": "java.lang.String",
      "sourceType": "com.example.demo.TestProperties"
    }
  ],
  "hints": []
}

Things seem to be working as they should and @Autowired has the expected effect on metadata generation. That matches the code which takes @Autowired into account when deducing the bind constructor.

@sobychacko Can you please double-check that @Autowired wasn't effective?

@sobychacko
Copy link
Contributor Author

Ah ok. I did see the same output that you got above for Boot 3.0. I will try the @Autowired approach once again. I might not have tried it properly. I will confirm for you.

@sobychacko
Copy link
Contributor Author

Adding @Autowired to the constructor indeed works now. I think I was adding the @Autowired to the constructor arg before, which didn't work. You can close this issue now. Sorry for the noise, and thanks for all the help!

sobychacko added a commit to sobychacko/spring-cloud-stream that referenced this issue Feb 7, 2023
When generating configuration metadata (spring-configuration-metadata.json),
KafkaBinderConfigurationProperties class is missing all the information except
for the constructor bound KafkaProperties. We need to add the @Autowired on the
constructor so that Boot will not do a constructor binding and thus include all
the JavaBeans style properties in the metadata. See this Boot issue for more details.

spring-projects/spring-boot#34031

In addition, this @Autowired is necessary for the properties to be correctly
bound when running in native mode.

Resolves spring-cloud#2640
Resolves spring-cloud#2644
@wilkinsona
Copy link
Member

Thanks, @sobychacko. I'm going to leave this open for a bit as I'd like to discuss things with the team to see if we can figure out a way to make @Autowired unnecessary. It isn't ideal that you need to add it to a constructor that won't actually be used for auto-wiring (as you're calling it manually from a @Bean method) just to make the metadata generation work correctly.

@wilkinsona wilkinsona added for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: feedback-provided Feedback has been provided labels Feb 8, 2023
@wilkinsona wilkinsona changed the title Constructor binding in ConfigurationProperties do not include other properties in metadata @Autowired is something required on the constructor of a ConfigurationProperties class just to ensure that the correct metadata is generated Feb 8, 2023
@wilkinsona wilkinsona changed the title @Autowired is something required on the constructor of a ConfigurationProperties class just to ensure that the correct metadata is generated @Autowired is sometimes required on the constructor of a ConfigurationProperties class just to ensure that the correct metadata is generated Feb 8, 2023
@philwebb
Copy link
Member

We're going to look at a way to signal that bean binding should be used that isn't @Autowired. We're considering an enum on @ConfigurationProperties.

@philwebb philwebb added type: bug A general bug and removed 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 labels Feb 15, 2023
@philwebb philwebb added this to the 3.0.x milestone Feb 15, 2023
@philwebb
Copy link
Member

I started looking at adding an enum to @ConfigurationProperties but it's quite involved for a patch release. It might be better to recommend that @ConfigurationProperties is used on the @Bean method rather than the class for now.

@philwebb philwebb added the status: on-hold We can't start working on this issue yet label Feb 23, 2023
@philwebb philwebb modified the milestones: 3.0.x, 3.x Feb 23, 2023
@wilkinsona
Copy link
Member

With thanks to @sobychacko for verifying, this has been fixed by #35564.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Sep 8, 2023
@wilkinsona wilkinsona removed this from the 3.x milestone Sep 8, 2023
@wilkinsona wilkinsona added status: duplicate A duplicate of another issue and removed status: on-hold We can't start working on this issue yet labels Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants