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

KafkaBinderConfigurationProperties is still not showing up in the generated JSON for config props #2640

Closed
rod2j opened this issue Jan 31, 2023 · 11 comments
Assignees
Milestone

Comments

@rod2j
Copy link

rod2j commented Jan 31, 2023

Issue spring-cloud-stream-binder-kafka#750, despite having been closed, is still present in version 4.0.1 of Spring Cloud Stream.

KafkaBinderConfigurationProperties still has a single parameterized constructor, so "ConstructorBinding" is still automatically applied,
resulting in a spring-configuration-metadata.json missing all the properties of the KafkaBinderConfigurationProperties class, except those of the KafkaProperties class passed as an argument of the constructor.

@sobychacko
Copy link
Contributor

sobychacko commented Jan 31, 2023

@rod2j I created this issue in Spring Boot to address the root cause of this behavior: spring-projects/spring-boot#34031. Thanks for reporting this issue.

@sobychacko sobychacko self-assigned this Feb 7, 2023
@sobychacko sobychacko added this to the 4.0.2 milestone Feb 7, 2023
@sobychacko
Copy link
Contributor

@rod2j This issue is now fixed through the above-referenced commit. Please try it with version 4.0.2-SNAPSHOT of the binder and see if the issue is resolved.

@philwebb
Copy link
Contributor

@sobychacko You might want to try removing @ConfigurationProperties from KafkaBinderConfigurationProperties and adding it to the @Bean method instead:

@ConfigurationProperties
@Bean
KafkaBinderConfigurationProperties configurationProperties(
		KafkaProperties kafkaProperties) {
	return new KafkaBinderConfigurationProperties(kafkaProperties);
}

I think this will give the annotation processor enough information to know that constructor binding cannot be used.

@sobychacko
Copy link
Contributor

sobychacko commented Feb 15, 2023

@philwebb We also noticed that adding the @Autowired to the constructor of KafkaBinderConfigurationProperties solved the problem of running the app as a native one in certain situations. Basically, none of the JavaBean bindings were working without the @Autowired on the constructor, possibly due to not generating the proper reflection hints. However, adding the @Autowired fixed that issue. Do you think if we add the @ConfigurationProperties as you suggested on the bean method, will it behave correctly for the native apps as well?

@philwebb
Copy link
Contributor

@sobychacko I think so, but I've not confirmed that for sure. It's certainly a bug if it doesn't.

@sobychacko sobychacko reopened this Feb 15, 2023
@sobychacko
Copy link
Contributor

Re-opening this issue until we verify the suggestion made by @philwebb ^^.

@sobychacko
Copy link
Contributor

@philwebb Moving @ConfigurationProperties from KafkaBinderConfigurationProperties and adding it as part of the @Bean method works for the proper generation of the configuration metadata. However, that native issue I mentioned above still needs the @autowired on the constructor. Without that, some hints for the JavaBean style properties in KafkaBinderConfigurationProperties are not generated. Do you see any harm in leaving the @autowired on the constructor but moving the @ConfigurationProperties to the @Bean method? Thanks!

@sobychacko
Copy link
Contributor

sobychacko commented Mar 7, 2023

Since the @autowired approach worked for both situations, I wonder whether we can stick with that strategy instead of moving the @ConfigurationProperties to @Bean? What is the downside of using the @autowired approach on the constructor?

@philwebb
Copy link
Contributor

philwebb commented Mar 7, 2023

It should be fine to leave it, it just looks a bit odd because it's not actually being used for autowiring.

@sobychacko
Copy link
Contributor

Re-opened again until we have a resolution for the native issue discussed above.

@sobychacko
Copy link
Contributor

sobychacko commented Sep 7, 2023

Spring Boot fixed the issues, due to why we needed to add @Autowired on the KafkaBinderConfigurationProperties constructor. We removed the @Autowired and confirmed that the native applications work without it. See the following Boot issues for details.
spring-projects/spring-boot#34507
spring-projects/spring-boot#35564

omercelikceng pushed a commit to omercelikceng/spring-cloud-stream that referenced this issue Sep 14, 2023
 - Earlier, we had to add Autowired on KafkaBinderConfigurationProperties constructor
   as there were some issues with runtime hints generation which caused issues when
   running an app in native mode. Spring Boot fixed these issues and we can remove
   this unnecessary Autowired from the constructor.

See the following issues from Spring Boot for more details.

spring-projects/spring-boot#34507
spring-projects/spring-boot#35564

Resolves spring-cloud#2640
omercelikceng pushed a commit to omercelikceng/spring-cloud-stream that referenced this issue Sep 18, 2023
 - Earlier, we had to add Autowired on KafkaBinderConfigurationProperties constructor
   as there were some issues with runtime hints generation which caused issues when
   running an app in native mode. Spring Boot fixed these issues and we can remove
   this unnecessary Autowired from the constructor.

See the following issues from Spring Boot for more details.

spring-projects/spring-boot#34507
spring-projects/spring-boot#35564

Resolves spring-cloud#2640
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants