matrei commented on code in PR #16798:
URL: https://github.com/apache/grails-core/pull/16798#discussion_r4431793058
##########
grails-doc/src/en/guide/conf/config/ymlPropertyOptions.adoc:
##########
@@ -34,6 +34,20 @@ production:
Similarly system arguments can be accessed.
+A placeholder is resolved wherever it appears in a value, including in an
element of a list or in a map inside one:
Review Comment:
This reads as if placeholders in list elements are now resolved everywhere,
but the change only covers `grailsApplication.config`. With Grails' YAML
loader, `NavigableMapPropertySource` also exposes the whole list under
`app.origins`. Both `Environment.getProperty('app.origins', List)` and Boot's
`Binder` return that list as it is, unresolved. The `Environment` resolves only
the by-index read (`app.origins[1]`).
I checked with:
```yaml
grails:
cors:
enabled: true
allowedOrigins:
- https://app.example.com
- '${EXTRA_ORIGIN:https://admin.example.com}'
```
- `config.getProperty('grails.cors.allowedOrigins', List)` gives
`[https://app.example.com, https://admin.example.com]` with this PR.
- `GrailsCorsConfiguration` bound through
`Binder.get(env).bind('grails.cors', ...)` gives `allowedOrigins ==
[https://app.example.com, ${EXTRA_ORIGIN:https://admin.example.com}]`, both on
7.0.x and with this PR.
- With Spring Boot's own YAML loader, the `Binder` gives
`https://admin.example.com`, because that loader only exposes the indexed keys.
So with this PR, `config` and `@ConfigurationProperties` beans disagree
about the same list. That doesn't block the code here. But I'd limit the docs
to `grailsApplication.config`, drop "as Spring's `Environment` does" for the
whole-list read (also in the table in the PR description), and open a follow-up
for the binder case. People reading this page will probably try it with
`grails.cors.allowedOrigins` first. Something like:
```suggestion
A placeholder in an element of a list, or in a map inside one, is resolved
when the list is read through `grailsApplication.config`:
```
##########
grails-doc/src/en/guide/conf/config/ymlPropertyOptions.adoc:
##########
@@ -34,6 +34,20 @@ production:
Similarly system arguments can be accessed.
+A placeholder is resolved wherever it appears in a value, including in an
element of a list or in a map inside one:
+
+[source,yaml]
+----
+app:
+ origins:
+ - https://app.example.com
+ - '${EXTRA_ORIGIN:https://admin.example.com}'
+----
+
+`grailsApplication.config` gives the resolved element whether the list is read
whole, with `config.getProperty('app.origins', List)`, or by index, with
`config.getProperty('app.origins[1]')`, as Spring's `Environment` does.
Review Comment:
```suggestion
`grailsApplication.config` gives the resolved element whether the list is
read whole, with `config.getProperty('app.origins', List)`, or by index, with
`config.getProperty('app.origins[1]')`.
NOTE: A list bound with `@ConfigurationProperties`, such as
`grails.cors.allowedOrigins`, still receives the placeholder as written when
`application.yml` is read through Grails' YAML loader.
```
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -203,6 +207,194 @@ class PropertySourcesConfigSpec extends Specification {
!config.containsKey('gorm.url')
}
+ void 'a placeholder in a list element from application.yml is resolved as
the environment resolves it'() {
+ given: "application.yml with a placeholder in a list of values and in
a list of maps"
+ def yml = """\
+app:
+ origins:
+ - https://a.example.com
+ - '\${EXAMPLE_ORIGIN:https://default.example.com}'
+ rows:
+ - url: '\${EXAMPLE_ORIGIN:https://default.example.com}'
+ name: first
+ db:
+ host: '\${EXAMPLE_ORIGIN:https://default.example.com}'
+ tags:
+ - '\${EXAMPLE_ORIGIN:https://default.example.com}'
+"""
+ def source = new YamlPropertySourceLoader().load('application.yml',
new ByteArrayResource(yml.bytes)).first()
+ def environment = new StandardEnvironment()
+ environment.propertySources.addFirst(source)
+ ConfigurationPropertySources.attach(environment)
+ def config = new PropertySourcesConfig(environment.propertySources)
+
+ expect: "the environment resolves each element"
+ environment.getProperty('app.origins[1]') ==
'https://default.example.com'
+ environment.getProperty('app.rows[0].url') ==
'https://default.example.com'
+
+ and: "the config answers the same, by index and in the whole list"
+ config.getProperty('app.origins[1]') == 'https://default.example.com'
+ config.getProperty('app.origins', List) == ['https://a.example.com',
'https://default.example.com']
+ config.navigate('app', 'origins') == ['https://a.example.com',
'https://default.example.com']
+ config.getProperty('app.rows[0].url') == 'https://default.example.com'
+ config.navigate('app', 'rows')[0].url == 'https://default.example.com'
+ config.navigate('app', 'rows')[0].name == 'first'
+
+ and: "so does a map or a list inside an element"
+ environment.getProperty('app.rows[0].db.host') ==
'https://default.example.com'
+ config.getProperty('app.rows[0].db.host') ==
'https://default.example.com'
+ config.navigate('app', 'rows')[0].db.host ==
'https://default.example.com'
+ config.navigate('app', 'rows')[0].tags ==
['https://default.example.com']
+ }
+
+ void 'a placeholder in a list held whole is resolved'() {
+ given: "a list held whole, as application.groovy gives it, of values,
of maps and of lists"
+ def map = new NavigableMap()
+ map.merge([app: [origins: ['https://a.example.com',
'${EXAMPLE_ORIGIN:https://default.example.com}'],
+ rows : [[url:
'${EXAMPLE_ORIGIN:https://default.example.com}', port: 8080]],
+ groups :
[['${EXAMPLE_ORIGIN:https://default.example.com}']]]], true)
+ def config = new PropertySourcesConfig(new
NavigableMapPropertySource('application.groovy', map))
+
+ expect:
+ config.getProperty('app.origins', List) == ['https://a.example.com',
'https://default.example.com']
+ config.navigate('app', 'origins') == ['https://a.example.com',
'https://default.example.com']
+ config.navigate('app', 'rows') == [[url:
'https://default.example.com', port: 8080]]
+ config.navigate('app', 'groups') == [['https://default.example.com']]
+
+ and: "the property source keeps the list as written"
Review Comment:
The description says the copy means "the property source keeps what it holds
and resolves again when the config is refreshed", but no feature calls
`refresh()`. I tried it: change the referenced value, call `config.refresh()`,
and the list element follows while the source stays as written. Since that's
the reason for the copy, could you add a feature for it?
##########
grails-core/src/main/groovy/org/grails/config/PropertySourcesConfig.java:
##########
@@ -115,15 +115,13 @@ private void
mergeEnumerablePropertySource(EnumerablePropertySource enumerablePr
final String[] propertyNames =
enumerablePropertySource.getPropertyNames();
for (String propertyName : propertyNames) {
Object value =
enumerablePropertySource.getProperty(propertyName);
- if (value instanceof ConfigObject) {
- if (((ConfigObject) value).isEmpty()) continue;
- } else {
- value = processAndEvaluate(value);
+ if (value instanceof ConfigObject && ((ConfigObject)
value).isEmpty()) {
+ continue;
}
- map.put(propertyName, value);
+ map.put(propertyName, processAndEvaluate(value));
}
- configMap.merge(map, true);
+ mergeResolved(map);
Review Comment:
The merge hook now resolves strings, lists and maps under indexed keys, and
it recurses into plain maps. Is `processAndEvaluate()` still needed here? I
tried merging the raw values instead:
```java
map.put(propertyName, value);
// ...
configMap.merge(map, true);
```
All config specs in grails-core still pass: 240 tests, including this PR's
spec and the escaped-placeholder feature. That would let us drop
`mergeResolved()` and the `resolvingPlaceholders` flag that is toggled around a
merge. Ordinary sources would then get the same per-entry cache as the YAML
path, so there would be one resolver instead of two.
The trade-off: `processAndEvaluate()` copies every list and map, so the
config never holds an ordinary source's own list instance. Without it, a list
with no placeholder from a `MapPropertySource` would be held as given. A later
indexed write from a higher-priority source (`app.list[1]`) would then change
it in place, which already happens today for lists from Grails' YAML loader. If
that copy matters, I'd rather keep it explicit than get it as a side effect of
a second resolver. I see the #16771 merge-up note relies on the flag, so let me
know if there's a reason I'm missing.
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -53,15 +57,116 @@
@Deprecated
public abstract class NavigableMapConfig implements Config {
protected static final Logger LOG =
LoggerFactory.getLogger(NavigableMapConfig.class);
+
+ /**
+ * A key in brackets that is not an index and holds no dot, {@code [x]} of
{@code routes[x]}.
+ */
+ private static final Pattern NAMED_SUBSCRIPT =
Pattern.compile("\\[([^.\\[\\]]*[^.\\[\\]0-9][^.\\[\\]]*)]");
protected ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
protected ConfigurableConversionService conversionService = new
DefaultConversionService();
+
+ /**
+ * Whether the merge resolves the placeholders in the values it writes. It
does not while it merges values that are
+ * resolved already, so a placeholder an escape kept as text is not
resolved a second time.
+ */
+ private boolean resolvingPlaceholders = true;
+
protected NavigableMap configMap = new NavigableMap() {
+
+ /**
+ * The texts the merge in progress has resolved, by the full name of
the entry and the text. The merge writes one
+ * entry under several keys: a dotted key under the whole key and in
its nested map, and a list element in its
+ * list in the nested map, in its list under the flat key and under
its indexed key. Every write takes one value,
+ * even for {@code ${random.uuid}}.
+ */
+ private Map<List<String>, String> resolvedTexts;
+
@Override
- protected Object mergeMapEntry(NavigableMap targetMap, String
sourceKey, Object newValue) {
- if (newValue instanceof CharSequence) {
- newValue = resolvePlaceholders(newValue.toString());
+ public void merge(Map sourceMap, boolean parseFlatKeys) {
+ resolvedTexts = new HashMap<>();
+ try {
+ super.merge(sourceMap, parseFlatKeys);
+ } finally {
+ resolvedTexts = null;
+ }
+ }
+
+ /**
+ * Resolves the placeholders in a value before it is written: in a
string, in the strings of a list, through
+ * the maps and lists it holds, and in a map written under an indexed
key, {@code rows[0]}, which is written into
+ * its list as it is. A map written under a plain key is merged entry
by entry, through this method.
+ */
+ @Override
+ protected void mergeMapEntry(NavigableMap rootMap, String path,
NavigableMap targetMap, String sourceKey,
+ Object sourceValue, boolean parseFlatKeys, boolean
isNestedSet) {
+ Object value = sourceValue;
+ if (resolvingPlaceholders && (sourceValue instanceof CharSequence
|| sourceValue instanceof List || sourceValue instanceof Map &&
isIndexed(sourceKey))) {
Review Comment:
Nit: `a || b || c && d` relies on operator precedence. Parentheses around
`sourceValue instanceof Map && isIndexed(sourceKey)` would make it easier to
read, and wrapping the line would help too.
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -203,6 +207,194 @@ class PropertySourcesConfigSpec extends Specification {
!config.containsKey('gorm.url')
}
+ void 'a placeholder in a list element from application.yml is resolved as
the environment resolves it'() {
+ given: "application.yml with a placeholder in a list of values and in
a list of maps"
+ def yml = """\
Review Comment:
Nit: `'''\` as in the later feature would avoid escaping `\${...}` here.
--
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]
