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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

terminux
Copy link
Contributor

@terminux terminux commented Apr 7, 2023

Hi, I found that PropertySourcesPropertyResolver (a resolver that does not support short-circuiting) is used in these places:

  • LoggingSystemProperties

PropertySourcesPropertyResolver resolver = new PropertySourcesPropertyResolver(

  • When the key is not a property name, such as when it is a SpEL expression:

In this case ConfigurationPropertyName.of(key, true) will return null, and then defaultResolver (inherited from PropertySourcesPropertyResolver) will be used for resolution.

ConfigurationPropertyName name = ConfigurationPropertyName.of(key, true);
if (name != null) {
try {
ConfigurationProperty configurationProperty = attached.findConfigurationProperty(name);
return (configurationProperty != null) ? configurationProperty.getValue() : null;
}
catch (Exception ex) {
}
}
}
return this.defaultResolver.getProperty(key, Object.class, false);

However the entire process property value will only be retrieved once, the current implementation is fine and we don't need to make any changes.

Closes gh-28687

@@ -20,11 +20,12 @@
import java.nio.charset.StandardCharsets;
import java.util.function.BiConsumer;

import org.springframework.boot.context.properties.source.ConfigurationPropertySources;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logging package does not allow the content of the context package to be imported, I am not sure if I need to modify the rules or use other methods to create property resolver.

<disallow pkg="org.springframework.boot.context" />

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That restriction was introduced in #8611. I'm not sure if we should relax it or not. I wonder if we can get away with using the Environment directly since it's already a PropertyResolver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your reply! If you use Environment directly, we may need to set its ignoreUnresolvableNestedPlaceholders property to true to ensure that it is consistent with the original behavior. Since the Environment is modified, this will also affect the external use of it. Maybe we should make a copy of Environment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I need to think about this one for a bit.

@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 20, 2023
@philwebb philwebb self-assigned this Jun 20, 2023
@philwebb philwebb added this to the 3.1.x milestone Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
3 participants