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.

Sign up for GitHub

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

Jump to bottom

Short circuit @Value injection when using a source already covered by ConfigurationPropertySourcesPropertySource #28687

Closed
ShijunDeng opened this issue Nov 16, 2021 · 5 comments · May be fixed by #34900
Closed

Short circuit @Value injection when using a source already covered by ConfigurationPropertySourcesPropertySource #28687

ShijunDeng opened this issue Nov 16, 2021 · 5 comments · May be fixed by #34900
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@ShijunDeng
Copy link

spring-boot/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySourcesPropertySource.java

Line 33 in aaad772

class ConfigurationPropertySourcesPropertySource extends PropertySource<Iterable<ConfigurationPropertySource>>

In Springboot application，ConfigurationPropertySourcesPropertySource is added to the environment and as first element, the ConfigurationPropertySourcesPropertySource acts as a facade over the existing property sources.

When handling @Value annotation, org.springframework.core.env.PropertySourcesPropertyResolver#getProperty will be called.
If the property is found then ConfigurationPropertySourcesPropertySource will return the result. otherwise, PropertySourcesPropertyResolver continues to check all subsequent sources. This is a little inefficient for any source that has already been check via ConfigurationPropertySourcesPropertySource.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 16, 2021
@philwebb
Copy link
Member

I think we may have fixed this already with #17400 (commit 6ad100e) which was added to Spring Boot 2.5. @ShijunDeng Can you check that you're using Spring Boot 2.5 or above? If you're finding an issue with that version could you please please provide a sample that shows the problem.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Nov 16, 2021
@ShijunDeng
Copy link
Author

ShijunDeng commented Nov 16, 2021 •
edited by philwebb

New similar problem with ConfigurationPropertySourcesPropertyResolver in #17400. My Spring Boot version is 2.5.6.

ConfigurationPropertySourcesPropertyResolver is Spring boot source code, but org.springframework.core.env.PropertySourcesPropertyResolver#getProperty is defined in spring-core. The @Value processor use PropertySourcesPropertyResolver. Set a breakpoint in method org.springframework.core.env.PropertySourcesPropertyResolver#getProperty, it's easy to see that PropertySourcesPropertyResolver continues to check all subsequent sources when not found key in ConfigurationPropertySourcesPropertySource.

as the screenshort below:
image

image

PropertySourcesPropertyResolver continues to check subsequent source such as SimpleCommandLinePropertySource {name='commandLineArgs'}

@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 Nov 16, 2021
@ShijunDeng
Copy link
Author

@philwebb ，hi，How is it going ？

@philwebb
Copy link
Member

philwebb commented Dec 9, 2021

I need to dig a bit more, but I suspect this resolver is being used which doesn't support the short-circuiting.

@philwebb philwebb changed the title Short circuit checking of source already covered by ConfigurationPropertySourcesPropertySource Short circuit @Value injection when using a source already covered by ConfigurationPropertySourcesPropertySource Dec 9, 2021
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Dec 9, 2021
@philwebb philwebb added this to the 2.x milestone Dec 9, 2021
@philwebb philwebb modified the milestones: 2.x, 3.x Aug 19, 2022
@terminux terminux mentioned this issue Apr 7, 2023
Closed
terminux added a commit to terminux/spring-boot that referenced this issue Apr 7, 2023
@terminux
Use configuration resolver to replace standard resolver
a2d0dc8 
Closes spring-projectsgh-28687
@terminux terminux mentioned this issue Apr 7, 2023
Open
terminux added a commit to terminux/spring-boot that referenced this issue Apr 7, 2023
@terminux
Use configuration resolver to replace standard resolver
6a09f4d 
Closes spring-projectsgh-28687
terminux added a commit to terminux/spring-boot that referenced this issue Apr 10, 2023
@terminux
Use configuration resolver to replace standard resolver
f245fa5 
Closes spring-projectsgh-28687
terminux added a commit to terminux/spring-boot that referenced this issue Apr 10, 2023
@terminux
Use configuration resolver to replace standard resolver
f85a29d 
Closes spring-projectsgh-28687
terminux added a commit to terminux/spring-boot that referenced this issue Apr 10, 2023
@terminux
Use configuration resolver to replace standard resolver
15f27d2 
Closes spring-projectsgh-28687
@philwebb
Copy link
Member

Closing in favor of PR #34900. Thanks @terminux!

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Jun 20, 2023
@snicoll snicoll removed this from the 3.x milestone Jun 21, 2023
@snicoll snicoll added the status: superseded An issue that has been superseded by another label Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
4 participants
@snicoll @philwebb @spring-projects-issues @ShijunDeng