codeconsole commented on code in PR #16795:
URL: https://github.com/apache/grails-core/pull/16795#discussion_r4417744323
##########
grails-core/src/main/groovy/org/grails/spring/context/support/GrailsPlaceholderConfigurer.java:
##########
@@ -78,6 +92,34 @@ public void setBeanFactory(BeanFactory beanFactory) {
this.beanFactory = beanFactory;
}
+ @Override
+ public void setEnvironment(Environment environment) {
+ super.setEnvironment(environment);
+ this.environment = environment;
+ }
+
+ /**
+ * Resolves a placeholder that names a list of plain values, such as
{@code ${app.names}}, to the
+ * elements of the list joined by commas, when neither the environment nor
the local properties have
+ * a value under that name.
+ *
+ * <p>The environment presents a configuration list only under its indexed
names, such as
+ * {@code app.names[0]}, so that Spring Boot binds it element by element.
The list is read as Spring
+ * Boot binds it, from the property source of the highest priority that
sets it, and the placeholders
+ * in its elements are resolved with the rest of the value. The name is
matched as
+ * {@code @ConfigurationProperties} binding matches it, so {@code
${app.allowedOrigins}} and
+ * {@code ${app.allowed-origins}} read the same list. A list holding
objects has no value a
+ * placeholder could be replaced with, so a placeholder naming it is not
resolved.</p>
+ */
+ @Override
+ protected ConfigurablePropertyResolver
createPropertyResolver(MutablePropertySources propertySources) {
+ if (this.environment instanceof ConfigurableEnvironment
configurableEnvironment &&
+
!propertySources.contains(CONFIGURATION_LISTS_PROPERTY_SOURCE_NAME)) {
Review Comment:
Following up on @jamesfredley's comment above: I reproduced it, and it isn't
limited to lists. `Bindable.listOf(String.class)` also binds a scalar, as a
one-element list, so when the configurer is given an explicit source list, this
source answers *every* environment property, not just lists. I loaded
`{app.names: [first, second], app.scalar: envscalar}` into the environment and
injected `${app.names:fallback}` and `${app.scalar:fallback}`:
| configurer | `app.names` | `app.scalar` |
|---|---|---|
| plain `PropertySourcesPlaceholderConfigurer`, `propertySources = new
MutablePropertySources()` | `fallback` | `fallback` |
| `GrailsPlaceholderConfigurer` at 40f143239e, empty sources |
`first,second` | `envscalar` |
| `GrailsPlaceholderConfigurer` at 40f143239e, sources = one unrelated
`MapPropertySource` | `first,second` | `envscalar` |
| `GrailsPlaceholderConfigurer` at 40f143239e, default sources |
`first,second` | `envscalar` |
So selecting your own sources no longer keeps the environment out. Adding
the list source only when Spring has put the environment in the list fixes it:
```java
if (this.environment instanceof ConfigurableEnvironment
configurableEnvironment &&
propertySources.contains(ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME) &&
!propertySources.contains(CONFIGURATION_LISTS_PROPERTY_SOURCE_NAME))
{
```
With that line, both explicit-source rows give `fallback, fallback` as plain
Spring does, the default row is unchanged, and all 15 features of
`GrailsPlaceholderConfigurerSpec` pass. Could you add a feature for it, such as
`setPropertySources(new MutablePropertySources())` with `${app.names:fallback}`
and an `app.names` list in the environment, expecting `fallback`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
