codeconsole commented on code in PR #16795:
URL: https://github.com/apache/grails-core/pull/16795#discussion_r4416066883
##########
grails-doc/src/en/guide/conf/config.adoc:
##########
@@ -165,9 +165,18 @@ To read a whole list, use
`grailsApplication.config.getProperty('grails.cors.all
`@ConfigurationProperties`. Spring's `Environment` exposes non-empty YAML
lists through indexed properties such as
`grails.cors.allowedOrigins[0]`. A list nested in a list uses one index per
level, such as `app.groups[0][1]`.
-Because the list itself is not a property of the `Environment`, a
`@Value('${grails.cors.allowedOrigins}')` placeholder
-cannot inject it, just as in a plain Spring Boot application. Bind the list
with `@ConfigurationProperties` or with Spring Boot's
-`Binder`, or write the value as one comma-separated string where a `@Value`
placeholder must read it.
+The list itself is not a property of the `Environment`, so
`environment.getProperty('grails.cors.allowedOrigins')`
+returns `null`, as in a plain Spring Boot application. A
`@Value('${grails.cors.allowedOrigins}')` placeholder, or any
Review Comment:
With the current head, this exact example,
`@Value('${grails.cors.allowedOrigins}')`, still injects the placeholder text,
because the configurer rejects the camelCase name (see the comment on
`GrailsPlaceholderConfigurer`). The text is correct once that's fixed. Nothing
to change here beyond the code fix.
##########
grails-core/src/test/groovy/grails/spring/GrailsPlaceholderConfigurerSpec.groovy:
##########
@@ -108,6 +119,155 @@ class GrailsPlaceholderConfigurerSpec extends
Specification {
bean.name == "test"
}
+
+ void "a placeholder naming a list of plain values resolves to the elements
of the list"() {
Review Comment:
Every feature here uses lowercase names (`app.names`, `app.origins`,
`app.items`), which is why the camelCase case passes the suite. Could you add a
feature for a camelCase key, such as `grails.cors.allowedOrigins: [a, b]`
injected through `${grails.cors.allowedOrigins}`, and ideally a kebab-case
placeholder (`${app.allowed-origins}`) against a camelCase key
(`app.allowedOrigins`)? Both fail on the current head without the `adapt`
change.
##########
grails-core/src/main/groovy/org/grails/spring/context/support/GrailsPlaceholderConfigurer.java:
##########
@@ -109,4 +149,35 @@ protected void visitMap(Map<?, ?> mapVal) {
// New in Spring 3.0: resolve placeholders in embedded values such as
annotation attributes.
beanFactoryToProcess.addEmbeddedValueResolver(valueResolver);
}
+
+ /**
+ * Presents each list of plain values in the environment under the name of
the list, its elements
+ * joined by commas.
+ */
+ private static final class ConfigurationListPropertySource extends
PropertySource<ConfigurableEnvironment> {
+
+ private final Binder binder;
+
+ ConfigurationListPropertySource(ConfigurableEnvironment environment) {
+ super(CONFIGURATION_LISTS_PROPERTY_SOURCE_NAME, environment);
+ // without a placeholder resolver: the placeholders in the
elements are resolved with the joined value
+ this.binder = new
Binder(ConfigurationPropertySources.get(environment));
+ }
+
+ @Override
+ public Object getProperty(String name) {
+ if (!ConfigurationPropertyName.isValid(name)) {
Review Comment:
`isValid` only accepts Spring Boot's canonical form (lowercase,
dot-separated, kebab-case), so any camelCase name returns `null` here and the
placeholder is left as written. I checked this against the PR head with a
context set up like the one in the spec, loading `{grails.cors.allowedOrigins:
[a, b], app.allowedOrigins: [c, d], app.names: [e, f]}`:
| placeholder | injected `String` |
|---|---|
| `${grails.cors.allowedOrigins}` | `${grails.cors.allowedOrigins}` |
| `${app.allowedOrigins}` | `${app.allowedOrigins}` |
| `${app.allowed-origins}` | `c,d` |
| `${app.names}` | `e,f` |
Before #16552, `${grails.cors.allowedOrigins}` resolved, so the regression
is still there for camelCase keys. Adapting the name instead of validating it
fixes it:
```java
ConfigurationPropertyName propertyName =
ConfigurationPropertyName.adapt(name, '.');
if (propertyName.isEmpty()) {
return null;
}
try {
return this.binder.bind(propertyName, Bindable.listOf(String.class))
```
With that change, all four placeholders above resolve (`a,b`, `c,d`, `c,d`,
`e,f`), and so does `${app.allowedOrigins}` against an `app.allowed-origins`
key, which is the same relaxed matching `@ConfigurationProperties` uses. All 11
features of `GrailsPlaceholderConfigurerSpec` still pass. The upgrade notes
already point readers to `adapt` for this reason in the `Binder` paragraph just
above.
--
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]
